On Tue 22-10-19 07:58:08, Damien Le Moal wrote:
> On 2019/10/21 17:38, Jan Kara wrote:
> > Factor out code handling revalidation of bdev on disk change into a
> > common helper.
> >
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
> > fs/block_dev.c | 26 ++++++++++++++------------
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 9c073dbdc1b0..88c6d35ec71d 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -1512,6 +1512,14 @@ EXPORT_SYMBOL(bd_set_size);
> >
> > static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
> >
> > +static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
> > +{
> > + if (invalidate)
> > + invalidate_partitions(bdev->bd_disk, bdev);
> > + else
> > + rescan_partitions(bdev->bd_disk, bdev);
> > +}
> > +
> > /*
> > * bd_mutex locking:
> > *
> > @@ -1594,12 +1602,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
> > * The latter is necessary to prevent ghost
> > * partitions on a removed medium.
> > */
> > - if (bdev->bd_invalidated) {
> > - if (!ret)
> > - rescan_partitions(disk, bdev);
> > - else if (ret == -ENOMEDIUM)
> > - invalidate_partitions(disk, bdev);
> > - }
> > + if (bdev->bd_invalidated &&
> > + (!ret || ret == -ENOMEDIUM))
> > + bdev_disk_changed(bdev, ret == -ENOMEDIUM);
>
> This is a little confusing since from its name, bdev_disk_changed() seem
> to imply that a new disk is present but this is called only if bdev is
> invalidated... What about calling this simply bdev_revalidate_disk(),
> since rescan_partitions() will call the disk revalidate method.
Honestly, the whole disk revalidation code is confusing to me :) I had to
draw a graph of which function calls which to understand what's going on in
that code and I think it could really use some refactoring. But that's
besides current point :)
Your "only if bdev is invalidated" is true but not actually a full story.
->bd_invalidated effectively gets set only through check_disk_change(). All
other places that end up calling flush_disk() clear bd_invalidated shortly
afterwards. So the function I've created is a direct counterpart to
check_disk_change() that just needs to happen after the device is
successfully open. I wanted to express that in the name - hence
bdev_disk_changed(). So yes, bdev_disk_changed() should be called exactly
when the new disk is present. It is bd_invalidated that is actually
misnamed.
Now I'm not really tied to bdev_disk_changed(). bdev_revalidate_disk()
seems a bit confusing to me though because the disk has actually been
already revalidated in check_disk_change() and the function won't
revalidate the disk for devices with partition scan disabled.
> Also, it seems to me that this function could be used without the
> complex "if ()" condition by slightly modifying it:
>
> static void bdev_revalidate_disk(struct block_device *bdev,
> bool invalidate)
> {
> if (bdev->bd_invalidated && invalidate)
> invalidate_partitions(bdev->bd_disk, bdev);
> else
> rescan_partitions(bdev->bd_disk, bdev);
> }
>
> Otherwise, this all looks fine to me.
Well, but you don't want to call rescan_partitions() if bd_invalidated is
not set. But yes, we could move bd_invalidated check into
bdev_disk_changed(), but then it would seem less clear why this function is
getting called. This ties somewhat to the discussion above. Hum, I guess if
we call the function just bdev_revalidate(), the name won't be confusing
and it would then make sense to move the bd_invalidated condition in there.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
