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

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).

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