linux-kernel - Re: [PATCH v3 06/11] md/md-bitmap: delay registration of bitmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a41300dd-22af-4619-9b68-a5cd6f8aa259@kernel.org>
Date: Sat, 19 Jul 2025 17:45:06 +0800
From: Yu Kuai <yukuai@...nel.org>
To: Li Nan <linan666@...weicloud.com>, Yu Kuai <yukuai1@...weicloud.com>,
 corbet@....net, agk@...hat.com, snitzer@...nel.org, mpatocka@...hat.com,
 song@...nel.org, yukuai3@...wei.com, hare@...e.de
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 dm-devel@...ts.linux.dev, linux-raid@...r.kernel.org, yi.zhang@...wei.com,
 yangerkun@...wei.com, johnny.chenyi@...wei.com
Subject: Re: [PATCH v3 06/11] md/md-bitmap: delay registration of bitmap_ops
 until creating bitmap

Hi,

在 2025/7/19 14:49, Li Nan 写道:
>
>
> 在 2025/7/18 17:23, Yu Kuai 写道:
>> From: Yu Kuai <yukuai3@...wei.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@...wei.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