diff mbox series

[2/7] block: merge invalidate_partitions into rescan_partitions

Message ID 20191114143438.14681-3-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/7] block: refactor rescan_partitions | expand

Commit Message

Christoph Hellwig Nov. 14, 2019, 2:34 p.m. UTC 
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(-)

Comments

Hannes Reinecke Nov. 15, 2019, 10:05 a.m. UTC | #1 
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
Eric Biggers Nov. 30, 2019, 9:49 p.m. UTC | #2 
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
Eric Biggers Nov. 30, 2019, 10:06 p.m. UTC | #3 
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.
Christoph Hellwig Dec. 2, 2019, 7:21 a.m. UTC | #4 
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 mbox series

Patch

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,