Message ID | 20191114143438.14681-3-hch@lst.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/7] block: refactor rescan_partitions | expand |
On 11/14/19 3:34 PM, Christoph Hellwig wrote: > A lot of the logic in invalidate_partitions and rescan_partitions is > shared. Merge the two functions to simplify things. There is a small > behavior change in that we now send the kevent 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> > Reviewed-by: Jan Kara <jack@suse.cz> > --- > 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(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
Hi Christoph, On Thu, Nov 14, 2019 at 03:34:33PM +0100, Christoph Hellwig wrote: > A lot of the logic in invalidate_partitions and rescan_partitions is > shared. Merge the two functions to simplify things. There is a small > behavior change in that we now send the kevent 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> > Reviewed-by: Jan Kara <jack@suse.cz> > --- > 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(-) > Mainline is broken for me because systemd-udevd spins forever using max CPU starting at boot time. I bisected it to this commit. I'm not an expert in this code, but the following patch fixes it: diff --git a/block/partition-generic.c b/block/partition-generic.c index 6b9f4f5d993a..b0eebd7580ab 100644 --- a/block/partition-generic.c +++ b/block/partition-generic.c @@ -599,7 +599,8 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev, * Tell userspace that the media / partition table may have * changed. */ - kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); + if (invalidate) + kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); return 0; } Do you have any better suggestion, or should I just send this as a formal patch? - Eric
On Sat, Nov 30, 2019 at 01:49:42PM -0800, Eric Biggers wrote: > Hi Christoph, > > On Thu, Nov 14, 2019 at 03:34:33PM +0100, Christoph Hellwig wrote: > > A lot of the logic in invalidate_partitions and rescan_partitions is > > shared. Merge the two functions to simplify things. There is a small > > behavior change in that we now send the kevent 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> > > Reviewed-by: Jan Kara <jack@suse.cz> > > --- > > 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(-) > > > > Mainline is broken for me because systemd-udevd spins forever using max CPU > starting at boot time. I bisected it to this commit. > > I'm not an expert in this code, but the following patch fixes it: > > diff --git a/block/partition-generic.c b/block/partition-generic.c > index 6b9f4f5d993a..b0eebd7580ab 100644 > --- a/block/partition-generic.c > +++ b/block/partition-generic.c > @@ -599,7 +599,8 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev, > * Tell userspace that the media / partition table may have > * changed. > */ > - kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); > + if (invalidate) > + kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE); > return 0; > } > > > Do you have any better suggestion, or should I just send this as a formal patch? > Actually this code was moved around more between the bad commit and mainline, so the fix for mainline would be: diff --git a/fs/block_dev.c b/fs/block_dev.c index ee63c2732fa2..69bf2fb6f7cd 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -1531,7 +1531,7 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate) ret = blk_add_partitions(disk, bdev); if (ret == -EAGAIN) goto rescan; - } else { + } else if (invalidate) { /* * Tell userspace that the media / partition table may have * changed.
On Sat, Nov 30, 2019 at 02:06:41PM -0800, Eric Biggers wrote: > Actually this code was moved around more between the bad commit and mainline, > so the fix for mainline would be: Can you send this JJens with a proper changelog and signoff? Whіle not sending the event for the non-partition case doesn't make a whole lot sense it clearly breaks userspace after we've done it for a long time.
diff --git a/block/ioctl.c b/block/ioctl.c index 8756efb1419e..f6576a6d5778 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 0909f66aab9d..da39a858fe47 100644 --- a/block/partition-generic.c +++ b/block/partition-generic.c @@ -576,7 +576,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; @@ -585,13 +586,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) @@ -599,26 +609,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,