Re: [PATCH 06/23] md/md-bitmap: add a new sysfs api bitmap_type

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/05/27 14:10, Hannes Reinecke 写道:
> On 5/24/25 08:13, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.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@huawei.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 ...

The design is that when bitmap is created, user can no longer set
bitmap_id, and it's right without the protecting there will be race
window.

> 
>> +    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'.

The first kstrtoint() is trying to convert the input string to int id,
looks like I missed return -EINVAL if the id can't be found in
md_submodule.

> Please invert the logic, first check for 'none', and only
> call kstroint if the match failed.

Sure, this sounds better.

Thanks for the review!
Kuai

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