[PATCH] mdadm: fix --grow with --add for linear

[PATCH] mdadm: fix --grow with --add for linear

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

For the case mdadm --grow with --add, the s.btype should not be
initialized yet, hence BitmapUnknown should be checked instead of
BitmapNone.

Noted that this behaviour should only support by md-linear, which is
removed from kernel, howerver, it turns out md-linear is used widely
in home NAS and we're planning to reintroduce it soon.

Fixes: 581ba1341017 ("mdadm: remove bitmap file support")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 mdadm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mdadm.c b/mdadm.c
index 7d3b656b..1fd4dcba 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1625,7 +1625,7 @@ int main(int argc, char *argv[])
 		if (devs_found > 1 && s.raiddisks == 0 && s.level == UnSet) {
 			/* must be '-a'. */
 			if (s.size > 0 || s.chunk ||
-			    s.layout_str || s.btype != BitmapNone) {
+			    s.layout_str || s.btype != BitmapUnknown) {
 				pr_err("--add cannot be used with other geometry changes in --grow mode\n");
 				rv = 1;
 				break;
-- 
2.39.2

Re: [PATCH] mdadm: fix --grow with --add for linear

[Mariusz Tkaczyk]

On Fri, 27 Dec 2024 14:07:02 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> For the case mdadm --grow with --add, the s.btype should not be
> initialized yet, hence BitmapUnknown should be checked instead of
> BitmapNone.

Hi Kuai,

For commit extra clarity it would be nice to include command you are
executing.

What if someone will do (not tested):
#mdadm --grow /dev/md0 --add /dev/sdx --bitmap=none

I think that it is perfectly valid, now it may work but I expect your
change to broke it.

I would say we need:

bool is_bitmap_set(struct shape *s) {
	if (s.layout)
		return true;
	if (s.btype == BitmapNone || s.btype != BitmapUnknown)
		return false;

	return true;
}

And respect both cases. Setting property to default should never be a
mistake.

Has it some sense? If no, I miss some explanation in commit message (or
better comment).

> 
> Noted that this behaviour should only support by md-linear, which is
> removed from kernel, howerver, it turns out md-linear is used widely
> in home NAS and we're planning to reintroduce it soon.

Wow. We get a lesson.

For the code, LGTM.

Thanks,
Mariusz

Re: [PATCH] mdadm: fix --grow with --add for linear

[Yu Kuai]

在 2024/12/31 16:49, Mariusz Tkaczyk 写道:
> On Fri, 27 Dec 2024 14:07:02 +0800
> Yu Kuai <yukuai1@huaweicloud.com> wrote:
> 
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> For the case mdadm --grow with --add, the s.btype should not be
>> initialized yet, hence BitmapUnknown should be checked instead of
>> BitmapNone.
> 
> Hi Kuai,
> 
> For commit extra clarity it would be nice to include command you are
> executing.
> 
> What if someone will do (not tested):
> #mdadm --grow /dev/md0 --add /dev/sdx --bitmap=none
> 
> I think that it is perfectly valid, now it may work but I expect your
> change to broke it.

Hi,

Sorry for the late reply, I forgot about this patch somehow :(

Changes from commit 581ba1341017:

@@ -1634,7 +1625,7 @@ int main(int argc, char *argv[])
                 if (devs_found > 1 && s.raiddisks == 0 && s.level == 
UnSet) {
                         /* must be '-a'. */
                         if (s.size > 0 || s.chunk ||
-                           s.layout_str || s.bitmap_file) {
+                           s.layout_str || s.btype != BitmapNone) {
                                 pr_err("--add cannot be used with other 
geometry changes in --grow mode\n");
                                 rv = 1;
                                 break;


Hence before the commit, bitmap=none is not valid in this case as well,
because s.bitmap_file will set to "none" in this case.

Thanks,
Kuai

> 
> I would say we need:
> 
> bool is_bitmap_set(struct shape *s) {
> 	if (s.layout)
> 		return true;
> 	if (s.btype == BitmapNone || s.btype != BitmapUnknown)
> 		return false;
> 
> 	return true;
> }
> 
> And respect both cases. Setting property to default should never be a
> mistake.
> 
> Has it some sense? If no, I miss some explanation in commit message (or
> better comment).
> 
>>
>> Noted that this behaviour should only support by md-linear, which is
>> removed from kernel, howerver, it turns out md-linear is used widely
>> in home NAS and we're planning to reintroduce it soon.
> 
> Wow. We get a lesson.
> 
> For the code, LGTM.
> 
> Thanks,
> Mariusz
> 
> .
> 

Re: [PATCH] mdadm: fix --grow with --add for linear

[Mariusz Tkaczyk]

On Sun, 26 Jan 2025 16:22:58 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> 在 2024/12/31 16:49, Mariusz Tkaczyk 写道:
> > On Fri, 27 Dec 2024 14:07:02 +0800
> > Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >   
> >> From: Yu Kuai <yukuai3@huawei.com>
> >>
> >> For the case mdadm --grow with --add, the s.btype should not be
> >> initialized yet, hence BitmapUnknown should be checked instead of
> >> BitmapNone.  
> > 
> > Hi Kuai,
> > 
> > For commit extra clarity it would be nice to include command you are
> > executing.
> > 
> > What if someone will do (not tested):
> > #mdadm --grow /dev/md0 --add /dev/sdx --bitmap=none
> > 
> > I think that it is perfectly valid, now it may work but I expect
> > your change to broke it.  
> 
> Hi,
> 
> Sorry for the late reply, I forgot about this patch somehow :(
> 
> Changes from commit 581ba1341017:
> 
> @@ -1634,7 +1625,7 @@ int main(int argc, char *argv[])
>                  if (devs_found > 1 && s.raiddisks == 0 && s.level == 
> UnSet) {
>                          /* must be '-a'. */
>                          if (s.size > 0 || s.chunk ||
> -                           s.layout_str || s.bitmap_file) {
> +                           s.layout_str || s.btype != BitmapNone) {
>                                  pr_err("--add cannot be used with
> other geometry changes in --grow mode\n");
>                                  rv = 1;
>                                  break;
> 
> 
> Hence before the commit, bitmap=none is not valid in this case as
> well, because s.bitmap_file will set to "none" in this case.
> 
> Thanks,
> Kuai
> 

OK, ack from my side then.
If there will be no more comments we can merge it. I opened
PR to test it:
https://github.com/md-raid-utilities/mdadm/pull/145

Thanks,
Mariusz