| Message ID | 27127b7d-7da6-cd31-01db-6725884a7286@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] mdadm: Clear extra flags when initializing metadata | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_14-PR | fail | merge-conflict |
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
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 >
在 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 >> > > > .
在 2025/3/10 22:41, Blazej Kucman 写道: > 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. > Thank you for your feedback. I will modify it in the next version. >> + >> /* 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? > Sure, I will create a PR and run the testcase. > Thansk, > Blazej > > .
On Tue, Mar 11, 2025 at 10:08 AM Wu Guanghao <wuguanghao3@huawei.com> wrote: > > > > 在 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. Yes, you're right. > > > > Regards > > Xiao > >> > >> Thansk, > >> Blazej > >> > > > > > > . >
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); + /* 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 +
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(). After the kernel commit 1726c7746("badblocks: improve badblocks_set() for multiple ranges handling"), if the length of a bad_blocks record 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