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

STALE2060d

Revision v2 of 2 in this series.

9 messages, 3 authors, 2021-02-24 · open the first message on its own page

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

From: Ming Lei <hidden>
Date: 2021-02-24 04:00:13

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 calling into ioctl(BLKRRPART).

V2:
	- don't save partitions state, and just check if current partition
	state is changed against partition devices	

Ming Lei (4):
  block: define parsed_partitions.flags as 'unsigned char'
  block: store partition flags into block_device
  block: re-organize blk_add_partitions
  block: avoid to drop & re-add partitions if partitions aren't changed

 block/partitions/check.h  |   2 +-
 block/partitions/core.c   | 126 ++++++++++++++++++++++++++++++++++----
 fs/block_dev.c            |  30 ++++-----
 include/linux/blk_types.h |   1 +
 include/linux/genhd.h     |   1 +
 5 files changed, 132 insertions(+), 28 deletions(-)

Cc: Ewan D. Milne <redacted>
-- 
2.29.2

[PATCH V2 1/4] block: define parsed_partitions.flags as 'unsigned char'

From: Ming Lei <hidden>
Date: 2021-02-24 04:00:19

So far, only 3 partition flags are used, it is enough to hold it in
'unsigned char'.

Cc: Ewan Milne <redacted>
Signed-off-by: Ming Lei <redacted>
---
 block/partitions/check.h | 2 +-
 block/partitions/core.c  | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/partitions/check.h b/block/partitions/check.h
index c577e9ee67f0..8f0ceed06c7b 100644
--- a/block/partitions/check.h
+++ b/block/partitions/check.h

@@ -14,7 +14,7 @@ struct parsed_partitions {
 	struct {
 		sector_t from;
 		sector_t size;
-		int flags;
+		unsigned char flags;
 		bool has_info;
 		struct partition_meta_info info;
 	} *parts;

diff --git a/block/partitions/core.c b/block/partitions/core.c
index f3d9ff2cafb6..430ff7863556 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c

@@ -314,7 +314,8 @@ static DEVICE_ATTR(whole_disk, 0444, whole_disk_show, NULL);
  * after all disk users are gone.
  */
 static struct block_device *add_partition(struct gendisk *disk, int partno,
-				sector_t start, sector_t len, int flags,
+				sector_t start, sector_t len,
+				unsigned char flags,
 				struct partition_meta_info *info)
 {
 	dev_t devt = MKDEV(0, 0);

-- 
2.29.2

[PATCH V2 2/4] block: store partition flags into block_device

From: Ming Lei <hidden>
Date: 2021-02-24 04:00:31

Prepare for checking if partition is changed.

Cc: Ewan Milne <redacted>
Signed-off-by: Ming Lei <redacted>
---
 block/partitions/core.c   | 1 +
 include/linux/blk_types.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index 430ff7863556..07981f663f66 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c

@@ -352,6 +352,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
 
 	bdev->bd_start_sect = start;
 	bdev_set_nr_sectors(bdev, len);
+	bdev->bd_part_flags = flags;
 
 	if (info) {
 		err = -ENOMEM;

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index db026b6ec15a..4d43b80cb9e9 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h

@@ -40,6 +40,7 @@ struct block_device {
 #endif
 	struct kobject		*bd_holder_dir;
 	u8			bd_partno;
+	u8			bd_part_flags;
 	/* number of times partitions within this device have been opened. */
 	unsigned		bd_part_count;

-- 
2.29.2

[PATCH V2 3/4] block: re-organize blk_add_partitions

From: Ming Lei <hidden>
Date: 2021-02-24 04:00:32

No functional change, make code more readable, just split
blk_add_partitions() into two helpers.

Cc: Ewan D. Milne <redacted>
Signed-off-by: Ming Lei <redacted>
---
 block/partitions/core.c | 50 +++++++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 9 deletions(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index 07981f663f66..288ca362ccbd 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c

@@ -603,17 +603,19 @@ 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;
+	struct parsed_partitions *state = NULL;
+	int ret = -EAGAIN;
 
 	if (!disk_part_scan_enabled(disk))
-		return 0;
+		goto out;
 
 	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

@@ -651,15 +653,45 @@ 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;
+}
+
+static int __blk_add_partitions(struct gendisk *disk,
+		struct block_device *bdev, struct parsed_partitions *state)
+{
+	int p;
+
 	/* 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++)
+	for (p = 1; p < state->limit; p++) {
 		if (!blk_add_partition(disk, bdev, state, p))
-			goto out_free_state;
+			return -EAGAIN;
+	}
+
+	return 0;
+}
+
+int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
+{
+	struct parsed_partitions *state;
+	int ret;
+
+	ret = __blk_check_partitions(disk, bdev, &state);
+	if (ret != 0)
+		return ret;
+	if (!state)
+		return 0;
+
+	ret = __blk_add_partitions(disk, bdev, state);
 
-	ret = 0;
-out_free_state:
 	free_partitions(state);
 	return ret;
 }

-- 
2.29.2

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

From: Ming Lei <hidden>
Date: 2021-02-24 04:00:34

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

ioctl(BLKRRPART) is called by systemd-udevd and some disk utilities not
unusually. 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, and only
drop & re-add them when partitions state is really changed.

Cc: Ewan D. Milne <redacted>
Signed-off-by: Ming Lei <redacted>
---
 block/partitions/core.c | 76 ++++++++++++++++++++++++++++++++++++++---
 fs/block_dev.c          | 30 ++++++++--------
 include/linux/genhd.h   |  1 +
 3 files changed, 88 insertions(+), 19 deletions(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index 288ca362ccbd..901a00fed3c9 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c

@@ -123,7 +123,7 @@ static void free_partitions(struct parsed_partitions *state)
 }
 
 static struct parsed_partitions *check_partition(struct gendisk *hd,
-		struct block_device *bdev)
+		struct block_device *bdev, bool show_part)
 {
 	struct parsed_partitions *state;
 	int i, res, err;

@@ -159,7 +159,8 @@ static struct parsed_partitions *check_partition(struct gendisk *hd,
 
 	}
 	if (res > 0) {
-		printk(KERN_INFO "%s", state->pp_buf);
+		if (show_part)
+			printk(KERN_INFO "%s", state->pp_buf);
 
 		free_page((unsigned long)state->pp_buf);
 		return state;

@@ -604,7 +605,8 @@ static bool blk_add_partition(struct gendisk *disk, struct block_device *bdev,
 }
 
 static int __blk_check_partitions(struct gendisk *disk,
-		struct block_device *bdev, struct parsed_partitions **s)
+		struct block_device *bdev, struct parsed_partitions **s,
+		bool show_part)
 {
 	struct parsed_partitions *state = NULL;
 	int ret = -EAGAIN;

@@ -612,7 +614,7 @@ static int __blk_check_partitions(struct gendisk *disk,
 	if (!disk_part_scan_enabled(disk))
 		goto out;
 
-	state = check_partition(disk, bdev);
+	state = check_partition(disk, bdev, show_part);
 	if (!state)
 		goto out;

@@ -679,12 +681,76 @@ static int __blk_add_partitions(struct gendisk *disk,
 	return 0;
 }
 
+static bool partition_changed(struct block_device *part,
+		struct parsed_partitions *state)
+{
+	int p = part->bd_partno;
+
+	if (part->bd_start_sect != state->parts[p].from)
+		return true;
+
+	if (bdev_nr_sectors(part) != state->parts[p].size)
+		return true;
+
+	if (part->bd_part_flags != state->parts[p].flags)
+		return true;
+
+	if (bcmp(part->bd_meta_info, &state->parts[p].info,
+				sizeof(struct partition_meta_info)))
+		return true;
+
+	return false;
+}
+
+static int partition_count(struct parsed_partitions *state)
+{
+	int p;
+	int cnt = 0;
+
+	for (p = 1; p < state->limit; p++) {
+		if (state->parts[p].size || state->parts[p].from)
+			cnt++;
+	}
+	return cnt;
+}
+
+bool blk_partition_changed(struct gendisk *disk, struct block_device *bdev)
+{
+	struct disk_part_iter piter;
+	struct block_device *part;
+	struct parsed_partitions *state = NULL;
+	bool changed = true;
+	int nr_parts = 0;
+
+	if (!get_capacity(disk))
+		goto out;
+
+	if (__blk_check_partitions(disk, bdev, &state, false) != 0 || !state)
+		goto out;
+
+	changed = false;
+	disk_part_iter_init(&piter, disk, 0);
+	while ((part = disk_part_iter_next(&piter))) {
+		if (partition_changed(part, state))
+			changed = true;
+		nr_parts++;
+	}
+	disk_part_iter_exit(&piter);
+
+	if (nr_parts != partition_count(state))
+		changed = true;
+ out:
+	if (state)
+		free_partitions(state);
+	return changed;
+}
+
 int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
 {
 	struct parsed_partitions *state;
 	int ret;
 
-	ret = __blk_check_partitions(disk, bdev, &state);
+	ret = __blk_check_partitions(disk, bdev, &state, true);
 	if (ret != 0)
 		return ret;
 	if (!state)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index ec26179c8062..b279649ba532 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c

@@ -1234,10 +1234,6 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 	clear_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
 
 rescan:
-	ret = blk_drop_partitions(bdev);
-	if (ret)
-		return ret;
-
 	/*
 	 * Historically we only set the capacity to zero for devices that
 	 * support partitions (independ of actually having partitions created).

@@ -1255,16 +1251,22 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 			disk->fops->revalidate_disk(disk);
 	}
 
-	if (get_capacity(disk)) {
-		ret = blk_add_partitions(disk, bdev);
-		if (ret == -EAGAIN)
-			goto rescan;
-	} else if (invalidate) {
-		/*
-		 * Tell userspace that the media / partition table may have
-		 * changed.
-		 */
-		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
+	if (blk_partition_changed(disk, bdev)) {
+		ret = blk_drop_partitions(bdev);
+		if (ret)
+			return ret;
+
+		if (get_capacity(disk)) {
+			ret = blk_add_partitions(disk, bdev);
+			if (ret == -EAGAIN)
+				goto rescan;
+		} else if (invalidate) {
+			/*
+			 * Tell userspace that the media / partition table may have
+			 * changed.
+			 */
+			kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
+		}
 	}
 
 	return ret;

diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index f364619092cc..ae9ebfb9a8e1 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h

@@ -274,6 +274,7 @@ 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_partition_changed(struct gendisk *disk, struct block_device *bdev);
 
 extern struct gendisk *__alloc_disk_node(int minors, int node_id);
 extern void put_disk(struct gendisk *disk);

-- 
2.29.2

Re: [PATCH V2 1/4] block: define parsed_partitions.flags as 'unsigned char'

From: Chaitanya Kulkarni <hidden>
Date: 2021-02-24 04:33:36

On 2/23/21 20:01, Ming Lei wrote:

So far, only 3 partition flags are used, it is enough to hold it in
'unsigned char'.

Cc: Ewan Milne <redacted>
Signed-off-by: Ming Lei <redacted>

Looks good.

Reviewed-by: Chaitanya Kulkarni <redacted>

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

From: Christoph Hellwig <hch@lst.de>
Date: 2021-02-24 08:19:21

On Wed, Feb 24, 2021 at 11:58:26AM +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 calling into ioctl(BLKRRPART).

Which is the f***king point of BLKRRPART and the behavior it had
since day 1.  Please fix the application(s) that all it all the time
instead of bloating the kernel, as said before.

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

From: Ming Lei <hidden>
Date: 2021-02-24 11:26:21

On Wed, Feb 24, 2021 at 09:18:25AM +0100, Christoph Hellwig wrote:

On Wed, Feb 24, 2021 at 11:58:26AM +0800, Ming Lei wrote:

quoted

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 calling into ioctl(BLKRRPART).

Which is the f***king point of BLKRRPART and the behavior it had
since day 1.  Please fix the application(s) that all it all the time
instead of bloating the kernel, as said before.

ioctl(BLKRRPART) can be called without changing partition table in
fdisk, cfdisk, sfdisk, systemd and blockdev at least, and it isn't only
on one single application. Even for blockdev, not sure if it can be fixed
because '--rereadpt' is simply one subcommand.

-- 
Ming

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

From: Christoph Hellwig <hch@lst.de>
Date: 2021-02-24 16:18:27

On Wed, Feb 24, 2021 at 07:23:54PM +0800, Ming Lei wrote:

quoted

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 calling into ioctl(BLKRRPART).

Which is the f***king point of BLKRRPART and the behavior it had
since day 1.  Please fix the application(s) that all it all the time
instead of bloating the kernel, as said before.

ioctl(BLKRRPART) can be called without changing partition table in
fdisk, cfdisk, sfdisk, systemd and blockdev at least, and it isn't only
on one single application. Even for blockdev, not sure if it can be fixed
because '--rereadpt' is simply one subcommand.

I can also do all kinds of other bad things when I really want to.  So
if a privileged user uses *fdisk, or explicitly calls
blockdev --rereadpt, we can expect this behavior, and all of them above
should not be frequent.

It seems like the main culprit is systemd, so please look into what is
going wrong there.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help