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

On Tue, Apr 21, 2026 at 10:29 AM Su Yue <[email protected]> wrote:
>
> On Mon 20 Apr 2026 at 15:05, Xiao Ni <[email protected]> wrote:
>
> > On Tue, Apr 7, 2026 at 6:27 PM Su Yue <[email protected]> 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 <[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);
> >> +}
> >
> > 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.

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

>
>
> >> +
> >>  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
> >>
>