Message-ID: <23b75e25-fa2f-4d12-8d96-6de01e43ad49@suse.de>
Date: Tue, 27 May 2025 08:10:10 +0200
From: Hannes Reinecke <hare@...e.de>
To: Yu Kuai <yukuai1@...weicloud.com>, hch@....de, xni@...hat.com,
 colyli@...nel.org, song@...nel.org, yukuai3@...wei.com
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-raid@...r.kernel.org, yi.zhang@...wei.com, yangerkun@...wei.com,
 johnny.chenyi@...wei.com
Subject: Re: [PATCH 06/23] md/md-bitmap: add a new sysfs api bitmap_type

On 5/24/25 08:13, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@...wei.com>
> 
> The api will be used by mdadm to set bitmap_ops while creating new array
> or assemble array, prepare to add a new bitmap.
> 
> Currently available options are:
> 
> cat /sys/block/md0/md/bitmap_type
> none [bitmap]
> 
> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
> ---
>   Documentation/admin-guide/md.rst | 73 ++++++++++++++----------
>   drivers/md/md.c                  | 96 ++++++++++++++++++++++++++++++--
>   drivers/md/md.h                  |  2 +
>   3 files changed, 135 insertions(+), 36 deletions(-)
> 
[ .. ]
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 311e52d5173d..4eb0c6effd5b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -672,13 +672,18 @@ static void active_io_release(struct percpu_ref *ref)
>   
>   static void no_op(struct percpu_ref *r) {}
>   
> -static void mddev_set_bitmap_ops(struct mddev *mddev, enum md_submodule_id id)
> +static bool mddev_set_bitmap_ops(struct mddev *mddev)
>   {
>   	xa_lock(&md_submodule);
> -	mddev->bitmap_ops = xa_load(&md_submodule, id);
> +	mddev->bitmap_ops = xa_load(&md_submodule, mddev->bitmap_id);
>   	xa_unlock(&md_submodule);
> -	if (!mddev->bitmap_ops)
> -		pr_warn_once("md: can't find bitmap id %d\n", id);
> +
> +	if (!mddev->bitmap_ops) {
> +		pr_warn_once("md: can't find bitmap id %d\n", mddev->bitmap_id);
> +		return false;
> +	}
> +
> +	return true;
>   }
>   
>   static void mddev_clear_bitmap_ops(struct mddev *mddev)
> @@ -688,8 +693,10 @@ static void mddev_clear_bitmap_ops(struct mddev *mddev)
>   
>   int mddev_init(struct mddev *mddev)
>   {
> -	/* TODO: support more versions */
> -	mddev_set_bitmap_ops(mddev, ID_BITMAP);
> +	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)) {
> @@ -4155,6 +4162,82 @@ new_level_store(struct mddev *mddev, const char *buf, size_t len)
>   static struct md_sysfs_entry md_new_level =
>   __ATTR(new_level, 0664, new_level_show, new_level_store);
>   
> +static ssize_t
> +bitmap_type_show(struct mddev *mddev, char *page)
> +{
> +	struct md_submodule_head *head;
> +	unsigned long i;
> +	ssize_t len = 0;
> +
> +	if (mddev->bitmap_id == ID_BITMAP_NONE)
> +		len += sprintf(page + len, "[none] ");
> +	else
> +		len += sprintf(page + len, "none ");
> +
> +	xa_lock(&md_submodule);
> +	xa_for_each(&md_submodule, i, head) {
> +		if (head->type != MD_BITMAP)
> +			continue;
> +
> +		if (mddev->bitmap_id == head->id)
> +			len += sprintf(page + len, "[%s] ", head->name);
> +		else
> +			len += sprintf(page + len, "%s ", head->name);
> +	}
> +	xa_unlock(&md_submodule);
> +
> +	len += sprintf(page + len, "\n");
> +	return len;
> +}
> +
> +static ssize_t
> +bitmap_type_store(struct mddev *mddev, const char *buf, size_t len)
> +{
> +	struct md_submodule_head *head;
> +	enum md_submodule_id id;
> +	unsigned long i;
> +	int err;
> +
> +	if (mddev->bitmap_ops)
> +		return -EBUSY;
> +
Why isn't this protected by md_submodule lock?
The lock is taken when updating ->bitmap_ops, so I would
have expected it to be taken when checking it ...

> +	err = kstrtoint(buf, 10, &id);
> +	if (!err) {
> +		if (id == ID_BITMAP_NONE) {
> +			mddev->bitmap_id = id;
> +			return len;
> +		}
> +
> +		xa_lock(&md_submodule);
> +		head = xa_load(&md_submodule, id);
> +		xa_unlock(&md_submodule);
> +
> +		if (head && head->type == MD_BITMAP) {
> +			mddev->bitmap_id = id;
> +			return len;
> +		}
> +	}
> +
> +	if (cmd_match(buf, "none")) {
> +		mddev->bitmap_id = ID_BITMAP_NONE;
> +		return len;
> +	}
> +
That is odd coding. The 'if (!err)' condition above might
fall through to here, but then we already now that it cannot
match 'none'.
Please invert the logic, first check for 'none', and only
call kstroint if the match failed.

> +	xa_lock(&md_submodule);
> +	xa_for_each(&md_submodule, i, head) {
> +		if (head->type == MD_BITMAP && cmd_match(buf, head->name)) {
> +			mddev->bitmap_id = head->id;
> +			xa_unlock(&md_submodule);
> +			return len;
> +		}
> +	}
> +	xa_unlock(&md_submodule);
> +	return -ENOENT;
> +}
> +
> +static struct md_sysfs_entry md_bitmap_type =
> +__ATTR(bitmap_type, 0664, bitmap_type_show, bitmap_type_store);
> +
>   static ssize_t
>   layout_show(struct mddev *mddev, char *page)
>   {
> @@ -5719,6 +5802,7 @@ __ATTR(serialize_policy, S_IRUGO | S_IWUSR, serialize_policy_show,
>   static struct attribute *md_default_attrs[] = {
>   	&md_level.attr,
>   	&md_new_level.attr,
> +	&md_bitmap_type.attr,
>   	&md_layout.attr,
>   	&md_raid_disks.attr,
>   	&md_uuid.attr,
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 13e3f9ce1b79..bf34c0a36551 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -40,6 +40,7 @@ enum md_submodule_id {
>   	ID_CLUSTER,
>   	ID_BITMAP,
>   	ID_LLBITMAP,	/* TODO */
> +	ID_BITMAP_NONE,
>   };
>   
>   struct md_submodule_head {
> @@ -565,6 +566,7 @@ struct mddev {
>   	struct percpu_ref		writes_pending;
>   	int				sync_checkers;	/* # of threads checking writes_pending */
>   
> +	enum md_submodule_id		bitmap_id;
>   	void				*bitmap; /* the bitmap for the device */
>   	struct bitmap_operations	*bitmap_ops;
>   	struct {

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@...e.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

Powered by blists - more mailing lists