| Message ID | 20230203072819.3408-1-zhongjiezhu1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: core: hub: fix usb_hub worker blocking drain_all_pages() worker issue | expand |
On Fri, Feb 03, 2023 at 03:28:19PM +0800, Zhu Zhongjie wrote: > From: Zhongjie Zhu <zhongjiezhu1@gmail.com> > > When disconnecting a usb mass storege, if there are a lot of inodes > like 10 thousands files need to be freed, the invalidate_inodes() will > run for a loog time to freeing all inodes, this will block other worker > to run in the cpu, so mark the usb_hub workqueue to WQ_CPU_INTENSIVE to > avoid this situation. Very infrequently this will happen. In the vast majority of cases, the usb_hub workqueue uses very little CPU time. Marking it WQ_CPU_INTENSIVE seems inappropriate. Alan Stern
Yes, this is a very special case. It will happen only when disconnecting the mass storage if there are too many files in the storage, and the scanning operation is running, and the file system is not unmounted. It looks like this issue should be fixed in the usb mass storage driver, but I don't find an appropriate place. On Fri, Feb 3, 2023 at 10:47 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Feb 03, 2023 at 03:28:19PM +0800, Zhu Zhongjie wrote: > > From: Zhongjie Zhu <zhongjiezhu1@gmail.com> > > > > When disconnecting a usb mass storege, if there are a lot of inodes > > like 10 thousands files need to be freed, the invalidate_inodes() will > > run for a loog time to freeing all inodes, this will block other worker > > to run in the cpu, so mark the usb_hub workqueue to WQ_CPU_INTENSIVE to > > avoid this situation. > > Very infrequently this will happen. In the vast majority of cases, the > usb_hub workqueue uses very little CPU time. Marking it > WQ_CPU_INTENSIVE seems inappropriate. > > Alan Stern
On Mon, Feb 06, 2023 at 11:33:15AM +0800, 朱忠杰 wrote: > Yes, this is a very special case. > > It will happen only when disconnecting the mass storage if there are > too many files in the storage, and the scanning operation is running, > and the file system is not unmounted. > It looks like this issue should be fixed in the usb mass storage > driver, but I don't find an appropriate place. That's not surprising, because usb-storage doesn't know anything about what's happening on the mass-storage device it connects to. All it does is send the commands that it gets from the SCSI subsystem to the device and receive the results back. It has no idea whether there is a mounted filesystem on the device, if the filesystem contains any files, or whether a scanning operation is running, A better place to look for fixing this might be the filesystem code. That's where the information about mounting, files, and scanning can be found. Alan Stern
On Mon, Feb 6, 2023 at 11:17 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Feb 06, 2023 at 11:33:15AM +0800, 朱忠杰 wrote: > > Yes, this is a very special case. > > > > It will happen only when disconnecting the mass storage if there are > > too many files in the storage, and the scanning operation is running, > > and the file system is not unmounted. > > It looks like this issue should be fixed in the usb mass storage > > driver, but I don't find an appropriate place. > > That's not surprising, because usb-storage doesn't know anything about > what's happening on the mass-storage device it connects to. All it does > is send the commands that it gets from the SCSI subsystem to the device > and receive the results back. It has no idea whether there is a mounted > filesystem on the device, if the filesystem contains any files, or > whether a scanning operation is running, > > A better place to look for fixing this might be the filesystem code. > That's where the information about mounting, files, and scanning can be > found. > > Alan Stern The problem is there is a for loop in the invalidate_inodes(), this function is in the block device driver. when the usb_disconnect is called, the filesystem is not umounted, userspace applications will be noticed the usb storage is disconnected, and then do the umounting work. the invalidate_inodes() is called in the usb hub worker, and will run for a long time. To fix this issue, the long running loop need to be moved out from the usb hub worker.
On Tue, Feb 07, 2023 at 10:02:51AM +0800, Zhongjie Zhu wrote: > On Mon, Feb 6, 2023 at 11:17 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Mon, Feb 06, 2023 at 11:33:15AM +0800, 朱忠杰 wrote: > > > Yes, this is a very special case. > > > > > > It will happen only when disconnecting the mass storage if there are > > > too many files in the storage, and the scanning operation is running, > > > and the file system is not unmounted. > > > It looks like this issue should be fixed in the usb mass storage > > > driver, but I don't find an appropriate place. > > > > That's not surprising, because usb-storage doesn't know anything about > > what's happening on the mass-storage device it connects to. All it does > > is send the commands that it gets from the SCSI subsystem to the device > > and receive the results back. It has no idea whether there is a mounted > > filesystem on the device, if the filesystem contains any files, or > > whether a scanning operation is running, > > > > A better place to look for fixing this might be the filesystem code. > > That's where the information about mounting, files, and scanning can be > > found. > > > > Alan Stern > > The problem is there is a for loop in the invalidate_inodes(), this > function is in the block device driver. when the usb_disconnect is > called, the filesystem is not umounted, userspace applications will be > noticed the usb storage is disconnected, and then do the umounting > work. > the invalidate_inodes() is called in the usb hub worker, and will run > for a long time. To fix this issue, the long running loop need to be > moved out from the usb hub worker. Oh, maybe I didn't understand. You've got a USB mass-storage device with a mounted filesystem and a lot of dirty inodes, right? Then a USB disconnect happens, and as part of the disconnect processing, invalidate_inodes() runs for a long time. Do you know why it takes so long? The I/O operations shouldn't need any time; they will all fail immediately because the device has been disconnected and so there is no way to communicate with it. Alan Stern
On Tue, Feb 7, 2023 at 12:07 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, Feb 07, 2023 at 10:02:51AM +0800, Zhongjie Zhu wrote: > > On Mon, Feb 6, 2023 at 11:17 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Mon, Feb 06, 2023 at 11:33:15AM +0800, 朱忠杰 wrote: > > > > Yes, this is a very special case. > > > > > > > > It will happen only when disconnecting the mass storage if there are > > > > too many files in the storage, and the scanning operation is running, > > > > and the file system is not unmounted. > > > > It looks like this issue should be fixed in the usb mass storage > > > > driver, but I don't find an appropriate place. > > > > > > That's not surprising, because usb-storage doesn't know anything about > > > what's happening on the mass-storage device it connects to. All it does > > > is send the commands that it gets from the SCSI subsystem to the device > > > and receive the results back. It has no idea whether there is a mounted > > > filesystem on the device, if the filesystem contains any files, or > > > whether a scanning operation is running, > > > > > > A better place to look for fixing this might be the filesystem code. > > > That's where the information about mounting, files, and scanning can be > > > found. > > > > > > Alan Stern > > > > The problem is there is a for loop in the invalidate_inodes(), this > > function is in the block device driver. when the usb_disconnect is > > called, the filesystem is not umounted, userspace applications will be > > noticed the usb storage is disconnected, and then do the umounting > > work. > > the invalidate_inodes() is called in the usb hub worker, and will run > > for a long time. To fix this issue, the long running loop need to be > > moved out from the usb hub worker. > > Oh, maybe I didn't understand. > > You've got a USB mass-storage device with a mounted filesystem and a lot > of dirty inodes, right? Then a USB disconnect happens, and as part of > the disconnect processing, invalidate_inodes() runs for a long time. > > Do you know why it takes so long? The I/O operations shouldn't need any > time; they will all fail immediately because the device has been > disconnected and so there is no way to communicate with it. > > Alan Stern Yes, invalidate_inodes() will free all the inodes related to the supper_block, there are more than 20 thousands inodes (some times more) need to be freed, the perf record shows the cpu is busy running the spin_lock and spin_unlock in the invalidate_inodes(). The work in this function is to free all the inodes with the super_block. Maybe I need to find out why the spin_lock is running so much first.
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 9eca403af2a8..850549b30277 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -5904,7 +5904,8 @@ int usb_hub_init(void) * device was gone before the EHCI controller had handed its port * over to the companion full-speed controller. */ - hub_wq = alloc_workqueue("usb_hub_wq", WQ_FREEZABLE, 0); + hub_wq = alloc_workqueue("usb_hub_wq", + WQ_FREEZABLE | WQ_CPU_INTENSIVE, 0); if (hub_wq) return 0;