[PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed

[PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Ming Lei]

Hi Guys,

The two patches changes block ioctl(BLKRRPART) for avoiding drop &
re-add partitions if partitions state isn't changed. The current
behavior confuses userspace because partitions can disappear anytime
when ioctl(BLKRRPART).

Ming Lei (2):
  block: move partitions check code into single helper
  block: avoid to drop & re-add partitions if partitions aren't changed

 block/genhd.c            |   2 +
 block/partitions/check.h |   2 +
 block/partitions/core.c  | 101 ++++++++++++++++++++++++++++++++-------
 fs/block_dev.c           |  28 +++++++++--
 include/linux/genhd.h    |   4 ++
 5 files changed, 118 insertions(+), 19 deletions(-)

Cc: Ewan D. Milne <emilne@redhat.com>
-- 
2.29.2

[PATCH 1/2] block: move partitions check code into single helper

[Ming Lei]

No functional change, make code more readable, and prepare for
supporting safe re-read partitions.

Cc: Ewan D. Milne <emilne@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/partitions/core.c | 51 ++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index b1cdf88f96e2..154013ea8623 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -599,17 +599,15 @@ static bool blk_add_partition(struct gendisk *disk, struct block_device *bdev,
 	return true;
 }
 
-int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
+static int blk_check_partitions(struct gendisk *disk,
+		struct block_device *bdev, struct parsed_partitions **s)
 {
-	struct parsed_partitions *state;
-	int ret = -EAGAIN, p;
-
-	if (!disk_part_scan_enabled(disk))
-		return 0;
+	int ret = -EAGAIN;
+	struct parsed_partitions *state = check_partition(disk, bdev);
 
-	state = check_partition(disk, bdev);
 	if (!state)
-		return 0;
+		goto out;
+
 	if (IS_ERR(state)) {
 		/*
 		 * I/O error reading the partition table.  If we tried to read
@@ -647,15 +645,40 @@ int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
 			goto out_free_state;
 	}
 
+out:
+	*s = state;
+	return 0;
+
+out_free_state:
+	free_partitions(state);
+	*s = NULL;
+	return ret;
+}
+
+int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
+{
+	struct parsed_partitions *state;
+	int ret, p;
+
+	if (!disk_part_scan_enabled(disk))
+		return 0;
+
+	ret = blk_check_partitions(disk, bdev, &state);
+	if (ret != 0)
+		return ret;
+
+	if (!state)
+		return 0;
+
 	/* tell userspace that the media / partition table may have changed */
 	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
 
-	for (p = 1; p < state->limit; p++)
-		if (!blk_add_partition(disk, bdev, state, p))
-			goto out_free_state;
-
-	ret = 0;
-out_free_state:
+	for (p = 1; p < state->limit; p++) {
+		if (!blk_add_partition(disk, bdev, state, p)) {
+			ret = -EAGAIN;
+			break;
+		}
+	}
 	free_partitions(state);
 	return ret;
 }
-- 
2.29.2

[PATCH 2/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Ming Lei]

block ioctl(BLKRRPART) always drops current partitions and adds
partitions again, even though there isn't any change in partitions table.

ioctl(BLKRRPART) may be called by systemd-udevd and some disk utilities
frequently. When it is run, partitions disk node are dropped and added
back, this way may confuse userspace or users, for example, one normal
workable partition device node may disappear any time.

Fix this issue by checking if there is real change in partitions state,
and only drop & re-add them when partitions state is really changed.

Cc: Ewan D. Milne <emilne@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/genhd.c            |  2 ++
 block/partitions/check.h |  2 ++
 block/partitions/core.c  | 76 ++++++++++++++++++++++++++++++++--------
 fs/block_dev.c           | 28 +++++++++++++--
 include/linux/genhd.h    |  4 +++
 5 files changed, 94 insertions(+), 18 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 304f8dcc9a9b..fbc8961c0a72 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -26,6 +26,7 @@
 #include <linux/badblocks.h>
 
 #include "blk.h"
+#include "partitions/check.h"
 
 static struct kobject *block_depr;
 
@@ -1215,6 +1216,7 @@ static void disk_release(struct device *dev)
 	bdput(disk->part0);
 	if (disk->queue)
 		blk_put_queue(disk->queue);
+	blk_free_partitions(disk);
 	kfree(disk);
 }
 struct class block_class = {
diff --git a/block/partitions/check.h b/block/partitions/check.h
index c577e9ee67f0..fc2ec2acddd2 100644
--- a/block/partitions/check.h
+++ b/block/partitions/check.h
@@ -68,3 +68,5 @@ int sgi_partition(struct parsed_partitions *state);
 int sun_partition(struct parsed_partitions *state);
 int sysv68_partition(struct parsed_partitions *state);
 int ultrix_partition(struct parsed_partitions *state);
+
+void blk_free_partitions(struct gendisk *hd);
diff --git a/block/partitions/core.c b/block/partitions/core.c
index 154013ea8623..277367b275ab 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -116,8 +116,15 @@ static struct parsed_partitions *allocate_partitions(struct gendisk *hd)
 
 static void free_partitions(struct parsed_partitions *state)
 {
-	vfree(state->parts);
-	kfree(state);
+	if (state) {
+		vfree(state->parts);
+		kfree(state);
+	}
+}
+
+void blk_free_partitions(struct gendisk *hd)
+{
+	free_partitions(hd->parts_state);
 }
 
 static struct parsed_partitions *check_partition(struct gendisk *hd,
@@ -655,32 +662,71 @@ static int blk_check_partitions(struct gendisk *disk,
 	return ret;
 }
 
+static bool partitions_changed(const struct parsed_partitions *old,
+		const struct parsed_partitions *new)
+{
+	if (old == new)	/* both are NULL */
+		return false;
+	if (!old || !new)
+		return true;
+
+	if (memcmp(old->name, new->name, BDEVNAME_SIZE))
+		return true;
+	if (old->limit != new->limit)
+		return true;
+	if (memcmp(old->parts, new->parts, old->limit * sizeof(old->parts[0])))
+		return true;
+	return old->next != new->next;
+}
+
+/* Return true if partitions state is changed */
+bool blk_update_partitions(struct gendisk *disk, struct block_device *bdev,
+		int *retval)
+{
+	struct parsed_partitions *state;
+
+	lockdep_assert_held(&bdev->bd_mutex);
+
+	*retval = -EAGAIN;
+	if (!get_capacity(disk))
+		return true;
+
+	*retval = blk_check_partitions(disk, bdev, &state);
+	if (*retval)
+		return true;
+
+	if (partitions_changed(disk->parts_state, state)) {
+		/* update to new partitions state */
+		free_partitions(disk->parts_state);
+		disk->parts_state = state;
+		return true;
+	}
+
+	free_partitions(state);
+	return false;
+}
+
 int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
 {
 	struct parsed_partitions *state;
-	int ret, p;
+	int p;
 
 	if (!disk_part_scan_enabled(disk))
 		return 0;
 
-	ret = blk_check_partitions(disk, bdev, &state);
-	if (ret != 0)
-		return ret;
+	/* tell userspace that the media / partition table may have changed */
+	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
 
+	state = disk->parts_state;
 	if (!state)
 		return 0;
 
-	/* tell userspace that the media / partition table may have changed */
-	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
-
 	for (p = 1; p < state->limit; p++) {
-		if (!blk_add_partition(disk, bdev, state, p)) {
-			ret = -EAGAIN;
-			break;
-		}
+		if (!blk_add_partition(disk, bdev, state, p))
+			return -EAGAIN;
 	}
-	free_partitions(state);
-	return ret;
+
+	return 0;
 }
 
 void *read_part_sector(struct parsed_partitions *state, sector_t n, Sector *p)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9d4b1a884d76..6d9a832f4e71 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1220,11 +1220,28 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 {
 	struct gendisk *disk = bdev->bd_disk;
 	int ret;
+	bool parts_valid = false;
 
 	lockdep_assert_held(&bdev->bd_mutex);
 
 	clear_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
 
+	if (!invalidate) {
+		sync_blockdev(bdev);
+		invalidate_bdev(bdev);
+		if (disk->fops->revalidate_disk)
+			disk->fops->revalidate_disk(disk);
+		/*
+		 * Return immediately if partitions state aren't changed,
+		 * then we can avoid partition removal & readd, which may
+		 * confuse userspace.
+		 */
+		if (!blk_update_partitions(disk, bdev, &ret))
+			return 0;
+		if (!ret)
+			parts_valid = true;
+	}
+
 rescan:
 	ret = blk_drop_partitions(bdev);
 	if (ret)
@@ -1243,14 +1260,19 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 		    !(disk->flags & GENHD_FL_REMOVABLE))
 			set_capacity(disk, 0);
 	} else {
-		if (disk->fops->revalidate_disk)
+		if (!parts_valid && disk->fops->revalidate_disk)
 			disk->fops->revalidate_disk(disk);
 	}
 
 	if (get_capacity(disk)) {
-		ret = blk_add_partitions(disk, bdev);
-		if (ret == -EAGAIN)
+		if (!parts_valid)
+			blk_update_partitions(disk, bdev, &ret);
+		if (!ret)
+			ret = blk_add_partitions(disk, bdev);
+		if (ret == -EAGAIN) {
+			parts_valid = false;
 			goto rescan;
+		}
 	} else if (invalidate) {
 		/*
 		 * Tell userspace that the media / partition table may have
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index f364619092cc..4f517305f81f 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -119,6 +119,7 @@ enum {
 
 struct disk_events;
 struct badblocks;
+struct parsed_partitions;
 
 struct blk_integrity {
 	const struct blk_integrity_profile	*profile;
@@ -166,6 +167,7 @@ struct gendisk {
 #endif
 	int node_id;
 	struct badblocks *bb;
+	struct parsed_partitions *parts_state;
 	struct lockdep_map lockdep_map;
 };
 
@@ -274,6 +276,8 @@ static inline sector_t get_capacity(struct gendisk *disk)
 int bdev_disk_changed(struct block_device *bdev, bool invalidate);
 int blk_add_partitions(struct gendisk *disk, struct block_device *bdev);
 int blk_drop_partitions(struct block_device *bdev);
+bool blk_update_partitions(struct gendisk *disk, struct block_device *bdev,
+		int *retval);
 
 extern struct gendisk *__alloc_disk_node(int minors, int node_id);
 extern void put_disk(struct gendisk *disk);
-- 
2.29.2

Re: [PATCH 2/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Christoph Hellwig]

On Fri, Feb 05, 2021 at 10:17:08AM +0800, Ming Lei wrote:
> block ioctl(BLKRRPART) always drops current partitions and adds
> partitions again, even though there isn't any change in partitions table.
> 
> ioctl(BLKRRPART) may be called by systemd-udevd and some disk utilities
> frequently.

Err, why?  We should probably fix udev to not do stupid things first.

Re: [PATCH 2/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Ming Lei]

On Fri, Feb 05, 2021 at 08:14:29AM +0100, Christoph Hellwig wrote:
> On Fri, Feb 05, 2021 at 10:17:08AM +0800, Ming Lei wrote:
> > block ioctl(BLKRRPART) always drops current partitions and adds
> > partitions again, even though there isn't any change in partitions table.
> > 
> > ioctl(BLKRRPART) may be called by systemd-udevd and some disk utilities
> > frequently.
> 
> Err, why?  We should probably fix udev to not do stupid things first.

It is one standard syscall, and the command is just for re-read
partition table, and it can be called by any application, fdisk
calls it too even though no any change done on the disk data,
same with parted, and there should be more.

	#define BLKRRPART  _IO(0x12,95) /* re-read partition table */

IMO, this syscall isn't supposed to drop partitions if user doesn't
touch the partition table, do you think it is one sane behavior to
drop partitions at will?

-- 
Ming

Re: [PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Ming Lei]

On Fri, Feb 05, 2021 at 10:17:06AM +0800, Ming Lei wrote:
> Hi Guys,
> 
> The two patches changes block ioctl(BLKRRPART) for avoiding drop &
> re-add partitions if partitions state isn't changed. The current
> behavior confuses userspace because partitions can disappear anytime
> when ioctl(BLKRRPART).
> 
> Ming Lei (2):
>   block: move partitions check code into single helper
>   block: avoid to drop & re-add partitions if partitions aren't changed
> 
>  block/genhd.c            |   2 +
>  block/partitions/check.h |   2 +
>  block/partitions/core.c  | 101 ++++++++++++++++++++++++++++++++-------
>  fs/block_dev.c           |  28 +++++++++--
>  include/linux/genhd.h    |   4 ++
>  5 files changed, 118 insertions(+), 19 deletions(-)
> 
> Cc: Ewan D. Milne <emilne@redhat.com>
> -- 
> 2.29.2
> 

Hello,

Ping...

-- 
Ming

Re: [PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Christoph Hellwig]

On Mon, Feb 15, 2021 at 12:03:41PM +0800, Ming Lei wrote:
> Hello,

I think this is a fundamentally bad idea.  We should not keep the
parsed partition state around forever just to work around some buggy
user space software.

Re: [PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Ming Lei]

On Tue, Feb 16, 2021 at 09:44:30AM +0100, Christoph Hellwig wrote:
> On Mon, Feb 15, 2021 at 12:03:41PM +0800, Ming Lei wrote:
> > Hello,
> 
> I think this is a fundamentally bad idea.  We should not keep the
> parsed partition state around forever just to work around some buggy
> user space software.

What is the bug in userspace software?

Do you think it is correct for ioctl(BLKRRPART) to always drop/re-add
partition device node?

-- 
Ming

Re: [PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Christoph Hellwig]

On Wed, Feb 17, 2021 at 11:07:14AM +0800, Ming Lei wrote:
> Do you think it is correct for ioctl(BLKRRPART) to always drop/re-add
> partition device node?

Yes, that is what it is designed to do.  The only reason to call this
ioctl is when userspace software has written new partition table
information to the disk.

Re: [PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed

[Ming Lei]

On Wed, Feb 17, 2021 at 08:16:29AM +0100, Christoph Hellwig wrote:
> On Wed, Feb 17, 2021 at 11:07:14AM +0800, Ming Lei wrote:
> > Do you think it is correct for ioctl(BLKRRPART) to always drop/re-add
> > partition device node?
> 
> Yes, that is what it is designed to do.  The only reason to call this
> ioctl is when userspace software has written new partition table
> information to the disk.

I am wondering how userspace can know this design or implication since
this behavior wasn't documented anywhere.

For example, 'blockdev --rereadpt' can do it simply, without updating
partition table at all.

The reality is that almost of all the main userspace consumers of
ioctl(BLKRRPART) didn't follow such 'rule', then partitions node from
'bdev' fs can disappear & re-appear anytime. I believe it is one bug
from userspace view.


Thanks,
Ming