Re: [PATCH v2 3/5] md/md-bitmap: add dummy bitmap ops for none to fix wrong bitmap offset

[Su Yue <l@damenly.org>]

On Mon 20 Apr 2026 at 15:05, Xiao Ni <xni@redhat.com> wrote:

> On Tue, Apr 7, 2026 at 6:27 PM Su Yue <glass.su@suse.com> wrote:
>>
>> 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 <yukuai@fnnas.com>
>> Signed-off-by: Su Yue <glass.su@suse.com>
>> ---
>>  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);
>> +}
>
> Hi Su
>
> bitmap_unregister_groups should also remove common_bitmap_group, 
> right?
>

As you pasted before, mdadm:Grow.c:

int Grow_addbitmap(char *devname, int fd, struct context *c, 
struct shape *s)
{
...
	if (array.state & (1 << MD_SB_BITMAP_PRESENT)) {
		if (s->btype == BitmapNone) {
			array.state &= ~(1 << MD_SB_BITMAP_PRESENT);
			if (md_set_array_info(fd, &array) != 0) {
				if (array.state & (1 << MD_SB_CLUSTERED))
					pr_err("failed to remove clustered 
					bitmap.\n");
				else
					pr_err("failed to remove internal bitmap.\n");
				return 1;
			}
			return 0;
		}
		pr_err("bitmap already present on %s\n", devname);
		return 1;
	}
...
}

In case of growing from internal to none, 
bitmap_unregister_groups() will
be called, if common_bitmap_group is removed, bitmap/location 
won't exist.
update_array_info() is a common function used by many call paths. 
Also
llbitmap is involved here. I don't want to make situation and code 
more
complicated like adding more codes in update_array_info().


>> +
>>  static struct bitmap_operations bitmap_ops = {
>
> You already changed md_bitmap_group to internal_bitmap_group. 
> It's
> better to change bitmap_ops to internal_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,
>
> How does bitmap/location can be deleted if array is created with 
> no
> bitmap? Can the `mdadm --stop` command get stuck?
>
No. It wont get stuck.

I guess here your concern is the timing of removing 
common_bitmap_group.
The life cycle is same as before fb8cc3b0d9db. At the time 
llbitmap is
not introduced, because there is no need to switch bitmap ops, so 
all
attrs of bitmap are removed in kobject_put() in 
mddev_delayed_delete()
queued by mddev_put(). This is now where common_bitmap_group is 
being removed.

--
Su
> Best Regards
> Xiao
>
>>  };
>>
>>  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
>>