| Message ID | 20190327135105.30893-1-mwilck@suse.com |
|---|---|
| Headers | show |
| Series | block: skip media change event handling if unsupported | expand |
Hello Jens, as Hannes and Christoph have reviewed and acked this set, is there anything more I can do to get it merged? Best regards, Martin On Wed, 2019-03-27 at 14:51 +0100, Martin Wilck wrote: > The block layer currently can't distinguish between gendisk devices > that > don't support any media change events, and devices that do support > them, > but only for internal purposes. Therefore the check_events() function > is > called e.g. for ordinary non-removable SCSI disks. While these > devices > are not polled under normal conditions, the check_events function is > called on certain synchronization points, in particular while the > device > is opened, closed, or probed. > > Under unfavorable conditions this can lead to processes being stalled > on > a blocked queue: a close() schedules a work item for check_events() > which gets blocked in the work queue, and subsequent open() tries to > flush the workqueue. The flush then stalls too, as long as the the > blocked work item can't finish. > > In principle, the gendisk->events field would make it very easy for > the > block layer to check only for events actually supported by the > device. > Currently this is impossible, because there are lots of drivers which > don't set gendisk->events although they implement the check_events() > method. This was introduced in commit 7c88a168da80 ("block: don't > propagate unlisted DISK_EVENTs to userland") and follow-up patches > because uevent generation by these drivers was found to possibly > generate infinite event loops between kernel and user space. A side > effect of these patches was that the distinction between such devices > and devices supporting no events at all was lost. > > This series implements a slightly different approach to event > handling > and uevent suppression. The drivers are changed to set the events > field > again. Whether or not uevents should be generated is controlled by a > separate flag bit, which is only set by the drivers that are known to > generate proper uevents (sd and sr). Once this is done, devices that > don't support any media change events can be clearly identified, and > the > whole events checking code path can be skipped. This simplifies > handling > of non-removable SCSI disks. > > I have tested this with removable and non-removable SCSI disks, SCSI > cdrom, and ide-cd. > > This patch set targets the same problem as Hannes' late submission > "sd: > skip non-removable devices in sd_check_events()". > > Changes in v3: > > - Improved formatting of commit messages (Christoph) > - 2/5: Rather than adding the event flag bits to the "events" field, > add a new field "event_flags" to struct gendisk (Christoph) > - 2/5: Add a pair of parentheses around flag test (Christoph) > > Changes in v2: > > Removed the unused async_events field from struct gendisk. > This simplifies the event handling logic a bit. > > Martin Wilck (5): > block: genhd: remove async_events field > block: disk_events: introduce event flags > Revert "ide: unexport DISK_EVENT_MEDIA_CHANGE for ide-gd and ide- > cd" > Revert "block: unexport DISK_EVENT_MEDIA_CHANGE for legacy/fringe > drivers" > block: check_events: don't bother with events if unsupported > > block/genhd.c | 48 ++++++++++++++++++++++------------ > ---- > drivers/block/amiflop.c | 1 + > drivers/block/ataflop.c | 1 + > drivers/block/floppy.c | 1 + > drivers/block/paride/pcd.c | 1 + > drivers/block/paride/pd.c | 1 + > drivers/block/paride/pf.c | 1 + > drivers/block/pktcdvd.c | 1 - > drivers/block/swim.c | 1 + > drivers/block/swim3.c | 1 + > drivers/block/xsysace.c | 1 + > drivers/cdrom/gdrom.c | 1 + > drivers/ide/ide-cd.c | 1 + > drivers/ide/ide-cd_ioctl.c | 5 ++-- > drivers/ide/ide-gd.c | 6 +++-- > drivers/scsi/sd.c | 1 + > drivers/scsi/sr.c | 1 + > include/linux/genhd.h | 11 +++++++-- > 18 files changed, 57 insertions(+), 27 deletions(-) >
On 3/27/19 7:51 AM, Martin Wilck wrote: > The block layer currently can't distinguish between gendisk devices that > don't support any media change events, and devices that do support them, > but only for internal purposes. Therefore the check_events() function is > called e.g. for ordinary non-removable SCSI disks. While these devices > are not polled under normal conditions, the check_events function is > called on certain synchronization points, in particular while the device > is opened, closed, or probed. > > Under unfavorable conditions this can lead to processes being stalled on > a blocked queue: a close() schedules a work item for check_events() > which gets blocked in the work queue, and subsequent open() tries to > flush the workqueue. The flush then stalls too, as long as the the > blocked work item can't finish. > > In principle, the gendisk->events field would make it very easy for the > block layer to check only for events actually supported by the device. > Currently this is impossible, because there are lots of drivers which > don't set gendisk->events although they implement the check_events() > method. This was introduced in commit 7c88a168da80 ("block: don't > propagate unlisted DISK_EVENTs to userland") and follow-up patches > because uevent generation by these drivers was found to possibly > generate infinite event loops between kernel and user space. A side > effect of these patches was that the distinction between such devices > and devices supporting no events at all was lost. > > This series implements a slightly different approach to event handling > and uevent suppression. The drivers are changed to set the events field > again. Whether or not uevents should be generated is controlled by a > separate flag bit, which is only set by the drivers that are known to > generate proper uevents (sd and sr). Once this is done, devices that > don't support any media change events can be clearly identified, and the > whole events checking code path can be skipped. This simplifies handling > of non-removable SCSI disks. > > I have tested this with removable and non-removable SCSI disks, SCSI > cdrom, and ide-cd. > > This patch set targets the same problem as Hannes' late submission "sd: > skip non-removable devices in sd_check_events()". Applied for 5.2, thanks.
The block layer currently can't distinguish between gendisk devices that don't support any media change events, and devices that do support them, but only for internal purposes. Therefore the check_events() function is called e.g. for ordinary non-removable SCSI disks. While these devices are not polled under normal conditions, the check_events function is called on certain synchronization points, in particular while the device is opened, closed, or probed. Under unfavorable conditions this can lead to processes being stalled on a blocked queue: a close() schedules a work item for check_events() which gets blocked in the work queue, and subsequent open() tries to flush the workqueue. The flush then stalls too, as long as the the blocked work item can't finish. In principle, the gendisk->events field would make it very easy for the block layer to check only for events actually supported by the device. Currently this is impossible, because there are lots of drivers which don't set gendisk->events although they implement the check_events() method. This was introduced in commit 7c88a168da80 ("block: don't propagate unlisted DISK_EVENTs to userland") and follow-up patches because uevent generation by these drivers was found to possibly generate infinite event loops between kernel and user space. A side effect of these patches was that the distinction between such devices and devices supporting no events at all was lost. This series implements a slightly different approach to event handling and uevent suppression. The drivers are changed to set the events field again. Whether or not uevents should be generated is controlled by a separate flag bit, which is only set by the drivers that are known to generate proper uevents (sd and sr). Once this is done, devices that don't support any media change events can be clearly identified, and the whole events checking code path can be skipped. This simplifies handling of non-removable SCSI disks. I have tested this with removable and non-removable SCSI disks, SCSI cdrom, and ide-cd. This patch set targets the same problem as Hannes' late submission "sd: skip non-removable devices in sd_check_events()". Changes in v3: - Improved formatting of commit messages (Christoph) - 2/5: Rather than adding the event flag bits to the "events" field, add a new field "event_flags" to struct gendisk (Christoph) - 2/5: Add a pair of parentheses around flag test (Christoph) Changes in v2: Removed the unused async_events field from struct gendisk. This simplifies the event handling logic a bit. Martin Wilck (5): block: genhd: remove async_events field block: disk_events: introduce event flags Revert "ide: unexport DISK_EVENT_MEDIA_CHANGE for ide-gd and ide-cd" Revert "block: unexport DISK_EVENT_MEDIA_CHANGE for legacy/fringe drivers" block: check_events: don't bother with events if unsupported block/genhd.c | 48 ++++++++++++++++++++++---------------- drivers/block/amiflop.c | 1 + drivers/block/ataflop.c | 1 + drivers/block/floppy.c | 1 + drivers/block/paride/pcd.c | 1 + drivers/block/paride/pd.c | 1 + drivers/block/paride/pf.c | 1 + drivers/block/pktcdvd.c | 1 - drivers/block/swim.c | 1 + drivers/block/swim3.c | 1 + drivers/block/xsysace.c | 1 + drivers/cdrom/gdrom.c | 1 + drivers/ide/ide-cd.c | 1 + drivers/ide/ide-cd_ioctl.c | 5 ++-- drivers/ide/ide-gd.c | 6 +++-- drivers/scsi/sd.c | 1 + drivers/scsi/sr.c | 1 + include/linux/genhd.h | 11 +++++++-- 18 files changed, 57 insertions(+), 27 deletions(-)