| Message ID | 20230606073950.225178-9-hch@lst.de (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [01/31] block: also call ->open for incremental partition opens | expand |
On Tue, Jun 06, 2023 at 09:39:27AM +0200, Christoph Hellwig wrote: > Factor the common logic between disk_check_media_change and > disk_force_media_change into a helper. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Looks good to me, Acked-by: Christian Brauner <brauner@kernel.org>
On 6/6/23 09:39, Christoph Hellwig wrote: > Factor the common logic between disk_check_media_change and > disk_force_media_change into a helper. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/disk-events.c | 37 ++++++++++++++++--------------------- > 1 file changed, 16 insertions(+), 21 deletions(-) > > diff --git a/block/disk-events.c b/block/disk-events.c > index 8b1b63225738f8..06f325662b3494 100644 > --- a/block/disk-events.c > +++ b/block/disk-events.c > @@ -262,6 +262,18 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask) > return pending; > } > > +static bool __disk_check_media_change(struct gendisk *disk, unsigned int events) > +{ > + if (!(events & DISK_EVENT_MEDIA_CHANGE)) > + return false; > + > + if (__invalidate_device(disk->part0, true)) > + pr_warn("VFS: busy inodes on changed media %s\n", > + disk->disk_name); > + set_bit(GD_NEED_PART_SCAN, &disk->state); > + return true; > +} > + > /** > * disk_check_media_change - check if a removable media has been changed > * @disk: gendisk to check > @@ -274,18 +286,9 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask) > */ > bool disk_check_media_change(struct gendisk *disk) > { > - unsigned int events; > - > - events = disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | > - DISK_EVENT_EJECT_REQUEST); > - if (!(events & DISK_EVENT_MEDIA_CHANGE)) > - return false; > - > - if (__invalidate_device(disk->part0, true)) > - pr_warn("VFS: busy inodes on changed media %s\n", > - disk->disk_name); > - set_bit(GD_NEED_PART_SCAN, &disk->state); > - return true; > + return __disk_check_media_change(disk, > + disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | > + DISK_EVENT_EJECT_REQUEST)); Can you move the call to disk_clear_events() out of the call to __disk_check_media_change()? I find this pattern hard to read. Cheers, Hannes
On Wed, Jun 07, 2023 at 02:19:00PM +0200, Hannes Reinecke wrote: >> - return true; >> + return __disk_check_media_change(disk, >> + disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | >> + DISK_EVENT_EJECT_REQUEST)); > > Can you move the call to disk_clear_events() out of the call to > __disk_check_media_change()? > I find this pattern hard to read. I suspect you've not done enough functional programming in your youth :)
On 6/7/23 14:21, Christoph Hellwig wrote: > On Wed, Jun 07, 2023 at 02:19:00PM +0200, Hannes Reinecke wrote: >>> - return true; >>> + return __disk_check_media_change(disk, >>> + disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | >>> + DISK_EVENT_EJECT_REQUEST)); >> >> Can you move the call to disk_clear_events() out of the call to >> __disk_check_media_change()? >> I find this pattern hard to read. > > I suspect you've not done enough functional programming in your youth :) That's why I said 'I find'; purely personal preference. If you're happy with: Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes (In my youth? One is tempted to quote Falco: "If you remember the '90s you haven't experienced them...")
diff --git a/block/disk-events.c b/block/disk-events.c index 8b1b63225738f8..06f325662b3494 100644 --- a/block/disk-events.c +++ b/block/disk-events.c @@ -262,6 +262,18 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask) return pending; } +static bool __disk_check_media_change(struct gendisk *disk, unsigned int events) +{ + if (!(events & DISK_EVENT_MEDIA_CHANGE)) + return false; + + if (__invalidate_device(disk->part0, true)) + pr_warn("VFS: busy inodes on changed media %s\n", + disk->disk_name); + set_bit(GD_NEED_PART_SCAN, &disk->state); + return true; +} + /** * disk_check_media_change - check if a removable media has been changed * @disk: gendisk to check @@ -274,18 +286,9 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask) */ bool disk_check_media_change(struct gendisk *disk) { - unsigned int events; - - events = disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | - DISK_EVENT_EJECT_REQUEST); - if (!(events & DISK_EVENT_MEDIA_CHANGE)) - return false; - - if (__invalidate_device(disk->part0, true)) - pr_warn("VFS: busy inodes on changed media %s\n", - disk->disk_name); - set_bit(GD_NEED_PART_SCAN, &disk->state); - return true; + return __disk_check_media_change(disk, + disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE | + DISK_EVENT_EJECT_REQUEST)); } EXPORT_SYMBOL(disk_check_media_change); @@ -303,15 +306,7 @@ EXPORT_SYMBOL(disk_check_media_change); bool disk_force_media_change(struct gendisk *disk, unsigned int events) { disk_event_uevent(disk, events); - - if (!(events & DISK_EVENT_MEDIA_CHANGE)) - return false; - - if (__invalidate_device(disk->part0, true)) - pr_warn("VFS: busy inodes on changed media %s\n", - disk->disk_name); - set_bit(GD_NEED_PART_SCAN, &disk->state); - return true; + return __disk_check_media_change(disk, events); } EXPORT_SYMBOL_GPL(disk_force_media_change);
Factor the common logic between disk_check_media_change and disk_force_media_change into a helper. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/disk-events.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)