* [PATCH 1/2] block: move partitions check code into single helper
2021-02-05 2:17 [PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
@ 2021-02-05 2:17 ` Ming Lei
2021-02-05 2:17 ` [PATCH 2/2] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
2021-02-15 4:03 ` [PATCH 0/2] " Ming Lei
2 siblings, 0 replies; 10+ messages in thread
From: Ming Lei @ 2021-02-05 2:17 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, linux-block, linux-kernel, Ming Lei, Ewan D . Milne
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/2] block: avoid to drop & re-add partitions if partitions aren't changed
2021-02-05 2:17 [PATCH 0/2] block: avoid to drop & re-add partitions if partitions aren't changed Ming Lei
2021-02-05 2:17 ` [PATCH 1/2] block: move partitions check code into single helper Ming Lei
@ 2021-02-05 2:17 ` Ming Lei
2021-02-05 7:14 ` Christoph Hellwig
2021-02-15 4:03 ` [PATCH 0/2] " Ming Lei
2 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2021-02-05 2:17 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, linux-block, linux-kernel, 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) 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
^ permalink raw reply related [flat|nested] 10+ messages in thread