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 Tue 21 Apr 2026 at 15:36, Xiao Ni <xni@redhat.com> wrote:

> On Tue, Apr 21, 2026 at 10:29 AM Su Yue <l@damenly.org> wrote:
>>
>> 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().
>
> The above mdadm codes use bitmap/location. In your patch, you 
> already
> pass false to md_bitmap_destroy in location_store. So removing
> common_bitmap_group in bitmap_unregister_groups can't affect the 
> above
> case.
>
Right. Got confusion between md_set_array_info() and 
update_array_info().

>
> But after reading your below comments. It's good to me that
> bitmap_unregister_groups doesn't remove common_bitmap_group.
>
Thanks.

--
Su
>>
>>
>> >> +
>> >>  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.
>
> Thanks for the explanation.
>
> Best Regards
> Xiao
>>
>> --
>> 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
>> >>
>>