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

[NeilBrown <nfbrown@novell.com>]

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