[PATCH] mdadm: check bitmap first when reshape raid1 to raid0

[PATCH] mdadm: check bitmap first when reshape raid1 to raid0

[Xiao Ni]

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.

And need to free subarray, close cfd in Grow_reshape.

It can't remove bitmap in Grow_reshape, because it's already frozen. I
think it should not unfreeze in the process of the function. So I just
test the bitmap.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c  | 45 ++++++++++++++++++++++++++++-----------------
 mdadm.c | 16 ++++++++++++----
 2 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/Grow.c b/Grow.c
index 80d7b22..c4ea2a5 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1600,15 +1600,14 @@ int Grow_reshape(char *devname, int fd,
 			cfd = open_dev_excl(st->container_devnm);
 		} else {
 			container = st->devnm;
-			close(fd);
 			cfd = open_dev_excl(st->devnm);
 			fd = cfd;
 		}
 		if (cfd < 0) {
 			pr_err("Unable to open container for %s\n",
 				devname);
-			free(subarray);
-			return 1;
+			rv = 1;
+			goto release;
 		}
 
 		rv = st->ss->load_container(st, cfd, NULL);
@@ -1616,8 +1615,8 @@ int Grow_reshape(char *devname, int fd,
 		if (rv) {
 			pr_err("Cannot read superblock for %s\n",
 				devname);
-			free(subarray);
-			return 1;
+			rv = 1;
+			goto release;
 		}
 
 		/* check if operation is supported for metadata handler */
@@ -1641,8 +1640,8 @@ int Grow_reshape(char *devname, int fd,
 					pr_err("cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
 					       devname, container);
 					sysfs_free(cc);
-					free(subarray);
-					return 1;
+					rv = 1;
+					goto release;
 				}
 			}
 			sysfs_free(cc);
@@ -1662,7 +1661,8 @@ int Grow_reshape(char *devname, int fd,
 		       s->raiddisks - array.raid_disks,
 		       s->raiddisks - array.raid_disks == 1 ? "" : "s",
 		       array.spare_disks + added_disks);
-		return 1;
+		rv = 1;
+		goto release;
 	}
 
 	sra = sysfs_read(fd, NULL, GET_LEVEL | GET_DISKS | GET_DEVS
@@ -1675,17 +1675,18 @@ int Grow_reshape(char *devname, int fd,
 	} else {
 		pr_err("failed to read sysfs parameters for %s\n",
 			devname);
-		return 1;
+		rv = 1;
+		goto release;
 	}
 	frozen = freeze(st);
 	if (frozen < -1) {
 		/* freeze() already spewed the reason */
-		sysfs_free(sra);
-		return 1;
+		rv = 1;
+		goto release;
 	} else if (frozen < 0) {
 		pr_err("%s is performing resync/recovery and cannot be reshaped\n", devname);
-		sysfs_free(sra);
-		return 1;
+		rv = 1;
+		goto release;
 	}
 
 	/* ========= set size =============== */
@@ -1898,11 +1899,16 @@ 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)) {
+		        pr_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");
-			if (cfd > -1)
-				close(cfd);
+			pr_err("Array cannot be reshaped\n");
 			rv = 1;
 			goto release;
 		}
@@ -2078,9 +2084,14 @@ size_change_error:
 		frozen = 0;
 	}
 release:
-	sysfs_free(sra);
 	if (frozen > 0)
 		unfreeze(st);
+	if (sra)
+		sysfs_free(sra);
+	if (cfd > -1)
+		close(cfd);
+	if (subarray)
+		free(subarray);
 	return rv;
 }
 
diff --git a/mdadm.c b/mdadm.c
index f56a8cf..95db2a0 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1299,7 +1299,8 @@ int main(int argc, char *argv[])
 			pr_err("'1' is an unusual number of drives for an array, so it is probably\n"
 				"     a mistake.  If you really mean it you will need to specify --force before\n"
 				"     setting the number of drives.\n");
-			exit(2);
+			rv = 2;
+			goto release;
 		}
 	}
 
@@ -1326,13 +1327,15 @@ int main(int argc, char *argv[])
 			rv = get_cluster_name(&c.homecluster);
 		if (rv) {
 			pr_err("The md can't get cluster name\n");
-			exit(1);
+			rv = 1;
+			goto release;
 		}
 	}
 
 	if (c.backup_file && data_offset != INVALID_SECTORS) {
 		pr_err("--backup-file and --data-offset are incompatible\n");
-		exit(2);
+		rv = 2;
+		goto release;
 	}
 
 	if ((mode == MISC && devmode == 'E')
@@ -1340,7 +1343,8 @@ int main(int argc, char *argv[])
 		/* Anyone may try this */;
 	else if (geteuid() != 0) {
 		pr_err("must be super-user to perform this action\n");
-		exit(1);
+		rv = 1;
+		goto release;
 	}
 
 	ident.autof = c.autof;
@@ -1640,6 +1644,10 @@ int main(int argc, char *argv[])
 		autodetect();
 		break;
 	}
+
+release:
+	if (mdfd > -1)
+		close(mdfd);
 	exit(rv);
 }
 
-- 
1.8.3.1

Re: [PATCH] mdadm: check bitmap first when reshape raid1 to raid0

[Xiao Ni]

Hi Neil

Could you review this patch? For the bitmap part, I don't remove it in 
Grow_reshape.
I described the reason in the patch.

Regards
Xiao

On 11/03/2015 08:25 AM, Xiao Ni wrote:
> 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.
>
> And need to free subarray, close cfd in Grow_reshape.
>
> It can't remove bitmap in Grow_reshape, because it's already frozen. I
> think it should not unfreeze in the process of the function. So I just
> test the bitmap.
>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>   Grow.c  | 45 ++++++++++++++++++++++++++++-----------------
>   mdadm.c | 16 ++++++++++++----
>   2 files changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 80d7b22..c4ea2a5 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1600,15 +1600,14 @@ int Grow_reshape(char *devname, int fd,
>   			cfd = open_dev_excl(st->container_devnm);
>   		} else {
>   			container = st->devnm;
> -			close(fd);
>   			cfd = open_dev_excl(st->devnm);
>   			fd = cfd;
>   		}
>   		if (cfd < 0) {
>   			pr_err("Unable to open container for %s\n",
>   				devname);
> -			free(subarray);
> -			return 1;
> +			rv = 1;
> +			goto release;
>   		}
>   
>   		rv = st->ss->load_container(st, cfd, NULL);
> @@ -1616,8 +1615,8 @@ int Grow_reshape(char *devname, int fd,
>   		if (rv) {
>   			pr_err("Cannot read superblock for %s\n",
>   				devname);
> -			free(subarray);
> -			return 1;
> +			rv = 1;
> +			goto release;
>   		}
>   
>   		/* check if operation is supported for metadata handler */
> @@ -1641,8 +1640,8 @@ int Grow_reshape(char *devname, int fd,
>   					pr_err("cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
>   					       devname, container);
>   					sysfs_free(cc);
> -					free(subarray);
> -					return 1;
> +					rv = 1;
> +					goto release;
>   				}
>   			}
>   			sysfs_free(cc);
> @@ -1662,7 +1661,8 @@ int Grow_reshape(char *devname, int fd,
>   		       s->raiddisks - array.raid_disks,
>   		       s->raiddisks - array.raid_disks == 1 ? "" : "s",
>   		       array.spare_disks + added_disks);
> -		return 1;
> +		rv = 1;
> +		goto release;
>   	}
>   
>   	sra = sysfs_read(fd, NULL, GET_LEVEL | GET_DISKS | GET_DEVS
> @@ -1675,17 +1675,18 @@ int Grow_reshape(char *devname, int fd,
>   	} else {
>   		pr_err("failed to read sysfs parameters for %s\n",
>   			devname);
> -		return 1;
> +		rv = 1;
> +		goto release;
>   	}
>   	frozen = freeze(st);
>   	if (frozen < -1) {
>   		/* freeze() already spewed the reason */
> -		sysfs_free(sra);
> -		return 1;
> +		rv = 1;
> +		goto release;
>   	} else if (frozen < 0) {
>   		pr_err("%s is performing resync/recovery and cannot be reshaped\n", devname);
> -		sysfs_free(sra);
> -		return 1;
> +		rv = 1;
> +		goto release;
>   	}
>   
>   	/* ========= set size =============== */
> @@ -1898,11 +1899,16 @@ 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)) {
> +		        pr_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");
> -			if (cfd > -1)
> -				close(cfd);
> +			pr_err("Array cannot be reshaped\n");
>   			rv = 1;
>   			goto release;
>   		}
> @@ -2078,9 +2084,14 @@ size_change_error:
>   		frozen = 0;
>   	}
>   release:
> -	sysfs_free(sra);
>   	if (frozen > 0)
>   		unfreeze(st);
> +	if (sra)
> +		sysfs_free(sra);
> +	if (cfd > -1)
> +		close(cfd);
> +	if (subarray)
> +		free(subarray);
>   	return rv;
>   }
>   
> diff --git a/mdadm.c b/mdadm.c
> index f56a8cf..95db2a0 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1299,7 +1299,8 @@ int main(int argc, char *argv[])
>   			pr_err("'1' is an unusual number of drives for an array, so it is probably\n"
>   				"     a mistake.  If you really mean it you will need to specify --force before\n"
>   				"     setting the number of drives.\n");
> -			exit(2);
> +			rv = 2;
> +			goto release;
>   		}
>   	}
>   
> @@ -1326,13 +1327,15 @@ int main(int argc, char *argv[])
>   			rv = get_cluster_name(&c.homecluster);
>   		if (rv) {
>   			pr_err("The md can't get cluster name\n");
> -			exit(1);
> +			rv = 1;
> +			goto release;
>   		}
>   	}
>   
>   	if (c.backup_file && data_offset != INVALID_SECTORS) {
>   		pr_err("--backup-file and --data-offset are incompatible\n");
> -		exit(2);
> +		rv = 2;
> +		goto release;
>   	}
>   
>   	if ((mode == MISC && devmode == 'E')
> @@ -1340,7 +1343,8 @@ int main(int argc, char *argv[])
>   		/* Anyone may try this */;
>   	else if (geteuid() != 0) {
>   		pr_err("must be super-user to perform this action\n");
> -		exit(1);
> +		rv = 1;
> +		goto release;
>   	}
>   
>   	ident.autof = c.autof;
> @@ -1640,6 +1644,10 @@ int main(int argc, char *argv[])
>   		autodetect();
>   		break;
>   	}
> +
> +release:
> +	if (mdfd > -1)
> +		close(mdfd);
>   	exit(rv);
>   }
>   

Re: [PATCH] mdadm: check bitmap first when reshape raid1 to raid0

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6183 bytes --]

On Tue, Nov 03 2015, Xiao Ni wrote:

> 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.
>
> And need to free subarray, close cfd in Grow_reshape.
>
> It can't remove bitmap in Grow_reshape, because it's already frozen. I
> think it should not unfreeze in the process of the function. So I just
> test the bitmap.

I don't find this acceptable, sorry.
If mdadm it asked the change the level from raid1 to raid0 it should do
that, unless it is dangerous or impossible.
Removing a bitmap is neither dangerous nor impossible.  To just do it.

I would probably put a test in before freezing the array.
 if array has a bitmap and a request was made to change the level to
 raid0, then remove the bitmap.

It that so hard?

Also you patch does multiple different things, some of which weren't
explained at all in the comment above.

In particlar, the change to mdadm.c is completely pointless.
It seems to just be adding a 'close' call before 'exit'.
As 'exit' implicitly closes all file descriptors there is nothing to be
gained by closing anything immediately before calling exit.

Thanks,
NeilBrown


>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c  | 45 ++++++++++++++++++++++++++++-----------------
>  mdadm.c | 16 ++++++++++++----
>  2 files changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 80d7b22..c4ea2a5 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1600,15 +1600,14 @@ int Grow_reshape(char *devname, int fd,
>  			cfd = open_dev_excl(st->container_devnm);
>  		} else {
>  			container = st->devnm;
> -			close(fd);
>  			cfd = open_dev_excl(st->devnm);
>  			fd = cfd;
>  		}
>  		if (cfd < 0) {
>  			pr_err("Unable to open container for %s\n",
>  				devname);
> -			free(subarray);
> -			return 1;
> +			rv = 1;
> +			goto release;
>  		}
>  
>  		rv = st->ss->load_container(st, cfd, NULL);
> @@ -1616,8 +1615,8 @@ int Grow_reshape(char *devname, int fd,
>  		if (rv) {
>  			pr_err("Cannot read superblock for %s\n",
>  				devname);
> -			free(subarray);
> -			return 1;
> +			rv = 1;
> +			goto release;
>  		}
>  
>  		/* check if operation is supported for metadata handler */
> @@ -1641,8 +1640,8 @@ int Grow_reshape(char *devname, int fd,
>  					pr_err("cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
>  					       devname, container);
>  					sysfs_free(cc);
> -					free(subarray);
> -					return 1;
> +					rv = 1;
> +					goto release;
>  				}
>  			}
>  			sysfs_free(cc);
> @@ -1662,7 +1661,8 @@ int Grow_reshape(char *devname, int fd,
>  		       s->raiddisks - array.raid_disks,
>  		       s->raiddisks - array.raid_disks == 1 ? "" : "s",
>  		       array.spare_disks + added_disks);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	}
>  
>  	sra = sysfs_read(fd, NULL, GET_LEVEL | GET_DISKS | GET_DEVS
> @@ -1675,17 +1675,18 @@ int Grow_reshape(char *devname, int fd,
>  	} else {
>  		pr_err("failed to read sysfs parameters for %s\n",
>  			devname);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	}
>  	frozen = freeze(st);
>  	if (frozen < -1) {
>  		/* freeze() already spewed the reason */
> -		sysfs_free(sra);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	} else if (frozen < 0) {
>  		pr_err("%s is performing resync/recovery and cannot be reshaped\n", devname);
> -		sysfs_free(sra);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	}
>  
>  	/* ========= set size =============== */
> @@ -1898,11 +1899,16 @@ 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)) {
> +		        pr_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");
> -			if (cfd > -1)
> -				close(cfd);
> +			pr_err("Array cannot be reshaped\n");
>  			rv = 1;
>  			goto release;
>  		}
> @@ -2078,9 +2084,14 @@ size_change_error:
>  		frozen = 0;
>  	}
>  release:
> -	sysfs_free(sra);
>  	if (frozen > 0)
>  		unfreeze(st);
> +	if (sra)
> +		sysfs_free(sra);
> +	if (cfd > -1)
> +		close(cfd);
> +	if (subarray)
> +		free(subarray);
>  	return rv;
>  }
>  
> diff --git a/mdadm.c b/mdadm.c
> index f56a8cf..95db2a0 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1299,7 +1299,8 @@ int main(int argc, char *argv[])
>  			pr_err("'1' is an unusual number of drives for an array, so it is probably\n"
>  				"     a mistake.  If you really mean it you will need to specify --force before\n"
>  				"     setting the number of drives.\n");
> -			exit(2);
> +			rv = 2;
> +			goto release;
>  		}
>  	}
>  
> @@ -1326,13 +1327,15 @@ int main(int argc, char *argv[])
>  			rv = get_cluster_name(&c.homecluster);
>  		if (rv) {
>  			pr_err("The md can't get cluster name\n");
> -			exit(1);
> +			rv = 1;
> +			goto release;
>  		}
>  	}
>  
>  	if (c.backup_file && data_offset != INVALID_SECTORS) {
>  		pr_err("--backup-file and --data-offset are incompatible\n");
> -		exit(2);
> +		rv = 2;
> +		goto release;
>  	}
>  
>  	if ((mode == MISC && devmode == 'E')
> @@ -1340,7 +1343,8 @@ int main(int argc, char *argv[])
>  		/* Anyone may try this */;
>  	else if (geteuid() != 0) {
>  		pr_err("must be super-user to perform this action\n");
> -		exit(1);
> +		rv = 1;
> +		goto release;
>  	}
>  
>  	ident.autof = c.autof;
> @@ -1640,6 +1644,10 @@ int main(int argc, char *argv[])
>  		autodetect();
>  		break;
>  	}
> +
> +release:
> +	if (mdfd > -1)
> +		close(mdfd);
>  	exit(rv);
>  }
>  
> -- 
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] mdadm: check bitmap first when reshape raid1 to raid0

[Xiao Ni]

On 12/21/2015 10:47 AM, NeilBrown wrote:
> On Tue, Nov 03 2015, Xiao Ni wrote:
>
>> 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.
>>
>> And need to free subarray, close cfd in Grow_reshape.
>>
>> It can't remove bitmap in Grow_reshape, because it's already frozen. I
>> think it should not unfreeze in the process of the function. So I just
>> test the bitmap.
> I don't find this acceptable, sorry.
> If mdadm it asked the change the level from raid1 to raid0 it should do
> that, unless it is dangerous or impossible.
> Removing a bitmap is neither dangerous nor impossible.  To just do it.
>
> I would probably put a test in before freezing the array.
>   if array has a bitmap and a request was made to change the level to
>   raid0, then remove the bitmap.
>
> It that so hard?

I focused my attention on the codes nearby the place where I modified.
It's easy to do this and I'll re-send it.
>
> Also you patch does multiple different things, some of which weren't
> explained at all in the comment above.

I'll give more comments for them.
>
> In particlar, the change to mdadm.c is completely pointless.
> It seems to just be adding a 'close' call before 'exit'.
> As 'exit' implicitly closes all file descriptors there is nothing to be
> gained by closing anything immediately before calling exit.

Hmm, I know this. I just thought it maybe better to call close in pairs
with open. Yes, as you said, it's really pointless.
>
> Thanks,
> NeilBrown
>
>
>> Signed-off-by: Xiao Ni <xni@redhat.com>
>> ---
>>   Grow.c  | 45 ++++++++++++++++++++++++++++-----------------
>>   mdadm.c | 16 ++++++++++++----
>>   2 files changed, 40 insertions(+), 21 deletions(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index 80d7b22..c4ea2a5 100644
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -1600,15 +1600,14 @@ int Grow_reshape(char *devname, int fd,
>>   			cfd = open_dev_excl(st->container_devnm);
>>   		} else {
>>   			container = st->devnm;
>> -			close(fd);
>>   			cfd = open_dev_excl(st->devnm);
>>   			fd = cfd;
>>   		}
>>   		if (cfd < 0) {
>>   			pr_err("Unable to open container for %s\n",
>>   				devname);
>> -			free(subarray);
>> -			return 1;
>> +			rv = 1;
>> +			goto release;
>>   		}
>>   
>>   		rv = st->ss->load_container(st, cfd, NULL);
>> @@ -1616,8 +1615,8 @@ int Grow_reshape(char *devname, int fd,
>>   		if (rv) {
>>   			pr_err("Cannot read superblock for %s\n",
>>   				devname);
>> -			free(subarray);
>> -			return 1;
>> +			rv = 1;
>> +			goto release;
>>   		}
>>   
>>   		/* check if operation is supported for metadata handler */
>> @@ -1641,8 +1640,8 @@ int Grow_reshape(char *devname, int fd,
>>   					pr_err("cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
>>   					       devname, container);
>>   					sysfs_free(cc);
>> -					free(subarray);
>> -					return 1;
>> +					rv = 1;
>> +					goto release;
>>   				}
>>   			}
>>   			sysfs_free(cc);
>> @@ -1662,7 +1661,8 @@ int Grow_reshape(char *devname, int fd,
>>   		       s->raiddisks - array.raid_disks,
>>   		       s->raiddisks - array.raid_disks == 1 ? "" : "s",
>>   		       array.spare_disks + added_disks);
>> -		return 1;
>> +		rv = 1;
>> +		goto release;
>>   	}
>>   
>>   	sra = sysfs_read(fd, NULL, GET_LEVEL | GET_DISKS | GET_DEVS
>> @@ -1675,17 +1675,18 @@ int Grow_reshape(char *devname, int fd,
>>   	} else {
>>   		pr_err("failed to read sysfs parameters for %s\n",
>>   			devname);
>> -		return 1;
>> +		rv = 1;
>> +		goto release;
>>   	}
>>   	frozen = freeze(st);
>>   	if (frozen < -1) {
>>   		/* freeze() already spewed the reason */
>> -		sysfs_free(sra);
>> -		return 1;
>> +		rv = 1;
>> +		goto release;
>>   	} else if (frozen < 0) {
>>   		pr_err("%s is performing resync/recovery and cannot be reshaped\n", devname);
>> -		sysfs_free(sra);
>> -		return 1;
>> +		rv = 1;
>> +		goto release;
>>   	}
>>   
>>   	/* ========= set size =============== */
>> @@ -1898,11 +1899,16 @@ 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)) {
>> +		        pr_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");
>> -			if (cfd > -1)
>> -				close(cfd);
>> +			pr_err("Array cannot be reshaped\n");
>>   			rv = 1;
>>   			goto release;
>>   		}
>> @@ -2078,9 +2084,14 @@ size_change_error:
>>   		frozen = 0;
>>   	}
>>   release:
>> -	sysfs_free(sra);
>>   	if (frozen > 0)
>>   		unfreeze(st);
>> +	if (sra)
>> +		sysfs_free(sra);
>> +	if (cfd > -1)
>> +		close(cfd);
>> +	if (subarray)
>> +		free(subarray);
>>   	return rv;
>>   }
>>   
>> diff --git a/mdadm.c b/mdadm.c
>> index f56a8cf..95db2a0 100644
>> --- a/mdadm.c
>> +++ b/mdadm.c
>> @@ -1299,7 +1299,8 @@ int main(int argc, char *argv[])
>>   			pr_err("'1' is an unusual number of drives for an array, so it is probably\n"
>>   				"     a mistake.  If you really mean it you will need to specify --force before\n"
>>   				"     setting the number of drives.\n");
>> -			exit(2);
>> +			rv = 2;
>> +			goto release;
>>   		}
>>   	}
>>   
>> @@ -1326,13 +1327,15 @@ int main(int argc, char *argv[])
>>   			rv = get_cluster_name(&c.homecluster);
>>   		if (rv) {
>>   			pr_err("The md can't get cluster name\n");
>> -			exit(1);
>> +			rv = 1;
>> +			goto release;
>>   		}
>>   	}
>>   
>>   	if (c.backup_file && data_offset != INVALID_SECTORS) {
>>   		pr_err("--backup-file and --data-offset are incompatible\n");
>> -		exit(2);
>> +		rv = 2;
>> +		goto release;
>>   	}
>>   
>>   	if ((mode == MISC && devmode == 'E')
>> @@ -1340,7 +1343,8 @@ int main(int argc, char *argv[])
>>   		/* Anyone may try this */;
>>   	else if (geteuid() != 0) {
>>   		pr_err("must be super-user to perform this action\n");
>> -		exit(1);
>> +		rv = 1;
>> +		goto release;
>>   	}
>>   
>>   	ident.autof = c.autof;
>> @@ -1640,6 +1644,10 @@ int main(int argc, char *argv[])
>>   		autodetect();
>>   		break;
>>   	}
>> +
>> +release:
>> +	if (mdfd > -1)
>> +		close(mdfd);
>>   	exit(rv);
>>   }
>>   
>> -- 
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mdadm: check bitmap first when reshape raid1 to raid0

[Xiao Ni]

Sorry, the patch is wrong. Please ignore this and I'll send another.

----- Original Message -----
> From: "Xiao Ni" <xni@redhat.com>
> To: linux-raid@vger.kernel.org
> Sent: Monday, November 2, 2015 11:18:27 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.
> 
> And need to free subarray, close cfd in Grow_reshape.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c  | 38 ++++++++++++++++++++------------------
>  mdadm.c | 16 ++++++++++++----
>  2 files changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index d711884..51ed0c0 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1600,15 +1600,14 @@ int Grow_reshape(char *devname, int fd,
>  			cfd = open_dev_excl(st->container_devnm);
>  		} else {
>  			container = st->devnm;
> -			close(fd);
>  			cfd = open_dev_excl(st->devnm);
>  			fd = cfd;
>  		}
>  		if (cfd < 0) {
>  			pr_err("Unable to open container for %s\n",
>  				devname);
> -			free(subarray);
> -			return 1;
> +			rv = 1;
> +			goto release;
>  		}
>  
>  		rv = st->ss->load_container(st, cfd, NULL);
> @@ -1616,8 +1615,8 @@ int Grow_reshape(char *devname, int fd,
>  		if (rv) {
>  			pr_err("Cannot read superblock for %s\n",
>  				devname);
> -			free(subarray);
> -			return 1;
> +			rv = 1;
> +			goto release;
>  		}
>  
>  		/* check if operation is supported for metadata handler */
> @@ -1641,8 +1640,8 @@ int Grow_reshape(char *devname, int fd,
>  					pr_err("cannot reshape arrays in container with unsupported metadata:
>  					%s(%s)\n",
>  					       devname, container);
>  					sysfs_free(cc);
> -					free(subarray);
> -					return 1;
> +					rv = 1;
> +					goto release;
>  				}
>  			}
>  			sysfs_free(cc);
> @@ -1662,7 +1661,8 @@ int Grow_reshape(char *devname, int fd,
>  		       s->raiddisks - array.raid_disks,
>  		       s->raiddisks - array.raid_disks == 1 ? "" : "s",
>  		       array.spare_disks + added_disks);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	}
>  
>  	sra = sysfs_read(fd, NULL, GET_LEVEL | GET_DISKS | GET_DEVS
> @@ -1675,17 +1675,18 @@ int Grow_reshape(char *devname, int fd,
>  	} else {
>  		pr_err("failed to read sysfs parameters for %s\n",
>  			devname);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	}
>  	frozen = freeze(st);
>  	if (frozen < -1) {
>  		/* freeze() already spewed the reason */
> -		sysfs_free(sra);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	} else if (frozen < 0) {
>  		pr_err("%s is performing resync/recovery and cannot be reshaped\n",
>  		devname);
> -		sysfs_free(sra);
> -		return 1;
> +		rv = 1;
> +		goto release;
>  	}
>  
>  	/* ========= set size =============== */
> @@ -1901,8 +1902,6 @@ size_change_error:
>  		
>  		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;
>  		}
> @@ -1910,8 +1909,6 @@ size_change_error:
>  		err = remove_disks_for_takeover(st, sra, array.layout);
>  		if (err) {
>  			dprintf("Array cannot be reshaped\n");
> -			if (cfd > -1)
> -				close(cfd);
>  			rv = 1;
>  			goto release;
>  		}
> @@ -2087,9 +2084,14 @@ size_change_error:
>  		frozen = 0;
>  	}
>  release:
> -	sysfs_free(sra);
>  	if (frozen > 0)
>  		unfreeze(st);
> +	if (sra)
> +		sysfs_free(sra);
> +	if (cfd > -1)
> +		close(cfd);
> +	if (subarray)
> +		free(subarray);
>  	return rv;
>  }
>  
> diff --git a/mdadm.c b/mdadm.c
> index f56a8cf..95db2a0 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1299,7 +1299,8 @@ int main(int argc, char *argv[])
>  			pr_err("'1' is an unusual number of drives for an array, so it is
>  			probably\n"
>  				"     a mistake.  If you really mean it you will need to specify --force
>  				before\n"
>  				"     setting the number of drives.\n");
> -			exit(2);
> +			rv = 2;
> +			goto release;
>  		}
>  	}
>  
> @@ -1326,13 +1327,15 @@ int main(int argc, char *argv[])
>  			rv = get_cluster_name(&c.homecluster);
>  		if (rv) {
>  			pr_err("The md can't get cluster name\n");
> -			exit(1);
> +			rv = 1;
> +			goto release;
>  		}
>  	}
>  
>  	if (c.backup_file && data_offset != INVALID_SECTORS) {
>  		pr_err("--backup-file and --data-offset are incompatible\n");
> -		exit(2);
> +		rv = 2;
> +		goto release;
>  	}
>  
>  	if ((mode == MISC && devmode == 'E')
> @@ -1340,7 +1343,8 @@ int main(int argc, char *argv[])
>  		/* Anyone may try this */;
>  	else if (geteuid() != 0) {
>  		pr_err("must be super-user to perform this action\n");
> -		exit(1);
> +		rv = 1;
> +		goto release;
>  	}
>  
>  	ident.autof = c.autof;
> @@ -1640,6 +1644,10 @@ int main(int argc, char *argv[])
>  		autodetect();
>  		break;
>  	}
> +
> +release:
> +	if (mdfd > -1)
> +		close(mdfd);
>  	exit(rv);
>  }
>  
> --
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] mdadm: check bitmap first when reshape raid1 to raid0

[Xiao Ni]

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.

And need to free subarray, close cfd in Grow_reshape.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c  | 38 ++++++++++++++++++++------------------
 mdadm.c | 16 ++++++++++++----
 2 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/Grow.c b/Grow.c
index d711884..51ed0c0 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1600,15 +1600,14 @@ int Grow_reshape(char *devname, int fd,
 			cfd = open_dev_excl(st->container_devnm);
 		} else {
 			container = st->devnm;
-			close(fd);
 			cfd = open_dev_excl(st->devnm);
 			fd = cfd;
 		}
 		if (cfd < 0) {
 			pr_err("Unable to open container for %s\n",
 				devname);
-			free(subarray);
-			return 1;
+			rv = 1;
+			goto release;
 		}
 
 		rv = st->ss->load_container(st, cfd, NULL);
@@ -1616,8 +1615,8 @@ int Grow_reshape(char *devname, int fd,
 		if (rv) {
 			pr_err("Cannot read superblock for %s\n",
 				devname);
-			free(subarray);
-			return 1;
+			rv = 1;
+			goto release;
 		}
 
 		/* check if operation is supported for metadata handler */
@@ -1641,8 +1640,8 @@ int Grow_reshape(char *devname, int fd,
 					pr_err("cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
 					       devname, container);
 					sysfs_free(cc);
-					free(subarray);
-					return 1;
+					rv = 1;
+					goto release;
 				}
 			}
 			sysfs_free(cc);
@@ -1662,7 +1661,8 @@ int Grow_reshape(char *devname, int fd,
 		       s->raiddisks - array.raid_disks,
 		       s->raiddisks - array.raid_disks == 1 ? "" : "s",
 		       array.spare_disks + added_disks);
-		return 1;
+		rv = 1;
+		goto release;
 	}
 
 	sra = sysfs_read(fd, NULL, GET_LEVEL | GET_DISKS | GET_DEVS
@@ -1675,17 +1675,18 @@ int Grow_reshape(char *devname, int fd,
 	} else {
 		pr_err("failed to read sysfs parameters for %s\n",
 			devname);
-		return 1;
+		rv = 1;
+		goto release;
 	}
 	frozen = freeze(st);
 	if (frozen < -1) {
 		/* freeze() already spewed the reason */
-		sysfs_free(sra);
-		return 1;
+		rv = 1;
+		goto release;
 	} else if (frozen < 0) {
 		pr_err("%s is performing resync/recovery and cannot be reshaped\n", devname);
-		sysfs_free(sra);
-		return 1;
+		rv = 1;
+		goto release;
 	}
 
 	/* ========= set size =============== */
@@ -1901,8 +1902,6 @@ size_change_error:
 		
 		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;
 		}
@@ -1910,8 +1909,6 @@ size_change_error:
 		err = remove_disks_for_takeover(st, sra, array.layout);
 		if (err) {
 			dprintf("Array cannot be reshaped\n");
-			if (cfd > -1)
-				close(cfd);
 			rv = 1;
 			goto release;
 		}
@@ -2087,9 +2084,14 @@ size_change_error:
 		frozen = 0;
 	}
 release:
-	sysfs_free(sra);
 	if (frozen > 0)
 		unfreeze(st);
+	if (sra)
+		sysfs_free(sra);
+	if (cfd > -1)
+		close(cfd);
+	if (subarray)
+		free(subarray);
 	return rv;
 }
 
diff --git a/mdadm.c b/mdadm.c
index f56a8cf..95db2a0 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1299,7 +1299,8 @@ int main(int argc, char *argv[])
 			pr_err("'1' is an unusual number of drives for an array, so it is probably\n"
 				"     a mistake.  If you really mean it you will need to specify --force before\n"
 				"     setting the number of drives.\n");
-			exit(2);
+			rv = 2;
+			goto release;
 		}
 	}
 
@@ -1326,13 +1327,15 @@ int main(int argc, char *argv[])
 			rv = get_cluster_name(&c.homecluster);
 		if (rv) {
 			pr_err("The md can't get cluster name\n");
-			exit(1);
+			rv = 1;
+			goto release;
 		}
 	}
 
 	if (c.backup_file && data_offset != INVALID_SECTORS) {
 		pr_err("--backup-file and --data-offset are incompatible\n");
-		exit(2);
+		rv = 2;
+		goto release;
 	}
 
 	if ((mode == MISC && devmode == 'E')
@@ -1340,7 +1343,8 @@ int main(int argc, char *argv[])
 		/* Anyone may try this */;
 	else if (geteuid() != 0) {
 		pr_err("must be super-user to perform this action\n");
-		exit(1);
+		rv = 1;
+		goto release;
 	}
 
 	ident.autof = c.autof;
@@ -1640,6 +1644,10 @@ int main(int argc, char *argv[])
 		autodetect();
 		break;
 	}
+
+release:
+	if (mdfd > -1)
+		close(mdfd);
 	exit(rv);
 }
 
-- 
1.8.3.1

Re: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

[Xiao Ni]

----- Original Message -----
> From: "Neil Brown" <neilb@suse.de>
> To: "Xiao Ni" <xni@redhat.com>
> Cc: "Jes Sorensen" <jes.sorensen@redhat.com>, linux-raid@vger.kernel.org
> Sent: Tuesday, October 27, 2015 7:10:17 AM
> Subject: Re: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0
> 
> On Mon, Oct 26 2015, Xiao Ni wrote:
> 
> >> 
> >
> > 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.

I'll try it and re-send a patch then. 
> 
> 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.


Yes, :) it's better to remove the bitmap than check it. 

> 
>  - 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

As you said, I haven't used external bitmaps before. I'll check how to handle
them and add it to the patch too.

Best Regards
Xiao

Re: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

[Neil Brown]

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

On Mon, Oct 26 2015, Xiao Ni wrote:

>> 
>
> 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


>
>  Grow.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Grow.c b/Grow.c
> index 80d7b22..d711884 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

[Xiao Ni]

----- Original Message -----
> From: "Neil Brown" <neilb@suse.de>
> To: "Xiao Ni" <xni@redhat.com>
> Cc: "Jes Sorensen" <jes.sorensen@redhat.com>, linux-raid@vger.kernel.org
> Sent: Monday, October 26, 2015 4:21:36 AM
> Subject: Re: Fwd: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0
> 
> Xiao Ni <xni@redhat.com> writes:
> 
> > 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" <xni@redhat.com>
> > To: xni@redhat.com
> > 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 <xni@redhat.com>
> > ---
> >  Grow.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 80d7b22..5e9b0bb 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -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;
> >  		}
> >  		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
> 

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(+)

diff --git a/Grow.c b/Grow.c
index 80d7b22..d711884 100644
--- a/Grow.c
+++ b/Grow.c
@@ -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

Re: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

[Jes Sorensen]

Xiao Ni <xni@redhat.com> writes:
> 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.
> ---
>  Grow.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Xiao,

I believe you are also required to apply a Signed-off-by: line when
posting patches against mdadm.

Cheers,
Jes

>
> diff --git a/Grow.c b/Grow.c
> index 80d7b22..5e9b0bb 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -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;
>  		}
>  		if (verbose >= 0)