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

[Blazej Kucman <blazej.kucman@linux.intel.com>]

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?

Thansk,
Blazej