mdadm: Check bitmap first when reshape raid1 to raid0

Hi Neil

I encountered one problem. When reshape one raid1 with bitmap to raid0, it'll lose legs.

I sent the patch by git-send-email, but I can't see the mail in linux-raid. So I forward it again. And add signed-off-by line.

Best Regards
Xiao

----- Forwarded Message -----
From: "Xiao Ni" <[email protected]>
To: [email protected]
Sent: Sunday, October 25, 2015 1:36:02 PM
Subject: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

One raid1 with bitmap is composed by 4 disks. It'll fail when rashape to raid0 and lose 3 disks. It should check bitmap first when reshape raid1 to raid0.

Signed-off-by Xiao Ni <[email protected]>

---
 Grow.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Grow.c +6 -0

index 80d7b22..5e9b0bb 100644
@@ -1898,6 +1898,12 @@ size_change_error:
 	     array.layout == ((1 << 8) + 2) && !(array.raid_disks & 1)) ||
 	    (s->level == 0 && array.level == 1 && sra)) {
 		int err;
+
+		if (array.state & (1<<MD_SB_BITMAP_PRESENT)) {
+			cont_err("Bitmap must be removed before level can be changed\n");
+			rv = 1;
+			goto release;
+		}
 		err = remove_disks_for_takeover(st, sra, array.layout);
 		if (err) {
 			dprintf("Array cannot be reshaped\n");

From: "Neil Brown" <[email protected]>
To: "Xiao Ni" <[email protected]>
Cc: "Jes Sorensen" <[email protected]>, [email protected]
Sent: Monday, October 26, 2015 4:21:36 AM
Subject: Re: Fwd: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

Quoted 55 lines · Xiao Ni

Hi Neil

I encountered one problem. When reshape one raid1 with bitmap to raid0, it'll
lose legs.

I sent the patch by git-send-email, but I can't see the mail in linux-raid. So
I forward it again. And add signed-off-by line.

Best Regards
Xiao

----- Forwarded Message -----
From: "Xiao Ni" <[email protected]>
To: [email protected]
Sent: Sunday, October 25, 2015 1:36:02 PM
Subject: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

One raid1 with bitmap is composed by 4 disks. It'll fail when rashape to raid0
and lose 3 disks. It should check bitmap first when reshape raid1 to raid0.

Signed-off-by Xiao Ni <[email protected]>

---
 Grow.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Grow.c +6 -3

index 80d7b22..5e9b0bb 100644
@@ -1898,6 +1898,12 @@ size_change_error:
 	     array.layout == ((1 << 8) + 2) && !(array.raid_disks & 1)) ||
 	    (s->level == 0 && array.level == 1 && sra)) {
 		int err;
+
+		if (array.state & (1<<MD_SB_BITMAP_PRESENT)) {
+			cont_err("Bitmap must be removed before level can be changed\n");
+			rv = 1;
+			goto release;
+		}
 		err = remove_disks_for_takeover(st, sra, array.layout);
 		if (err) {
 			dprintf("Array cannot be reshaped\n");

@@ -2706,9 +2712,6 @@ static int impose_level(int fd, int level, char
 *devname, int verbose)
 			err = errno;
 			pr_err("%s: could not set level to %s\n",
 				devname, c);
-			if (err == EBUSY &&
-			    (array.state & (1<<MD_SB_BITMAP_PRESENT)))
-				cont_err("Bitmap must be removed before level can be changed\n");
 			return err;
 		}

Quoted 2 lines

if (verbose >= 0)
--

You appear to have identified a problem that needs fixing, but the patch is fairly obviously wrong.

You've removed a test and error message that could apply to any level change, and added a test and error message that only applies to some specific level changes.
Why does that error message no longer apply to all the other possible level changes?

The test that you have added is mostly OK ... but you missed something. Look around and similar nearby code.
The test you removed certainly should stay.

NeilBrown

Quoted 1 line

Thanks for printing out the mistake. I checked the nearby code and found I missed close the cfd. Is this I missed? But I'm wondering that it doesn't close it in the following code. Now it's just closed when it's failed to reshape raid1/raid10 to raid0.

 Grow.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Grow.c +9 -0

index 80d7b22..d711884 100644
@@ -1898,6 +1898,15 @@ size_change_error:
             array.layout == ((1 << 8) + 2) && !(array.raid_disks & 1)) ||
            (s->level == 0 && array.level == 1 && sra)) {
                int err;
+               
+               if (array.state & (1<<MD_SB_BITMAP_PRESENT)) {
+                       dprintf("Bitmap must be removed before level can be changed\n");
+                       if (cfd > -1)
+                               close(cfd);
+                       rv = 1;
+                       goto release;
+               }
+
                err = remove_disks_for_takeover(st, sra, array.layout);
 

Best Regards
Xiao

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: "Neil Brown" <[email protected]>
To: "Xiao Ni" <[email protected]>
Cc: "Jes Sorensen" <[email protected]>, [email protected]
Sent: Tuesday, October 27, 2015 7:10:17 AM
Subject: Re: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

Quoted 6 lines · Xiao Ni

Quoted 1 line

Thanks for printing out the mistake. I checked the nearby code and found I missed close the cfd. Is this I missed? But I'm wondering that it doesn't close it in the following code. Now it's just closed when it's failed to reshape raid1/raid10 to raid0.

That is a very sensible question to ask - thanks.
After a quick look - it does seem very strange. That handling of 'cfd' and 'fd' seems quite odd.

If you would like to work out what should happen and find a simple way to fix the code, that would be great. But I don't insist.

The patch below is quite acceptable as it is a simple solution to a simple problem and appear consistent with nearby code. So if you gave it a proper description, formatted it properly and posted it in a way that didn't mess up all the white space, I would very likely accept it.

However:
- maybe we should just remove the bitmap rather than complain if we find one. After all, we are removing other things (extra devices). A raid0 can never had a bitmap, so when converting to a raid0, it does make sense to remove the bitmap.

- The test you used only checks for an internal bitmap, not an external one. External bitmaps are hardly ever used except by people who know exactly what they are doing, so I'm not too fussed about handling them perfectly, but testing for and removing an external bitmap might make sense.

Thanks,
NeilBrown