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

[Xiao Ni <xni@redhat.com>]

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.

Regards
Xiao
>
> Thansk,
> Blazej
>