Re: [PATCH v2] mdadm: Clear extra flags when initializing metadata

[Wu Guanghao <wuguanghao3@huawei.com>]

在 2025/3/11 9:10, Xiao Ni 写道:
> On Mon, Mar 10, 2025 at 10:42 PM Blazej Kucman
> <blazej.kucman@linux.intel.com> wrote:
>>
>> On Mon, 10 Mar 2025 19:09:36 +0800
>> Wu Guanghao <wuguanghao3@huawei.com> wrote:
>>
>> Hi,
>> Thanks for your patch.
>>
>> you are only adding a change to native metadata so it would be good to
>> emphasize this in the title, please change "mdadm:" to "super1:"
>>
>> There are also a few checkpatch issues,
>>
>>
>>> When adding a disk to a RAID1 array, the metadata is read from the
>>> existing member disks for sync. However, only the bad_blocks flag are
>>> copied, the bad_blocks records are not copied, so the bad_blocks
>>> records are all zeros. The kernel function super_1_load() detects
>>> bad_blocks flag and reads the bad_blocks record, then sets the bad
>>> block using badblocks_set().
>>
>> WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit
>> description?) #8:
>> the bad_blocks records are not copied, so the bad_blocks records are
>> all zeros.
>>
>>>
>>> After the kernel commit 1726c7746("badblocks: improve badblocks_set()
>>> for multiple ranges handling"), if the length of a bad_blocks record
>>
>> please use SHA-1 ID - first 12 characters and space between ID and
>> (Tile)
>>
>>> is 0, it will return a failure. Therefore the device addition will
>>> fail.
>>>
>>> So when adding a new disk, some flags cannot be sync and need to be
>>> cleared.
>>>
>>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
>>> ---
>>> Changelog:
>>> v2:
>>>     Add a testcase.
>>>     Clear extra replace flag.
>>> ---
>>>  super1.c                 |  4 ++++
>>>  tests/05r1-add-badblocks | 25 +++++++++++++++++++++++++
>>>  2 files changed, 29 insertions(+)
>>>  create mode 100644 tests/05r1-add-badblocks
>>>
>>> diff --git a/super1.c b/super1.c
>>> index fe3c4c64..f4a29f4f 100644
>>> --- a/super1.c
>>> +++ b/super1.c
>>> @@ -1971,6 +1971,10 @@ static int write_init_super1(struct supertype
>>> *st) long bm_offset;
>>>       bool raid0_need_layout = false;
>>>
>>> +     /* Clear extra flags */
>>> +     sb->feature_map &= ~__cpu_to_le32(MD_FEATURE_BAD_BLOCKS |
>>> +                                          MD_FEATURE_REPLACEMENT);
>>
>> ERROR: code indent should use tabs where possible
>> #36: FILE: super1.c:1976:
>> +                                          MD_FEATURE_REPLACEMENT);$
>>
>> WARNING: please, no spaces at the start of a line
>> #36: FILE: super1.c:1976:
>> +                                          MD_FEATURE_REPLACEMENT);$
>>
>> However, in this case the code will fit on one line, the limit is 100
>> characters.
>>
>>> +
>>>       /* Since linux kernel v5.4, raid0 always has a layout */
>>>       if (has_raid0_layout(sb) && get_linux_version() >= 5004000)
>>>               raid0_need_layout = true;
>>> diff --git a/tests/05r1-add-badblocks b/tests/05r1-add-badblocks
>>> new file mode 100644
>>> index 00000000..88b064f2
>>> --- /dev/null
>>> +++ b/tests/05r1-add-badblocks
>>> @@ -0,0 +1,25 @@
>>> +#
>>> +# create a raid1 with a drive and set badblocks for the drive.
>>> +# add a new drive does not cause an error.
>>> +#
>>> +
>>> +# create raid1
>>> +mdadm -CR $md0 -l1 -n2 -e1.0 $dev1 missing
>>> +testdev $md0 1 $mdsize1a 64
>>> +sleep 3
>>> +
>>> +# set badblocks for the drive
>>> +dev1_name=$(basename $dev1)
>>> +echo "10000 100" > /sys/block/md0/md/dev-$dev1_name/bad_blocks
>>> +echo "write_error" > /sys/block/md0/md/dev-$dev1_name/state
>>> +
>>> +# maybe fail but that's ok, as it's only intended to
>>> +# record the bad block in the metadata.
>>> +mkfs.ext3 $md0
>>> +
>>> +# re-add and recovery
>>> +mdadm $md0 -a $dev2
>>> +check recovery
>>> +
>>> +mdadm -S $md0
>>> +
>>
>> Since you added the test, would you be able to issue a PR on github
>> to get the tests running?
> 
> Hi all
> 
> I've been fixing the test failures recently and I'll create a PR for
> this patch set. So you can create a PR tomorrow. If you are not
> familiar with this, I can help.
>
Is this the repository: https://github.com/md-raid-utilities/mdadm/ ?
If so, I will crerate a PR containing the new changes.


> Regards
> Xiao
>>
>> Thansk,
>> Blazej
>>
> 
> 
> .