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

[Su Yue <l@damenly.org>]

On Wed 04 Mar 2026 at 10:30, "Yu Kuai" <yukuai@fnnas.com> 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.

--
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 <glass.su@suse.com>
>> ---
>>   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");