[PATCH v2 3/5] md/md-bitmap: add dummy bitmap ops for none to fix wrong bitmap offset
Su Yue <[email protected]>
| Newsgroups | gmane.linux.raid |
|---|---|
| Message-ID | <[email protected]> |
Before commit fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
if CONFIG_MD_BITMAP is enabled, both bitmap none, internal and clustered have
the sysfs file bitmap/location.
After the commit, if bitmap is none, bitmap/location doesn't exist anymore.
It breaks 'grow' behavior of a md array of madam with MD_FEATURE_BITMAP_OFFSET.
Take level=mirror and metadata=1.2 as an example:
$ mdadm --create /dev/md0 -f --bitmap=none --raid-devices=2 --level=mirror \
--metadata=1.2 /dev/vdd /dev/vde
$ mdadm --grow /dev/md0 --bitmap=internal
$ cat /sys/block/md0/md/bitmap/location
Before:+8
After: +2
While growing bitmap from none to internal, clustered and llbitmap,
mdadm/Grow.c:Grow_addbitmap() tries to detect bitmap/location first.
1)If bitmap/location exists, it sets bitmap/location after getinfo_super().
2)If bitmap/location doesn't exist, mdadm just calls md_set_array_info() then
mddev->bitmap_info.default_offset will be used.
Situation can be worse if growing none to clustered, bitmap offset of the node
calling `madm --grow` will be changed but the other node are reading bitmap sb from
the old location.
Here introducing a dummy bitmap_operations for ID_BITMAP_NONE to restore sysfs
file bitmap/location for ID_BITMAP_NONE and ID_BITMAP.
location_store() now calls md_bitmap_(create|destroy) with false sysfs parameter then
(add,remove)_internal_bitmap() will be called to manage sysfs entries.
Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
Suggested-by: Yu Kuai <[email protected]>
Signed-off-by: Su Yue <[email protected]>
---
drivers/md/md-bitmap.c | 116 ++++++++++++++++++++++++++++++++++++---
drivers/md/md-bitmap.h | 3 +
drivers/md/md-llbitmap.c | 12 ++++
drivers/md/md.c | 16 +++---
4 files changed, 130 insertions(+), 17 deletions(-)
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index ac06c9647bf0..a8a176428c61 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -240,6 +240,11 @@ static bool bitmap_enabled(void *data, bool flush)
bitmap->storage.filemap != NULL;
}
+static bool dummy_bitmap_enabled(void *data, bool flush)
+{
+ return false;
+}
+
/*
* check a page and, if necessary, allocate it (or hijack it if the alloc fails)
*
@@ -2201,6 +2206,11 @@ static int bitmap_create(struct mddev *mddev)
return 0;
}
+static int dummy_bitmap_create(struct mddev *mddev)
+{
+ return 0;
+}
+
static int bitmap_load(struct mddev *mddev)
{
int err = 0;
@@ -2594,6 +2604,9 @@ location_show(struct mddev *mddev, char *page)
return len;
}
+static int create_internal_group(struct mddev *mddev);
+static void remove_internal_group(struct mddev *mddev);
+
static ssize_t
location_store(struct mddev *mddev, const char *buf, size_t len)
{
@@ -2618,7 +2631,8 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
goto out;
}
- md_bitmap_destroy(mddev, true);
+ md_bitmap_destroy(mddev, false);
+ remove_internal_group(mddev);
mddev->bitmap_info.offset = 0;
if (mddev->bitmap_info.file) {
struct file *f = mddev->bitmap_info.file;
@@ -2659,14 +2673,22 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
*/
mddev->bitmap_id = ID_BITMAP;
mddev->bitmap_info.offset = offset;
- rv = md_bitmap_create(mddev, true);
+ rv = md_bitmap_create(mddev, false);
if (rv)
goto out;
+ rv = create_internal_group(mddev);
+ if (rv) {
+ mddev->bitmap_info.offset = 0;
+ bitmap_destroy(mddev);
+ goto out;
+ }
+
rv = bitmap_load(mddev);
if (rv) {
+ remove_internal_group(mddev);
mddev->bitmap_info.offset = 0;
- md_bitmap_destroy(mddev, true);
+ md_bitmap_destroy(mddev, false);
goto out;
}
}
@@ -2960,8 +2982,7 @@ static struct md_sysfs_entry max_backlog_used =
__ATTR(max_backlog_used, S_IRUGO | S_IWUSR,
behind_writes_used_show, behind_writes_used_reset);
-static struct attribute *md_bitmap_attrs[] = {
- &bitmap_location.attr,
+static struct attribute *internal_bitmap_attrs[] = {
&bitmap_space.attr,
&bitmap_timeout.attr,
&bitmap_backlog.attr,
@@ -2972,11 +2993,57 @@ static struct attribute *md_bitmap_attrs[] = {
NULL
};
-static struct attribute_group md_bitmap_group = {
+static struct attribute_group internal_bitmap_group = {
.name = "bitmap",
- .attrs = md_bitmap_attrs,
+ .attrs = internal_bitmap_attrs,
};
+/* Only necessary attrs for compatibility */
+static struct attribute *common_bitmap_attrs[] = {
+ &bitmap_location.attr,
+ NULL
+};
+
+static const struct attribute_group common_bitmap_group = {
+ .name = "bitmap",
+ .attrs = common_bitmap_attrs,
+};
+
+static int create_internal_group(struct mddev *mddev)
+{
+ /*
+ * md_bitmap_group and md_bitmap_common_group are using same name
+ * 'bitmap'.
+ */
+ return sysfs_merge_group(&mddev->kobj, &internal_bitmap_group);
+}
+
+static void remove_internal_group(struct mddev *mddev)
+{
+ sysfs_unmerge_group(&mddev->kobj, &internal_bitmap_group);
+}
+
+static int bitmap_register_groups(struct mddev *mddev)
+{
+ int ret;
+
+ ret = sysfs_create_group(&mddev->kobj, &common_bitmap_group);
+
+ if (ret)
+ return ret;
+
+ ret = sysfs_merge_group(&mddev->kobj, &internal_bitmap_group);
+ if (ret)
+ sysfs_remove_group(&mddev->kobj, &common_bitmap_group);
+
+ return ret;
+}
+
+static void bitmap_unregister_groups(struct mddev *mddev)
+{
+ sysfs_unmerge_group(&mddev->kobj, &internal_bitmap_group);
+}
+
static struct bitmap_operations bitmap_ops = {
.head = {
.type = MD_BITMAP,
@@ -3018,21 +3085,52 @@ static struct bitmap_operations bitmap_ops = {
.set_pages = bitmap_set_pages,
.free = md_bitmap_free,
- .group = &md_bitmap_group,
+ .register_groups = bitmap_register_groups,
+ .unregister_groups = bitmap_unregister_groups,
+};
+
+static int none_bitmap_register_groups(struct mddev *mddev)
+{
+ return sysfs_create_group(&mddev->kobj, &common_bitmap_group);
+}
+
+static struct bitmap_operations none_bitmap_ops = {
+ .head = {
+ .type = MD_BITMAP,
+ .id = ID_BITMAP_NONE,
+ .name = "none",
+ },
+
+ .enabled = dummy_bitmap_enabled,
+ .create = dummy_bitmap_create,
+ .destroy = bitmap_destroy,
+ .load = bitmap_load,
+ .get_stats = bitmap_get_stats,
+ .free = md_bitmap_free,
+
+ .register_groups = none_bitmap_register_groups,
+ .unregister_groups = NULL,
};
int md_bitmap_init(void)
{
+ int ret;
+
md_bitmap_wq = alloc_workqueue("md_bitmap", WQ_MEM_RECLAIM | WQ_UNBOUND,
0);
if (!md_bitmap_wq)
return -ENOMEM;
- return register_md_submodule(&bitmap_ops.head);
+ ret = register_md_submodule(&bitmap_ops.head);
+ if (ret)
+ return ret;
+
+ return register_md_submodule(&none_bitmap_ops.head);
}
void md_bitmap_exit(void)
{
destroy_workqueue(md_bitmap_wq);
unregister_md_submodule(&bitmap_ops.head);
+ unregister_md_submodule(&none_bitmap_ops.head);
}
diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
index b42a28fa83a0..10bc6854798c 100644
--- a/drivers/md/md-bitmap.h
+++ b/drivers/md/md-bitmap.h
@@ -125,6 +125,9 @@ struct bitmap_operations {
void (*set_pages)(void *data, unsigned long pages);
void (*free)(void *data);
+ int (*register_groups)(struct mddev *mddev);
+ void (*unregister_groups)(struct mddev *mddev);
+
struct attribute_group *group;
};
diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
index bf398d7476b3..9b3ea4f1d268 100644
--- a/drivers/md/md-llbitmap.c
+++ b/drivers/md/md-llbitmap.c
@@ -1561,6 +1561,16 @@ static struct attribute_group md_llbitmap_group = {
.attrs = md_llbitmap_attrs,
};
+static int llbitmap_register_groups(struct mddev *mddev)
+{
+ return sysfs_create_group(&mddev->kobj, &md_llbitmap_group);
+}
+
+static void llbitmap_unregister_groups(struct mddev *mddev)
+{
+ sysfs_remove_group(&mddev->kobj, &md_llbitmap_group);
+}
+
static struct bitmap_operations llbitmap_ops = {
.head = {
.type = MD_BITMAP,
@@ -1597,6 +1607,8 @@ static struct bitmap_operations llbitmap_ops = {
.dirty_bits = llbitmap_dirty_bits,
.write_all = llbitmap_write_all,
+ .register_groups = llbitmap_register_groups,
+ .unregister_groups = llbitmap_unregister_groups,
.group = &md_llbitmap_group,
};
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d3c8f77b4fe3..55a95b227b83 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -683,8 +683,7 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
struct bitmap_operations *old = mddev->bitmap_ops;
struct md_submodule_head *head;
- if (mddev->bitmap_id == ID_BITMAP_NONE ||
- (old && old->head.id == mddev->bitmap_id))
+ if (old && old->head.id == mddev->bitmap_id)
return true;
xa_lock(&md_submodule);
@@ -703,8 +702,8 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
mddev->bitmap_ops = (void *)head;
xa_unlock(&md_submodule);
- if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
- if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
+ if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->register_groups) {
+ if (mddev->bitmap_ops->register_groups(mddev))
pr_warn("md: cannot register extra bitmap attributes for %s\n",
mdname(mddev));
else
@@ -724,8 +723,8 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
static void mddev_clear_bitmap_ops(struct mddev *mddev, bool remove_sysfs)
{
if (remove_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops &&
- mddev->bitmap_ops->group)
- sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
+ mddev->bitmap_ops->unregister_groups)
+ mddev->bitmap_ops->unregister_groups(mddev);
mddev->bitmap_ops = NULL;
}
@@ -6610,8 +6609,9 @@ int md_run(struct mddev *mddev)
(unsigned long long)pers->size(mddev, 0, 0) / 2);
err = -EINVAL;
}
- if (err == 0 && pers->sync_request &&
- (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
+ if (err == 0 && pers->sync_request) {
+ if (mddev->bitmap_info.offset == 0)
+ mddev->bitmap_id = ID_BITMAP_NONE;
err = md_bitmap_create(mddev, true);
if (err)
pr_warn("%s: failed to create bitmap (%d)\n",
--
2.53.0