| Message ID | 20231017184823.1383356-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] block: simplify bdev_del_partition() | expand |
On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote: > disk_check_media_change is mostly called from ->open where it makes > little sense to mark the file system on the device as dead, as we > are just opening it. So instead of calling bdev_mark_dead from > disk_check_media_change move it into the few callers that are not > in an open instance. This avoid calling into bdev_mark_dead and > thus taking s_umount with open_mutex held. ->open() is called when opening bdev every times, and there can be existed openers, so not sure if it makes little sense here. Thanks, Ming
On Wed, Oct 18, 2023 at 11:16:28AM +0800, Ming Lei wrote: > On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote: > > disk_check_media_change is mostly called from ->open where it makes > > little sense to mark the file system on the device as dead, as we > > are just opening it. So instead of calling bdev_mark_dead from > > disk_check_media_change move it into the few callers that are not > > in an open instance. This avoid calling into bdev_mark_dead and > > thus taking s_umount with open_mutex held. > > ->open() is called when opening bdev every times, and there can be > existed openers, so not sure if it makes little sense here. Yes. It has to be a non-exclusive open, though, and how is that going to help us with any general use case? If we really want to make the media change notifications any useful we'd better just do the work straight from the workqueue that polls for it.
On Wed, Oct 18, 2023 at 08:46:46AM +0200, Christoph Hellwig wrote: > On Wed, Oct 18, 2023 at 11:16:28AM +0800, Ming Lei wrote: > > On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote: > > > disk_check_media_change is mostly called from ->open where it makes > > > little sense to mark the file system on the device as dead, as we > > > are just opening it. So instead of calling bdev_mark_dead from > > > disk_check_media_change move it into the few callers that are not > > > in an open instance. This avoid calling into bdev_mark_dead and > > > thus taking s_umount with open_mutex held. > > > > ->open() is called when opening bdev every times, and there can be > > existed openers, so not sure if it makes little sense here. > > Yes. It has to be a non-exclusive open, though, and how is that going > to help us with any general use case? If we really want to make the > media change notifications any useful we'd better just do the work > straight from the workqueue that polls for it. Given ->mark_dead() is added recently, userspace shouldn't depend on this behavior, so this patch looks fine: Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote: > disk_check_media_change is mostly called from ->open where it makes > little sense to mark the file system on the device as dead, as we > are just opening it. So instead of calling bdev_mark_dead from > disk_check_media_change move it into the few callers that are not > in an open instance. This avoid calling into bdev_mark_dead and > thus taking s_umount with open_mutex held. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Looks good to me, Reviewed-by: Christian Brauner <brauner@kernel.org>
On Wed, Oct 18, 2023 at 05:15:36PM +0800, Ming Lei wrote: > > Yes. It has to be a non-exclusive open, though, and how is that going > > to help us with any general use case? If we really want to make the > > media change notifications any useful we'd better just do the work > > straight from the workqueue that polls for it. > > Given ->mark_dead() is added recently, userspace shouldn't depend on > this behavior, so this patch looks fine: While the method is new, disk_check_media_change called __invalidate_device before, which ended up both taking s_umount and thrashing the buffer cache under the fs.
I turns out that we'd need bdev_mark_dead generally exported for this. I don't quite like that, but I don't really see a way around it. Maybe fix that up in your tree?
On Thu, Oct 19, 2023 at 07:57:40AM +0200, Christoph Hellwig wrote: > I turns out that we'd need bdev_mark_dead generally exported for this. > I don't quite like that, but I don't really see a way around it. > Maybe fix that up in your tree? Done.
On Tue 17-10-23 20:48:21, Christoph Hellwig wrote: > disk_check_media_change is mostly called from ->open where it makes > little sense to mark the file system on the device as dead, as we > are just opening it. So instead of calling bdev_mark_dead from > disk_check_media_change move it into the few callers that are not > in an open instance. This avoid calling into bdev_mark_dead and > thus taking s_umount with open_mutex held. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Nice and looks good to me (with the fixed up export). Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > block/disk-events.c | 18 +++++++----------- > drivers/block/ataflop.c | 4 +++- > drivers/block/floppy.c | 4 +++- > 3 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/block/disk-events.c b/block/disk-events.c > index 13c3372c465a39..2f697224386aa8 100644 > --- a/block/disk-events.c > +++ b/block/disk-events.c > @@ -266,11 +266,8 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask) > * disk_check_media_change - check if a removable media has been changed > * @disk: gendisk to check > * > - * Check whether a removable media has been changed, and attempt to free all > - * dentries and inodes and invalidates all block device page cache entries in > - * that case. > - * > - * Returns %true if the media has changed, or %false if not. > + * Returns %true and marks the disk for a partition rescan whether a removable > + * media has been changed, and %false if the media did not change. > */ > bool disk_check_media_change(struct gendisk *disk) > { > @@ -278,12 +275,11 @@ bool disk_check_media_change(struct gendisk *disk) > > events = disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | > DISK_EVENT_EJECT_REQUEST); > - if (!(events & DISK_EVENT_MEDIA_CHANGE)) > - return false; > - > - bdev_mark_dead(disk->part0, true); > - set_bit(GD_NEED_PART_SCAN, &disk->state); > - return true; > + if (events & DISK_EVENT_MEDIA_CHANGE) { > + set_bit(GD_NEED_PART_SCAN, &disk->state); > + return true; > + } > + return false; > } > EXPORT_SYMBOL(disk_check_media_change); > > diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c > index cd738cab725f39..50949207798d2a 100644 > --- a/drivers/block/ataflop.c > +++ b/drivers/block/ataflop.c > @@ -1760,8 +1760,10 @@ static int fd_locked_ioctl(struct block_device *bdev, blk_mode_t mode, > /* invalidate the buffer track to force a reread */ > BufferDrive = -1; > set_bit(drive, &fake_change); > - if (disk_check_media_change(disk)) > + if (disk_check_media_change(disk)) { > + bdev_mark_dead(disk->part0, true); > floppy_revalidate(disk); > + } > return 0; > default: > return -EINVAL; > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > index ea4eb88a2e45f4..11114a5d9e5c46 100644 > --- a/drivers/block/floppy.c > +++ b/drivers/block/floppy.c > @@ -3215,8 +3215,10 @@ static int invalidate_drive(struct gendisk *disk) > /* invalidate the buffer track to force a reread */ > set_bit((long)disk->private_data, &fake_change); > process_fd_request(); > - if (disk_check_media_change(disk)) > + if (disk_check_media_change(disk)) { > + bdev_mark_dead(disk->part0, true); > floppy_revalidate(disk); > + } > return 0; > } > > -- > 2.39.2 >
diff --git a/block/disk-events.c b/block/disk-events.c index 13c3372c465a39..2f697224386aa8 100644 --- a/block/disk-events.c +++ b/block/disk-events.c @@ -266,11 +266,8 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask) * disk_check_media_change - check if a removable media has been changed * @disk: gendisk to check * - * Check whether a removable media has been changed, and attempt to free all - * dentries and inodes and invalidates all block device page cache entries in - * that case. - * - * Returns %true if the media has changed, or %false if not. + * Returns %true and marks the disk for a partition rescan whether a removable + * media has been changed, and %false if the media did not change. */ bool disk_check_media_change(struct gendisk *disk) { @@ -278,12 +275,11 @@ bool disk_check_media_change(struct gendisk *disk) events = disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | DISK_EVENT_EJECT_REQUEST); - if (!(events & DISK_EVENT_MEDIA_CHANGE)) - return false; - - bdev_mark_dead(disk->part0, true); - set_bit(GD_NEED_PART_SCAN, &disk->state); - return true; + if (events & DISK_EVENT_MEDIA_CHANGE) { + set_bit(GD_NEED_PART_SCAN, &disk->state); + return true; + } + return false; } EXPORT_SYMBOL(disk_check_media_change); diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c index cd738cab725f39..50949207798d2a 100644 --- a/drivers/block/ataflop.c +++ b/drivers/block/ataflop.c @@ -1760,8 +1760,10 @@ static int fd_locked_ioctl(struct block_device *bdev, blk_mode_t mode, /* invalidate the buffer track to force a reread */ BufferDrive = -1; set_bit(drive, &fake_change); - if (disk_check_media_change(disk)) + if (disk_check_media_change(disk)) { + bdev_mark_dead(disk->part0, true); floppy_revalidate(disk); + } return 0; default: return -EINVAL; diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index ea4eb88a2e45f4..11114a5d9e5c46 100644 --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -3215,8 +3215,10 @@ static int invalidate_drive(struct gendisk *disk) /* invalidate the buffer track to force a reread */ set_bit((long)disk->private_data, &fake_change); process_fd_request(); - if (disk_check_media_change(disk)) + if (disk_check_media_change(disk)) { + bdev_mark_dead(disk->part0, true); floppy_revalidate(disk); + } return 0; }
disk_check_media_change is mostly called from ->open where it makes little sense to mark the file system on the device as dead, as we are just opening it. So instead of calling bdev_mark_dead from disk_check_media_change move it into the few callers that are not in an open instance. This avoid calling into bdev_mark_dead and thus taking s_umount with open_mutex held. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/disk-events.c | 18 +++++++----------- drivers/block/ataflop.c | 4 +++- drivers/block/floppy.c | 4 +++- 3 files changed, 13 insertions(+), 13 deletions(-)