| Message ID | 20191021083808.19335-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bdev: Refresh bdev size for disks without partitioning | expand |
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@suse.cz> > --- > 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. 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. > > if (ret) > goto out_clear; > @@ -1632,12 +1637,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part) > if (bdev->bd_disk->fops->open) > ret = bdev->bd_disk->fops->open(bdev, mode); > /* the same as first opener case, read comment there */ > - if (bdev->bd_invalidated) { > - if (!ret) > - rescan_partitions(bdev->bd_disk, bdev); > - else if (ret == -ENOMEDIUM) > - invalidate_partitions(bdev->bd_disk, bdev); > - } > + if (bdev->bd_invalidated && > + (!ret || ret == -ENOMEDIUM)) > + bdev_disk_changed(bdev, ret == -ENOMEDIUM); > if (ret) > goto out_unlock_bdev; > } >
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@suse.cz> > > --- > > 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
On 2019/10/22 18:15, Jan Kara wrote: > 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@suse.cz> >>> --- >>> 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 :) OK. I guess I got confused too... > > 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. Got it. > > 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. Indeed, we don't want another partition scan. Missed that one. For the function name, since the goal is to revalidate only the bdev capacity, what about bdev_revalidate_capacity() then ? But looking at the code and seeing the partitions functions calls does not clarify things though. Oh well, keep the name you proposed and we can cleanup everything with a refactor :) Cheers.
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); if (ret) goto out_clear; @@ -1632,12 +1637,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part) if (bdev->bd_disk->fops->open) ret = bdev->bd_disk->fops->open(bdev, mode); /* the same as first opener case, read comment there */ - if (bdev->bd_invalidated) { - if (!ret) - rescan_partitions(bdev->bd_disk, bdev); - else if (ret == -ENOMEDIUM) - invalidate_partitions(bdev->bd_disk, bdev); - } + if (bdev->bd_invalidated && + (!ret || ret == -ENOMEDIUM)) + bdev_disk_changed(bdev, ret == -ENOMEDIUM); if (ret) goto out_unlock_bdev; }
Factor out code handling revalidation of bdev on disk change into a common helper. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/block_dev.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)