Re: [PATCH 1/3] md: restore bitmap/location to fix wrong bitmap offset while growing

Hi,

在 2026/3/4 11:14, Su Yue 写道:
> On Wed 04 Mar 2026 at 10:30, "Yu Kuai" <[email protected]> wrote:
>
>> Hi,
>>
>> 在 2026/3/3 11:37, Su Yue 写道:
>>> 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.
>>
>> Now that we have a new sysfs attribute bitmap_type, can we fix this by:
>> - in the kernel, allow writing to this file in this case;
>> - in mdadm and the grow case above, write to this file first, and change
>> bitmap_type from none to bitmap(For llbitmap, there is still more 
>> work to do).
>>
> Yes. It's indeed feasible. But how about old versions mdadm? We can't 
> require
> users' madadm + kernel combinations for old feature. Kernel part 
> should keep
> compatibility with userspace. sysfs changes and broken haviros are not 
> ideal
> especially userspace depends on it unless there's a strong reason.
> That's why linux/Documentation/ABI exists.

Okay, I can accept keep this old behavior.

However, instead of introducing a new common_group with the same name "bitmap",
I'll prefer to introducing a separate bitmap_ops for none bitmap as well, and
you can define the attrs that are necessary.

For llbitmap, I think it's fine, we don't need this old sysfs attr anyway. I'll
support to convert from none/bitmap to llbitmap by writing the new bitmap_type
file.
  

>
> -- 
> Su
>
>>>
>>> Here restore sysfs file bitmap/location for ID_BITMAP_NONE and 
>>> ID_BITMAP.
>>> And it d adds the entry for llbitmap too.
>>>
>>> New attribute_group md_bitmap_common_group is introduced and created in
>>> md_alloc() as before commit fb8cc3b0d9db.
>>> Add New operations register_group and unregister_group to struct 
>>> bitmap_operations.
>>>
>>> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops 
>>> until creating bitmap")
>>> Signed-off-by: Su Yue <[email protected]>
>>> ---
>>>   drivers/md/md-bitmap.c   | 32 +++++++++++++++++++++++++++++++-
>>>   drivers/md/md-bitmap.h   |  5 +++++
>>>   drivers/md/md-llbitmap.c | 13 +++++++++++++
>>>   drivers/md/md.c          | 16 ++++++++++++----
>>>   4 files changed, 61 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>>> index 83378c033c72..8ff1dc94ed78 100644
>>> --- a/drivers/md/md-bitmap.c
>>> +++ b/drivers/md/md-bitmap.c
>>> @@ -2956,7 +2956,6 @@ __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,
>>>       &bitmap_space.attr,
>>>       &bitmap_timeout.attr,
>>>       &bitmap_backlog.attr,
>>> @@ -2967,11 +2966,40 @@ static struct attribute *md_bitmap_attrs[] = {
>>>       NULL
>>>   };
>>>
>>> +static struct attribute *md_bitmap_common_attrs[] = {
>>> +    &bitmap_location.attr,
>>> +    NULL
>>> +};
>>> +
>>>   static struct attribute_group md_bitmap_group = {
>>>       .name = "bitmap",
>>>       .attrs = md_bitmap_attrs,
>>>   };
>>>
>>> +static struct attribute_group md_bitmap_common_group = {
>>> +    .name = "bitmap",
>>> +    .attrs = md_bitmap_common_attrs,
>>> +};
>>> +
>>> +int md_sysfs_create_common_group(struct mddev *mddev)
>>> +{
>>> +    return sysfs_create_group(&mddev->kobj, &md_bitmap_common_group);
>>> +}
>>> +
>>> +static int bitmap_register_group(struct mddev *mddev)
>>> +{
>>> +    /*
>>> +     * md_bitmap_group and md_bitmap_common_group are using same name
>>> +     * 'bitmap'.
>>> +     */
>>> +    return sysfs_merge_group(&mddev->kobj, &md_bitmap_group);
>>> +}
>>> +
>>> +static void bitmap_unregister_group(struct mddev *mddev)
>>> +{
>>> +    sysfs_unmerge_group(&mddev->kobj, &md_bitmap_group);
>>> +}
>>> +
>>>   static struct bitmap_operations bitmap_ops = {
>>>       .head = {
>>>           .type    = MD_BITMAP,
>>> @@ -3013,6 +3041,8 @@ static struct bitmap_operations bitmap_ops = {
>>>       .set_pages        = bitmap_set_pages,
>>>       .free            = md_bitmap_free,
>>>
>>> +    .register_group        = bitmap_register_group,
>>> +    .unregister_group    = bitmap_unregister_group,
>>>       .group            = &md_bitmap_group,
>>>   };
>>>
>>> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
>>> index b42a28fa83a0..371791e9011d 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_group)(struct mddev *mddev);
>>> +    void (*unregister_group)(struct mddev *mddev);
>>> +
>>>       struct attribute_group *group;
>>>   };
>>>
>>> @@ -169,6 +172,8 @@ static inline void md_bitmap_end_sync(struct 
>>> mddev *mddev, sector_t offset,
>>>       mddev->bitmap_ops->end_sync(mddev, offset, blocks);
>>>   }
>>>
>>> +int md_sysfs_create_common_group(struct mddev *mddev);
>>> +
>>>   #ifdef CONFIG_MD_BITMAP
>>>   int md_bitmap_init(void);
>>>   void md_bitmap_exit(void);
>>> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
>>> index bf398d7476b3..24ff5f7f8751 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_group(struct mddev *mddev)
>>> +{
>>> +    return sysfs_create_group(&mddev->kobj, &md_llbitmap_group);
>>> +}
>>> +
>>> +static void llbitmap_unregister_group(struct mddev *mddev)
>>> +{
>>> +    sysfs_remove_group(&mddev->kobj, &md_llbitmap_group);
>>> +}
>>> +
>>>   static struct bitmap_operations llbitmap_ops = {
>>>       .head = {
>>>           .type    = MD_BITMAP,
>>> @@ -1597,6 +1607,9 @@ static struct bitmap_operations llbitmap_ops = {
>>>       .dirty_bits        = llbitmap_dirty_bits,
>>>       .write_all        = llbitmap_write_all,
>>>
>>> +    .register_group        = llbitmap_register_group,
>>> +    .unregister_group    = llbitmap_unregister_group,
>>> +
>>>       .group            = &md_llbitmap_group,
>>>   };
>>>
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index 3ce6f9e9d38e..ab969e950ea8 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -703,8 +703,8 @@ static bool mddev_set_bitmap_ops(struct mddev 
>>> *mddev)
>>>       mddev->bitmap_ops = (void *)head;
>>>       xa_unlock(&md_submodule);
>>>
>>> -    if (!mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
>>> -        if (sysfs_create_group(&mddev->kobj, 
>>> mddev->bitmap_ops->group))
>>> +    if (!mddev_is_dm(mddev) && mddev->bitmap_ops->register_group) {
>>> +        if (mddev->bitmap_ops->register_group(mddev))
>>>               pr_warn("md: cannot register extra bitmap attributes 
>>> for %s\n",
>>>                   mdname(mddev));
>>>           else
>>> @@ -724,8 +724,8 @@ static bool mddev_set_bitmap_ops(struct mddev 
>>> *mddev)
>>>   static void mddev_clear_bitmap_ops(struct mddev *mddev)
>>>   {
>>>       if (!mddev_is_dm(mddev) && mddev->bitmap_ops &&
>>> -        mddev->bitmap_ops->group)
>>> -        sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
>>> +        mddev->bitmap_ops->unregister_group)
>>> +        mddev->bitmap_ops->unregister_group(mddev);
>>>
>>>       mddev->bitmap_ops = NULL;
>>>   }
>>> @@ -6369,6 +6369,14 @@ struct mddev *md_alloc(dev_t dev, char *name)
>>>           return ERR_PTR(error);
>>>       }
>>>
>>> +    /*
>>> +     * md_sysfs_remove_common_group is not needed because 
>>> mddev_delayed_delete
>>> +     * calls kobject_put(&mddev->kobj) if mddev is to be deleted.
>>> +     */
>>> +    if (md_sysfs_create_common_group(mddev))
>>> +        pr_warn("md: cannot register common bitmap attributes for 
>>> %s\n",
>>> +            mdname(mddev));
>>> +
>>>       kobject_uevent(&mddev->kobj, KOBJ_ADD);
>>>       mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd,   
>>> "array_state");
>>>       mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd,   
>>> "level");

-- 
Thansk,
Kuai