| Message ID | 20191106151439.30056-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] block: refactor rescan_partitions | expand |
On Wed, Nov 06, 2019 at 04:14:36PM +0100, Christoph Hellwig wrote:
> A lot of the logic in invalidate_partitions and invalidate_partitions
One of these should say 'rescan_partitions'. Otherwise, patch looks good.
On 11/6/19 4:14 PM, Christoph Hellwig wrote: > A lot of the logic in invalidate_partitions and invalidate_partitions ... rescan_partitions and invalidate_partitions ... > is shared. Merge the two functions to simplify things. There is > a small behavior change in that we now send the keven change notice > also if we were not invalidating but no partitions were found, which > seems like the right thing to do. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/ioctl.c | 2 +- > block/partition-generic.c | 38 ++++++++++++++------------------------ > fs/block_dev.c | 5 +---- > include/linux/genhd.h | 4 ++-- > 4 files changed, 18 insertions(+), 31 deletions(-) > > diff --git a/block/ioctl.c b/block/ioctl.c > index 15a0eb80ada9..8a7e33ce2097 100644 > --- a/block/ioctl.c > +++ b/block/ioctl.c > @@ -171,7 +171,7 @@ int __blkdev_reread_part(struct block_device *bdev) > > lockdep_assert_held(&bdev->bd_mutex); > > - return rescan_partitions(disk, bdev); > + return rescan_partitions(disk, bdev, false); > } > EXPORT_SYMBOL(__blkdev_reread_part); > > diff --git a/block/partition-generic.c b/block/partition-generic.c > index f113be069b40..eae9daa8a523 100644 > --- a/block/partition-generic.c > +++ b/block/partition-generic.c > @@ -632,7 +632,8 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev) > return ret; > } > > -int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > +int rescan_partitions(struct gendisk *disk, struct block_device *bdev, > + bool invalidate) > { > int ret; > > @@ -641,13 +642,22 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > if (ret) > return ret; > > - if (disk->fops->revalidate_disk) > + if (invalidate) > + set_capacity(disk, 0); > + else if (disk->fops->revalidate_disk) > disk->fops->revalidate_disk(disk); > - check_disk_size_change(disk, bdev, true); > + > + check_disk_size_change(disk, bdev, !invalidate); > bdev->bd_invalidated = 0; > > - if (!get_capacity(disk)) > + if (!get_capacity(disk)) { > + /* > + * Tell userspace that the media / partition table may have > + * changed. > + */ > + kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); > return 0; > + } > I wonder; wouldn't we miss a true size change here? check_disk_size_change() will issue a kernel message (but no usevent) if we have a real disk size change; if we fail to revalidate the disk an uevent is issued, but no kernel message. Can't we combine both by making check_disk_size_change() a bool function and drop the call to 'get_capacity()', seeing that it's done in check_disk_size_change() anyway? Cheers, Hannes
On Thu, Nov 07, 2019 at 10:47:39AM +0100, Hannes Reinecke wrote: > > + check_disk_size_change(disk, bdev, !invalidate); > > bdev->bd_invalidated = 0; > > > > - if (!get_capacity(disk)) > > + if (!get_capacity(disk)) { > > + /* > > + * Tell userspace that the media / partition table may have > > + * changed. > > + */ > > + kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); > > return 0; > > + } > > > I wonder; wouldn't we miss a true size change here? Why would we? We first unconditionally drop all partitions. Then if the device ha a non-zero capacity we probe for partitions. What could we miss?
On Wed 06-11-19 16:14:36, Christoph Hellwig wrote: > A lot of the logic in invalidate_partitions and invalidate_partitions ^^ rescan_partitions as others already mentioned > is shared. Merge the two functions to simplify things. There is > a small behavior change in that we now send the keven change notice ^^^ kevent > also if we were not invalidating but no partitions were found, which > seems like the right thing to do. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Otherwise the patch looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > block/ioctl.c | 2 +- > block/partition-generic.c | 38 ++++++++++++++------------------------ > fs/block_dev.c | 5 +---- > include/linux/genhd.h | 4 ++-- > 4 files changed, 18 insertions(+), 31 deletions(-) > > diff --git a/block/ioctl.c b/block/ioctl.c > index 15a0eb80ada9..8a7e33ce2097 100644 > --- a/block/ioctl.c > +++ b/block/ioctl.c > @@ -171,7 +171,7 @@ int __blkdev_reread_part(struct block_device *bdev) > > lockdep_assert_held(&bdev->bd_mutex); > > - return rescan_partitions(disk, bdev); > + return rescan_partitions(disk, bdev, false); > } > EXPORT_SYMBOL(__blkdev_reread_part); > > diff --git a/block/partition-generic.c b/block/partition-generic.c > index f113be069b40..eae9daa8a523 100644 > --- a/block/partition-generic.c > +++ b/block/partition-generic.c > @@ -632,7 +632,8 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev) > return ret; > } > > -int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > +int rescan_partitions(struct gendisk *disk, struct block_device *bdev, > + bool invalidate) > { > int ret; > > @@ -641,13 +642,22 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > if (ret) > return ret; > > - if (disk->fops->revalidate_disk) > + if (invalidate) > + set_capacity(disk, 0); > + else if (disk->fops->revalidate_disk) > disk->fops->revalidate_disk(disk); > - check_disk_size_change(disk, bdev, true); > + > + check_disk_size_change(disk, bdev, !invalidate); > bdev->bd_invalidated = 0; > > - if (!get_capacity(disk)) > + if (!get_capacity(disk)) { > + /* > + * Tell userspace that the media / partition table may have > + * changed. > + */ > + kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); > return 0; > + } > > ret = blk_add_partitions(disk, bdev); > if (ret == -EAGAIN) > @@ -655,26 +665,6 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > return ret; > } > > -int invalidate_partitions(struct gendisk *disk, struct block_device *bdev) > -{ > - int res; > - > - if (!bdev->bd_invalidated) > - return 0; > - > - res = drop_partitions(disk, bdev); > - if (res) > - return res; > - > - set_capacity(disk, 0); > - check_disk_size_change(disk, bdev, false); > - bdev->bd_invalidated = 0; > - /* tell userspace that the media / partition table may have changed */ > - kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); > - > - return 0; > -} > - > unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p) > { > struct address_space *mapping = bdev->bd_inode->i_mapping; > diff --git a/fs/block_dev.c b/fs/block_dev.c > index d612468ee66b..0af62b76d031 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -1511,10 +1511,7 @@ 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 (disk_part_scan_enabled(bdev->bd_disk)) { > - if (invalidate) > - invalidate_partitions(bdev->bd_disk, bdev); > - else > - rescan_partitions(bdev->bd_disk, bdev); > + rescan_partitions(bdev->bd_disk, bdev, invalidate); > } else { > check_disk_size_change(bdev->bd_disk, bdev, !invalidate); > bdev->bd_invalidated = 0; > diff --git a/include/linux/genhd.h b/include/linux/genhd.h > index 8b5330dd5ac0..fd7774e64f0b 100644 > --- a/include/linux/genhd.h > +++ b/include/linux/genhd.h > @@ -622,8 +622,8 @@ extern dev_t blk_lookup_devt(const char *name, int partno); > extern char *disk_name (struct gendisk *hd, int partno, char *buf); > > extern int disk_expand_part_tbl(struct gendisk *disk, int target); > -extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev); > -extern int invalidate_partitions(struct gendisk *disk, struct block_device *bdev); > +int rescan_partitions(struct gendisk *disk, struct block_device *bdev, > + bool invalidate); > extern struct hd_struct * __must_check add_partition(struct gendisk *disk, > int partno, sector_t start, > sector_t len, int flags, > -- > 2.20.1 >
diff --git a/block/ioctl.c b/block/ioctl.c index 15a0eb80ada9..8a7e33ce2097 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -171,7 +171,7 @@ int __blkdev_reread_part(struct block_device *bdev) lockdep_assert_held(&bdev->bd_mutex); - return rescan_partitions(disk, bdev); + return rescan_partitions(disk, bdev, false); } EXPORT_SYMBOL(__blkdev_reread_part); diff --git a/block/partition-generic.c b/block/partition-generic.c index f113be069b40..eae9daa8a523 100644 --- a/block/partition-generic.c +++ b/block/partition-generic.c @@ -632,7 +632,8 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev) return ret; } -int rescan_partitions(struct gendisk *disk, struct block_device *bdev) +int rescan_partitions(struct gendisk *disk, struct block_device *bdev, + bool invalidate) { int ret; @@ -641,13 +642,22 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) if (ret) return ret; - if (disk->fops->revalidate_disk) + if (invalidate) + set_capacity(disk, 0); + else if (disk->fops->revalidate_disk) disk->fops->revalidate_disk(disk); - check_disk_size_change(disk, bdev, true); + + check_disk_size_change(disk, bdev, !invalidate); bdev->bd_invalidated = 0; - if (!get_capacity(disk)) + if (!get_capacity(disk)) { + /* + * Tell userspace that the media / partition table may have + * changed. + */ + kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); return 0; + } ret = blk_add_partitions(disk, bdev); if (ret == -EAGAIN) @@ -655,26 +665,6 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) return ret; } -int invalidate_partitions(struct gendisk *disk, struct block_device *bdev) -{ - int res; - - if (!bdev->bd_invalidated) - return 0; - - res = drop_partitions(disk, bdev); - if (res) - return res; - - set_capacity(disk, 0); - check_disk_size_change(disk, bdev, false); - bdev->bd_invalidated = 0; - /* tell userspace that the media / partition table may have changed */ - kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); - - return 0; -} - unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p) { struct address_space *mapping = bdev->bd_inode->i_mapping; diff --git a/fs/block_dev.c b/fs/block_dev.c index d612468ee66b..0af62b76d031 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -1511,10 +1511,7 @@ 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 (disk_part_scan_enabled(bdev->bd_disk)) { - if (invalidate) - invalidate_partitions(bdev->bd_disk, bdev); - else - rescan_partitions(bdev->bd_disk, bdev); + rescan_partitions(bdev->bd_disk, bdev, invalidate); } else { check_disk_size_change(bdev->bd_disk, bdev, !invalidate); bdev->bd_invalidated = 0; diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 8b5330dd5ac0..fd7774e64f0b 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -622,8 +622,8 @@ extern dev_t blk_lookup_devt(const char *name, int partno); extern char *disk_name (struct gendisk *hd, int partno, char *buf); extern int disk_expand_part_tbl(struct gendisk *disk, int target); -extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev); -extern int invalidate_partitions(struct gendisk *disk, struct block_device *bdev); +int rescan_partitions(struct gendisk *disk, struct block_device *bdev, + bool invalidate); extern struct hd_struct * __must_check add_partition(struct gendisk *disk, int partno, sector_t start, sector_t len, int flags,
A lot of the logic in invalidate_partitions and invalidate_partitions is shared. Merge the two functions to simplify things. There is a small behavior change in that we now send the keven change notice also if we were not invalidating but no partitions were found, which seems like the right thing to do. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/ioctl.c | 2 +- block/partition-generic.c | 38 ++++++++++++++------------------------ fs/block_dev.c | 5 +---- include/linux/genhd.h | 4 ++-- 4 files changed, 18 insertions(+), 31 deletions(-)