* [PATCH V2 1/4] block: define parsed_partitions.flags as 'unsigned char'
2021-02-24 3:58 [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
@ 2021-02-24 3:58 ` Ming Lei
2021-02-24 4:32 ` Chaitanya Kulkarni
2021-02-24 3:58 ` [PATCH V2 2/4] block: store partition flags into block_device Ming Lei
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2021-02-24 3:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Ming Lei, Ewan Milne
So far, only 3 partition flags are used, it is enough to hold it in
'unsigned char'.
Cc: Ewan Milne <emilne@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2 2/4] block: store partition flags into block_device
2021-02-24 3:58 [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
2021-02-24 3:58 ` [PATCH V2 1/4] block: define parsed_partitions.flags as 'unsigned char' Ming Lei
@ 2021-02-24 3:58 ` Ming Lei
2021-02-24 3:58 ` [PATCH V2 3/4] block: re-organize blk_add_partitions Ming Lei
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2021-02-24 3:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Ming Lei, Ewan Milne
Prepare for checking if partition is changed.
Cc: Ewan Milne <emilne@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2 3/4] block: re-organize blk_add_partitions
2021-02-24 3:58 [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
2021-02-24 3:58 ` [PATCH V2 1/4] block: define parsed_partitions.flags as 'unsigned char' Ming Lei
2021-02-24 3:58 ` [PATCH V2 2/4] block: store partition flags into block_device Ming Lei
@ 2021-02-24 3:58 ` Ming Lei
2021-02-24 3:58 ` [PATCH V2 4/4] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
2021-02-24 8:18 ` [PATCH V2 0/3] " Christoph Hellwig
4 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2021-02-24 3:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Ming Lei, Ewan D . Milne
No functional change, make code more readable, just split
blk_add_partitions() into two helpers.
Cc: Ewan D. Milne <emilne@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2 4/4] block: avoid to drop & re-add partitions if partitions aren't changed
2021-02-24 3:58 [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
` (2 preceding siblings ...)
2021-02-24 3:58 ` [PATCH V2 3/4] block: re-organize blk_add_partitions Ming Lei
@ 2021-02-24 3:58 ` Ming Lei
2021-02-24 8:18 ` [PATCH V2 0/3] " Christoph Hellwig
4 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2021-02-24 3:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Ming Lei, Ewan D . Milne
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 <emilne@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed
2021-02-24 3:58 [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
` (3 preceding siblings ...)
2021-02-24 3:58 ` [PATCH V2 4/4] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
@ 2021-02-24 8:18 ` Christoph Hellwig
2021-02-24 11:23 ` Ming Lei
4 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2021-02-24 8:18 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Ewan D . Milne
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed
2021-02-24 8:18 ` [PATCH V2 0/3] " Christoph Hellwig
@ 2021-02-24 11:23 ` Ming Lei
2021-02-24 16:14 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2021-02-24 11:23 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, Ewan D . Milne, linux-kernel
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:
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 0/3] block: avoid to drop & re-add partitions if partitions aren't changed
2021-02-24 11:23 ` Ming Lei
@ 2021-02-24 16:14 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2021-02-24 16:14 UTC (permalink / raw)
To: Ming Lei
Cc: Christoph Hellwig, Jens Axboe, linux-block, Ewan D . Milne, linux-kernel
On Wed, Feb 24, 2021 at 07:23:54PM +0800, Ming Lei wrote:
> > > 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.
^ permalink raw reply [flat|nested] 9+ messages in thread