[PATCH v3 06/11] md/md-bitmap: delay registration of bitmap_ops until creating bitmap

Yu Kuai posted 11 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v3 06/11] md/md-bitmap: delay registration of bitmap_ops until creating bitmap
Posted by Yu Kuai 2 months, 2 weeks ago
From: Yu Kuai <yukuai3@huawei.com>

Currently bitmap_ops is registered while allocating mddev, this is fine
when there is only one bitmap_ops.

Delay setting bitmap_ops until creating bitmap, so that user can choose
which bitmap to use before running the array.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 Documentation/admin-guide/md.rst |  3 ++
 drivers/md/md.c                  | 82 +++++++++++++++++++++-----------
 2 files changed, 56 insertions(+), 29 deletions(-)

diff --git a/Documentation/admin-guide/md.rst b/Documentation/admin-guide/md.rst
index 356d2a344f08..03a9f5025f99 100644
--- a/Documentation/admin-guide/md.rst
+++ b/Documentation/admin-guide/md.rst
@@ -388,6 +388,9 @@ All md devices contain:
      bitmap
          The default internal bitmap
 
+If bitmap_type is not none, then additional bitmap attributes will be
+created after md device KOBJ_CHANGE event.
+
 If bitmap_type is bitmap, then the md device will also contain:
 
   bitmap/location
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d8b0dfdb4bfc..639b0143cbb1 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -674,9 +674,11 @@ static void no_op(struct percpu_ref *r) {}
 
 static bool mddev_set_bitmap_ops(struct mddev *mddev)
 {
+	struct bitmap_operations *old = mddev->bitmap_ops;
 	struct md_submodule_head *head;
 
-	if (mddev->bitmap_id == ID_BITMAP_NONE)
+	if (mddev->bitmap_id == ID_BITMAP_NONE ||
+	    (old && old->head.id == mddev->bitmap_id))
 		return true;
 
 	xa_lock(&md_submodule);
@@ -694,6 +696,18 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
 
 	mddev->bitmap_ops = (void *)head;
 	xa_unlock(&md_submodule);
+
+	if (mddev->bitmap_ops->group) {
+		if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
+			pr_warn("md: cannot register extra bitmap attributes for %s\n",
+				mdname(mddev));
+		else
+			/*
+			 * Inform user with KOBJ_CHANGE about new bitmap
+			 * attributes.
+			 */
+			kobject_uevent(&mddev->kobj, KOBJ_CHANGE);
+	}
 	return true;
 
 err:
@@ -703,28 +717,25 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
 
 static void mddev_clear_bitmap_ops(struct mddev *mddev)
 {
+	if (mddev->bitmap_ops && mddev->bitmap_ops->group)
+		sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
+
 	mddev->bitmap_ops = NULL;
 }
 
 int mddev_init(struct mddev *mddev)
 {
-	if (!IS_ENABLED(CONFIG_MD_BITMAP)) {
+	if (!IS_ENABLED(CONFIG_MD_BITMAP))
 		mddev->bitmap_id = ID_BITMAP_NONE;
-	} else {
+	else
 		mddev->bitmap_id = ID_BITMAP;
-		if (!mddev_set_bitmap_ops(mddev))
-			return -EINVAL;
-	}
 
 	if (percpu_ref_init(&mddev->active_io, active_io_release,
-			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
-		mddev_clear_bitmap_ops(mddev);
+			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
 		return -ENOMEM;
-	}
 
 	if (percpu_ref_init(&mddev->writes_pending, no_op,
 			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
-		mddev_clear_bitmap_ops(mddev);
 		percpu_ref_exit(&mddev->active_io);
 		return -ENOMEM;
 	}
@@ -752,6 +763,7 @@ int mddev_init(struct mddev *mddev)
 	mddev->resync_min = 0;
 	mddev->resync_max = MaxSector;
 	mddev->level = LEVEL_NONE;
+	mddev->bitmap_id = ID_BITMAP;
 
 	INIT_WORK(&mddev->sync_work, md_start_sync);
 	INIT_WORK(&mddev->del_work, mddev_delayed_delete);
@@ -762,7 +774,6 @@ EXPORT_SYMBOL_GPL(mddev_init);
 
 void mddev_destroy(struct mddev *mddev)
 {
-	mddev_clear_bitmap_ops(mddev);
 	percpu_ref_exit(&mddev->active_io);
 	percpu_ref_exit(&mddev->writes_pending);
 }
@@ -6125,11 +6136,6 @@ struct mddev *md_alloc(dev_t dev, char *name)
 		return ERR_PTR(error);
 	}
 
-	if (md_bitmap_registered(mddev) && mddev->bitmap_ops->group)
-		if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
-			pr_warn("md: cannot register extra 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");
@@ -6205,6 +6211,26 @@ static void md_safemode_timeout(struct timer_list *t)
 
 static int start_dirty_degraded;
 
+static int md_bitmap_create(struct mddev *mddev)
+{
+	if (mddev->bitmap_id == ID_BITMAP_NONE)
+		return -EINVAL;
+
+	if (!mddev_set_bitmap_ops(mddev))
+		return -ENOENT;
+
+	return mddev->bitmap_ops->create(mddev);
+}
+
+static void md_bitmap_destroy(struct mddev *mddev)
+{
+	if (!md_bitmap_registered(mddev))
+		return;
+
+	mddev->bitmap_ops->destroy(mddev);
+	mddev_clear_bitmap_ops(mddev);
+}
+
 int md_run(struct mddev *mddev)
 {
 	int err;
@@ -6369,9 +6395,9 @@ int md_run(struct mddev *mddev)
 			(unsigned long long)pers->size(mddev, 0, 0) / 2);
 		err = -EINVAL;
 	}
-	if (err == 0 && pers->sync_request && md_bitmap_registered(mddev) &&
+	if (err == 0 && pers->sync_request &&
 	    (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
-		err = mddev->bitmap_ops->create(mddev);
+		err = md_bitmap_create(mddev);
 		if (err)
 			pr_warn("%s: failed to create bitmap (%d)\n",
 				mdname(mddev), err);
@@ -6444,8 +6470,7 @@ int md_run(struct mddev *mddev)
 		pers->free(mddev, mddev->private);
 	mddev->private = NULL;
 	put_pers(pers);
-	if (md_bitmap_registered(mddev))
-		mddev->bitmap_ops->destroy(mddev);
+	md_bitmap_destroy(mddev);
 abort:
 	bioset_exit(&mddev->io_clone_set);
 exit_sync_set:
@@ -6468,7 +6493,7 @@ int do_md_run(struct mddev *mddev)
 	if (md_bitmap_registered(mddev)) {
 		err = mddev->bitmap_ops->load(mddev);
 		if (err) {
-			mddev->bitmap_ops->destroy(mddev);
+			md_bitmap_destroy(mddev);
 			goto out;
 		}
 	}
@@ -6659,8 +6684,7 @@ static void __md_stop(struct mddev *mddev)
 {
 	struct md_personality *pers = mddev->pers;
 
-	if (md_bitmap_registered(mddev))
-		mddev->bitmap_ops->destroy(mddev);
+	md_bitmap_destroy(mddev);
 	mddev_detach(mddev);
 	spin_lock(&mddev->lock);
 	mddev->pers = NULL;
@@ -7440,16 +7464,16 @@ static int set_bitmap_file(struct mddev *mddev, int fd)
 	err = 0;
 	if (mddev->pers) {
 		if (fd >= 0) {
-			err = mddev->bitmap_ops->create(mddev);
+			err = md_bitmap_create(mddev);
 			if (!err)
 				err = mddev->bitmap_ops->load(mddev);
 
 			if (err) {
-				mddev->bitmap_ops->destroy(mddev);
+				md_bitmap_destroy(mddev);
 				fd = -1;
 			}
 		} else if (fd < 0) {
-			mddev->bitmap_ops->destroy(mddev);
+			md_bitmap_destroy(mddev);
 		}
 	}
 
@@ -7764,12 +7788,12 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 				mddev->bitmap_info.default_offset;
 			mddev->bitmap_info.space =
 				mddev->bitmap_info.default_space;
-			rv = mddev->bitmap_ops->create(mddev);
+			rv = md_bitmap_create(mddev);
 			if (!rv)
 				rv = mddev->bitmap_ops->load(mddev);
 
 			if (rv)
-				mddev->bitmap_ops->destroy(mddev);
+				md_bitmap_destroy(mddev);
 		} else {
 			struct md_bitmap_stats stats;
 
@@ -7795,7 +7819,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 				put_cluster_ops(mddev);
 				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
 			}
-			mddev->bitmap_ops->destroy(mddev);
+			md_bitmap_destroy(mddev);
 			mddev->bitmap_info.offset = 0;
 		}
 	}
-- 
2.39.2
Re: [PATCH v3 06/11] md/md-bitmap: delay registration of bitmap_ops until creating bitmap
Posted by Hannes Reinecke 2 months, 2 weeks ago
On 7/18/25 11:23, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently bitmap_ops is registered while allocating mddev, this is fine
> when there is only one bitmap_ops.
> 
> Delay setting bitmap_ops until creating bitmap, so that user can choose
> which bitmap to use before running the array.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   Documentation/admin-guide/md.rst |  3 ++
>   drivers/md/md.c                  | 82 +++++++++++++++++++++-----------
>   2 files changed, 56 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/admin-guide/md.rst b/Documentation/admin-guide/md.rst
> index 356d2a344f08..03a9f5025f99 100644
> --- a/Documentation/admin-guide/md.rst
> +++ b/Documentation/admin-guide/md.rst
> @@ -388,6 +388,9 @@ All md devices contain:
>        bitmap
>            The default internal bitmap
>   
> +If bitmap_type is not none, then additional bitmap attributes will be
> +created after md device KOBJ_CHANGE event.
> +
>   If bitmap_type is bitmap, then the md device will also contain:
>   
>     bitmap/location

Please state _which_ attributes are created with the KOBJ_CHANGE
event.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d8b0dfdb4bfc..639b0143cbb1 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -674,9 +674,11 @@ static void no_op(struct percpu_ref *r) {}
>   
>   static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   {
> +	struct bitmap_operations *old = mddev->bitmap_ops;
>   	struct md_submodule_head *head;
>   
> -	if (mddev->bitmap_id == ID_BITMAP_NONE)
> +	if (mddev->bitmap_id == ID_BITMAP_NONE ||
> +	    (old && old->head.id == mddev->bitmap_id))
>   		return true;
>   
>   	xa_lock(&md_submodule);
> @@ -694,6 +696,18 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   
>   	mddev->bitmap_ops = (void *)head;
>   	xa_unlock(&md_submodule);
> +
> +	if (mddev->bitmap_ops->group) {
> +		if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> +			pr_warn("md: cannot register extra bitmap attributes for %s\n",
> +				mdname(mddev));
> +		else
> +			/*
> +			 * Inform user with KOBJ_CHANGE about new bitmap
> +			 * attributes.
> +			 */
> +			kobject_uevent(&mddev->kobj, KOBJ_CHANGE);
> +	}
>   	return true;
>   
>   err:
> @@ -703,28 +717,25 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   
>   static void mddev_clear_bitmap_ops(struct mddev *mddev)
>   {
> +	if (mddev->bitmap_ops && mddev->bitmap_ops->group)
> +		sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
> +
>   	mddev->bitmap_ops = NULL;
>   }
>   
>   int mddev_init(struct mddev *mddev)
>   {
> -	if (!IS_ENABLED(CONFIG_MD_BITMAP)) {
> +	if (!IS_ENABLED(CONFIG_MD_BITMAP))
>   		mddev->bitmap_id = ID_BITMAP_NONE;
> -	} else {
> +	else
>   		mddev->bitmap_id = ID_BITMAP;
> -		if (!mddev_set_bitmap_ops(mddev))
> -			return -EINVAL;
> -	}
>   
>   	if (percpu_ref_init(&mddev->active_io, active_io_release,
> -			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> -		mddev_clear_bitmap_ops(mddev);
> +			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
>   		return -ENOMEM;
> -	}
>   
>   	if (percpu_ref_init(&mddev->writes_pending, no_op,
>   			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> -		mddev_clear_bitmap_ops(mddev);
>   		percpu_ref_exit(&mddev->active_io);
>   		return -ENOMEM;
>   	}
> @@ -752,6 +763,7 @@ int mddev_init(struct mddev *mddev)
>   	mddev->resync_min = 0;
>   	mddev->resync_max = MaxSector;
>   	mddev->level = LEVEL_NONE;
> +	mddev->bitmap_id = ID_BITMAP;
>   
>   	INIT_WORK(&mddev->sync_work, md_start_sync);
>   	INIT_WORK(&mddev->del_work, mddev_delayed_delete);
> @@ -762,7 +774,6 @@ EXPORT_SYMBOL_GPL(mddev_init);
>   
>   void mddev_destroy(struct mddev *mddev)
>   {
> -	mddev_clear_bitmap_ops(mddev);
>   	percpu_ref_exit(&mddev->active_io);
>   	percpu_ref_exit(&mddev->writes_pending);
>   }
> @@ -6125,11 +6136,6 @@ struct mddev *md_alloc(dev_t dev, char *name)
>   		return ERR_PTR(error);
>   	}
>   
> -	if (md_bitmap_registered(mddev) && mddev->bitmap_ops->group)
> -		if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> -			pr_warn("md: cannot register extra 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");
> @@ -6205,6 +6211,26 @@ static void md_safemode_timeout(struct timer_list *t)
>   
>   static int start_dirty_degraded;
>   
> +static int md_bitmap_create(struct mddev *mddev)
> +{
> +	if (mddev->bitmap_id == ID_BITMAP_NONE)
> +		return -EINVAL;
> +
> +	if (!mddev_set_bitmap_ops(mddev))
> +		return -ENOENT;
> +
> +	return mddev->bitmap_ops->create(mddev);
> +}
> +
> +static void md_bitmap_destroy(struct mddev *mddev)
> +{
> +	if (!md_bitmap_registered(mddev))
> +		return;
> +
> +	mddev->bitmap_ops->destroy(mddev);
> +	mddev_clear_bitmap_ops(mddev);
> +}
> +
>   int md_run(struct mddev *mddev)
>   {
>   	int err;
> @@ -6369,9 +6395,9 @@ int md_run(struct mddev *mddev)
>   			(unsigned long long)pers->size(mddev, 0, 0) / 2);
>   		err = -EINVAL;
>   	}
> -	if (err == 0 && pers->sync_request && md_bitmap_registered(mddev) &&
> +	if (err == 0 && pers->sync_request &&
>   	    (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
> -		err = mddev->bitmap_ops->create(mddev);
> +		err = md_bitmap_create(mddev);
>   		if (err)
>   			pr_warn("%s: failed to create bitmap (%d)\n",
>   				mdname(mddev), err);
> @@ -6444,8 +6470,7 @@ int md_run(struct mddev *mddev)
>   		pers->free(mddev, mddev->private);
>   	mddev->private = NULL;
>   	put_pers(pers);
> -	if (md_bitmap_registered(mddev))
> -		mddev->bitmap_ops->destroy(mddev);
> +	md_bitmap_destroy(mddev);
>   abort:
>   	bioset_exit(&mddev->io_clone_set);
>   exit_sync_set:
> @@ -6468,7 +6493,7 @@ int do_md_run(struct mddev *mddev)
>   	if (md_bitmap_registered(mddev)) {
>   		err = mddev->bitmap_ops->load(mddev);
>   		if (err) {
> -			mddev->bitmap_ops->destroy(mddev);
> +			md_bitmap_destroy(mddev);
>   			goto out;
>   		}
>   	}
> @@ -6659,8 +6684,7 @@ static void __md_stop(struct mddev *mddev)
>   {
>   	struct md_personality *pers = mddev->pers;
>   
> -	if (md_bitmap_registered(mddev))
> -		mddev->bitmap_ops->destroy(mddev);
> +	md_bitmap_destroy(mddev);
>   	mddev_detach(mddev);
>   	spin_lock(&mddev->lock);
>   	mddev->pers = NULL;
> @@ -7440,16 +7464,16 @@ static int set_bitmap_file(struct mddev *mddev, int fd)
>   	err = 0;
>   	if (mddev->pers) {
>   		if (fd >= 0) {
> -			err = mddev->bitmap_ops->create(mddev);
> +			err = md_bitmap_create(mddev);
>   			if (!err)
>   				err = mddev->bitmap_ops->load(mddev);
>   
>   			if (err) {
> -				mddev->bitmap_ops->destroy(mddev);
> +				md_bitmap_destroy(mddev);
>   				fd = -1;
>   			}
>   		} else if (fd < 0) {
> -			mddev->bitmap_ops->destroy(mddev);
> +			md_bitmap_destroy(mddev);
>   		}
>   	}
>   
> @@ -7764,12 +7788,12 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
>   				mddev->bitmap_info.default_offset;
>   			mddev->bitmap_info.space =
>   				mddev->bitmap_info.default_space;
> -			rv = mddev->bitmap_ops->create(mddev);
> +			rv = md_bitmap_create(mddev);
>   			if (!rv)
>   				rv = mddev->bitmap_ops->load(mddev);
>   
>   			if (rv)
> -				mddev->bitmap_ops->destroy(mddev);
> +				md_bitmap_destroy(mddev);
>   		} else {
>   			struct md_bitmap_stats stats;
>   
> @@ -7795,7 +7819,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
>   				put_cluster_ops(mddev);
>   				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
>   			}
> -			mddev->bitmap_ops->destroy(mddev);
> +			md_bitmap_destroy(mddev);
>   			mddev->bitmap_info.offset = 0;
>   		}
>   	}

Otherwise:
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Re: [PATCH v3 06/11] md/md-bitmap: delay registration of bitmap_ops until creating bitmap
Posted by Yu Kuai 2 months, 2 weeks ago
Hi,

在 2025/07/21 14:01, Hannes Reinecke 写道:
>> diff --git a/Documentation/admin-guide/md.rst 
>> b/Documentation/admin-guide/md.rst
>> index 356d2a344f08..03a9f5025f99 100644
>> --- a/Documentation/admin-guide/md.rst
>> +++ b/Documentation/admin-guide/md.rst
>> @@ -388,6 +388,9 @@ All md devices contain:
>>        bitmap
>>            The default internal bitmap
>> +If bitmap_type is not none, then additional bitmap attributes will be
>> +created after md device KOBJ_CHANGE event.
>> +
>>   If bitmap_type is bitmap, then the md device will also contain:
>>     bitmap/location
> 
> Please state _which_ attributes are created with the KOBJ_CHANGE
> event.

Just a notice in case you missed that following statements are exactly
the attributes for bitmap, or do I misunderstood somehow?

Thanks,
Kuai

Re: [PATCH v3 06/11] md/md-bitmap: delay registration of bitmap_ops until creating bitmap
Posted by Li Nan 2 months, 2 weeks ago

在 2025/7/18 17:23, Yu Kuai 写道:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently bitmap_ops is registered while allocating mddev, this is fine
> when there is only one bitmap_ops.
> 
> Delay setting bitmap_ops until creating bitmap, so that user can choose
> which bitmap to use before running the array.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   Documentation/admin-guide/md.rst |  3 ++
>   drivers/md/md.c                  | 82 +++++++++++++++++++++-----------
>   2 files changed, 56 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/admin-guide/md.rst b/Documentation/admin-guide/md.rst
> index 356d2a344f08..03a9f5025f99 100644
> --- a/Documentation/admin-guide/md.rst
> +++ b/Documentation/admin-guide/md.rst
> @@ -388,6 +388,9 @@ All md devices contain:
>        bitmap
>            The default internal bitmap
>   
> +If bitmap_type is not none, then additional bitmap attributes will be
> +created after md device KOBJ_CHANGE event.
> +
>   If bitmap_type is bitmap, then the md device will also contain:
>   
>     bitmap/location
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d8b0dfdb4bfc..639b0143cbb1 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -674,9 +674,11 @@ static void no_op(struct percpu_ref *r) {}
>   
>   static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   {
> +	struct bitmap_operations *old = mddev->bitmap_ops;
>   	struct md_submodule_head *head;
>   
> -	if (mddev->bitmap_id == ID_BITMAP_NONE)
> +	if (mddev->bitmap_id == ID_BITMAP_NONE ||
> +	    (old && old->head.id == mddev->bitmap_id))
>   		return true;
>   
>   	xa_lock(&md_submodule);
> @@ -694,6 +696,18 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   
>   	mddev->bitmap_ops = (void *)head;
>   	xa_unlock(&md_submodule);
> +
> +	if (mddev->bitmap_ops->group) {
> +		if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> +			pr_warn("md: cannot register extra bitmap attributes for %s\n",
> +				mdname(mddev));
> +		else
> +			/*
> +			 * Inform user with KOBJ_CHANGE about new bitmap
> +			 * attributes.
> +			 */
> +			kobject_uevent(&mddev->kobj, KOBJ_CHANGE);
> +	}
>   	return true;
>   
>   err:
> @@ -703,28 +717,25 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   
>   static void mddev_clear_bitmap_ops(struct mddev *mddev)
>   {
> +	if (mddev->bitmap_ops && mddev->bitmap_ops->group)
> +		sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
> +
>   	mddev->bitmap_ops = NULL;
>   }
>   
>   int mddev_init(struct mddev *mddev)
>   {
> -	if (!IS_ENABLED(CONFIG_MD_BITMAP)) {
> +	if (!IS_ENABLED(CONFIG_MD_BITMAP))
>   		mddev->bitmap_id = ID_BITMAP_NONE;
> -	} else {
> +	else
>   		mddev->bitmap_id = ID_BITMAP;

'bitmap_id' is set here.

> -		if (!mddev_set_bitmap_ops(mddev))
> -			return -EINVAL;
> -	}
>   
>   	if (percpu_ref_init(&mddev->active_io, active_io_release,
> -			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> -		mddev_clear_bitmap_ops(mddev);
> +			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
>   		return -ENOMEM;
> -	}
>   
>   	if (percpu_ref_init(&mddev->writes_pending, no_op,
>   			    PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> -		mddev_clear_bitmap_ops(mddev);
>   		percpu_ref_exit(&mddev->active_io);
>   		return -ENOMEM;
>   	}
> @@ -752,6 +763,7 @@ int mddev_init(struct mddev *mddev)
>   	mddev->resync_min = 0;
>   	mddev->resync_max = MaxSector;
>   	mddev->level = LEVEL_NONE;
> +	mddev->bitmap_id = ID_BITMAP;
>   

This change is wrong.

-- 
Thanks,
Nan

Re: [PATCH v3 06/11] md/md-bitmap: delay registration of bitmap_ops until creating bitmap
Posted by Yu Kuai 2 months, 2 weeks ago
Hi,

在 2025/7/19 14:49, Li Nan 写道:
>
>
> 在 2025/7/18 17:23, Yu Kuai 写道:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Currently bitmap_ops is registered while allocating mddev, this is fine
>> when there is only one bitmap_ops.
>>
>> Delay setting bitmap_ops until creating bitmap, so that user can choose
>> which bitmap to use before running the array.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   Documentation/admin-guide/md.rst |  3 ++
>>   drivers/md/md.c                  | 82 +++++++++++++++++++++-----------
>>   2 files changed, 56 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/md.rst 
>> b/Documentation/admin-guide/md.rst
>> index 356d2a344f08..03a9f5025f99 100644
>> --- a/Documentation/admin-guide/md.rst
>> +++ b/Documentation/admin-guide/md.rst
>> @@ -388,6 +388,9 @@ All md devices contain:
>>        bitmap
>>            The default internal bitmap
>>   +If bitmap_type is not none, then additional bitmap attributes will be
>> +created after md device KOBJ_CHANGE event.
>> +
>>   If bitmap_type is bitmap, then the md device will also contain:
>>       bitmap/location
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index d8b0dfdb4bfc..639b0143cbb1 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -674,9 +674,11 @@ static void no_op(struct percpu_ref *r) {}
>>     static bool mddev_set_bitmap_ops(struct mddev *mddev)
>>   {
>> +    struct bitmap_operations *old = mddev->bitmap_ops;
>>       struct md_submodule_head *head;
>>   -    if (mddev->bitmap_id == ID_BITMAP_NONE)
>> +    if (mddev->bitmap_id == ID_BITMAP_NONE ||
>> +        (old && old->head.id == mddev->bitmap_id))
>>           return true;
>>         xa_lock(&md_submodule);
>> @@ -694,6 +696,18 @@ static bool mddev_set_bitmap_ops(struct mddev 
>> *mddev)
>>         mddev->bitmap_ops = (void *)head;
>>       xa_unlock(&md_submodule);
>> +
>> +    if (mddev->bitmap_ops->group) {
>> +        if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
>> +            pr_warn("md: cannot register extra bitmap attributes for 
>> %s\n",
>> +                mdname(mddev));
>> +        else
>> +            /*
>> +             * Inform user with KOBJ_CHANGE about new bitmap
>> +             * attributes.
>> +             */
>> +            kobject_uevent(&mddev->kobj, KOBJ_CHANGE);
>> +    }
>>       return true;
>>     err:
>> @@ -703,28 +717,25 @@ static bool mddev_set_bitmap_ops(struct mddev 
>> *mddev)
>>     static void mddev_clear_bitmap_ops(struct mddev *mddev)
>>   {
>> +    if (mddev->bitmap_ops && mddev->bitmap_ops->group)
>> +        sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
>> +
>>       mddev->bitmap_ops = NULL;
>>   }
>>     int mddev_init(struct mddev *mddev)
>>   {
>> -    if (!IS_ENABLED(CONFIG_MD_BITMAP)) {
>> +    if (!IS_ENABLED(CONFIG_MD_BITMAP))
>>           mddev->bitmap_id = ID_BITMAP_NONE;
>> -    } else {
>> +    else
>>           mddev->bitmap_id = ID_BITMAP;
>
> 'bitmap_id' is set here.
>
>> -        if (!mddev_set_bitmap_ops(mddev))
>> -            return -EINVAL;
>> -    }
>>         if (percpu_ref_init(&mddev->active_io, active_io_release,
>> -                PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
>> -        mddev_clear_bitmap_ops(mddev);
>> +                PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
>>           return -ENOMEM;
>> -    }
>>         if (percpu_ref_init(&mddev->writes_pending, no_op,
>>                   PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
>> -        mddev_clear_bitmap_ops(mddev);
>>           percpu_ref_exit(&mddev->active_io);
>>           return -ENOMEM;
>>       }
>> @@ -752,6 +763,7 @@ int mddev_init(struct mddev *mddev)
>>       mddev->resync_min = 0;
>>       mddev->resync_max = MaxSector;
>>       mddev->level = LEVEL_NONE;
>> +    mddev->bitmap_id = ID_BITMAP;
>
> This change is wrong.

Yes, this line from v1 should be removed. Will change this in v3.

Thanks,
Kuai