Re: [PATCH] kill-subarray: fix, IMSM cannot kill-subarray with unsupported metadata

[NeilBrown <neilb@suse.de>]

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

On Mon, 31 Oct 2011 16:52:22 +0000 "Labun, Marcin" <Marcin.Labun@intel.com>
wrote:

> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Monday, October 31, 2011 1:32 AM
> > To: Labun, Marcin
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J
> > Subject: Re: [PATCH] kill-subarray: fix, IMSM cannot kill-subarray with
> > unsupported metadata
> > 
> > On Fri, 28 Oct 2011 14:46:57 +0000 "Labun, Marcin"
> > <Marcin.Labun@intel.com>
> > wrote:
> > 
> > > Hi Neil,
> > > Please review modified patch in response for your comments.
> > > I have introduced two flags, one for activation blocking, second for
> > > container wide reshapes
> > > Blocking:
> > > - some arrays in container are blocked, the others can be activated
> > > and reshaped,
> > > - some arrays are blocked, others can be activated but container wide
> > reshaped is blocked for all.
> > >
> > > Thanks,
> > > Marcin Labun
> > 
> > Thanks.  This looks mostly OK.
> > I have applied it -  with a number of formatting improvements and the
> > removal for a debugging printf :-)
> 
> Thanks!
> It seems that map_lock call appeared from *nowhere* in the modified patch :). Please see the context below.

Thanks for checking and noticing that.
It was a merge error on my part.  As your patch added a map_unlock call I
reasoned that the map_lock call which you patch left unchanged needed to be
added back - I obviously wasn't thinking clearly.
I have removed it and the map_unlock that your patch added.


> 
> -	/* do not assemble arrays that might have bad blocks */
> -	if (list && list->array.state & (1<<MD_SB_BBM_ERRORS)) {
> -		fprintf(stderr, Name ": BBM log found in metadata. "
> -					"Cannot activate array(s).\n");
> -		/* free container data and exit */
> -		sysfs_free(list);
> -		return 2;
> -	}
> -
> +	/* when nothing to activate - quit */
> +	if (list == NULL)
> +		return 0;
> +	if (map_lock(&map))
> +		fprintf(stderr, Name ": failed to get exclusive lock on "
> +			"mapfile\n");
> 
> > 
> > However this bit looks wrong:
> > 
> > > +/*
> > > + * helper routine to check metadata reshape avalability
> > > + * 1. Do not "grow" arrays with volume activation blocked
> > > + * 2. do not reshape containers with container reshape blocked
> > > + *
> > > + * IN:
> > > + *	subarray - array name or NULL for container wide reshape
> > > + *	content - md device info from container_content
> > > + * OUT:
> > > + *	0 - block reshape
> > > + */
> > > +static int check_reshape(char *subarray, struct mdinfo *content) {
> > > +	char *ep;
> > > +	unsigned int idx;
> > > +	printf("subarray: %s\n", subarray);
> > > +
> > > +
> > > +	if (!subarray) {
> > > +		if (content->array.state &
> > (1<<MD_SB_BLOCK_CONTAINER_RESHAPE))
> > > +			return 0;
> > > +	}
> > > +	else {
> > > +		/* do not "grow" arrays with volume activation blocked */
> > > +		idx = strtoul(subarray, &ep, 10);
> > > +		if ((*ep == '\0') && (content->container_member == (int)
> > idx) &&
> > > +		    (content->array.state & (1<<MD_SB_BLOCK_VOLUME)))
> > > +			return 0;
> > > +	}
> > > +
> > > +	return 1;
> > > +}
> > 
> > Grow.c shouldn't be doing a 'strtoul' here.  The subarray string
> > belongs to the metadata handler, mdadm core code shouldn't interpret it
> > at all.
> > 
> > What exactly is the point of that bit of code?
> > 
> > Thanks,
> > NeilBrown
> Grow.c generates subarray string in super_by_fd() function.Derived from (struct mdinfo) sra->text_version.
> 
> In check_reshape function I want to know on which subarray grow operates to get subarray blocking bits.

I think I see - thanks.

I have changed the code to do what I think it should.  Please review patch
below.

Thanks,
NeilBrown


commit 446894ea8db17a2dfc740f81d58190c5ac8167d5
Author: NeilBrown <neilb@suse.de>
Date:   Tue Nov 1 15:45:46 2011 +1100

    Grow: fix check_reshape and open_code it.
    
    check_reshape should not try to parse the subarray string - only
    metadata handlers are allowed to do that.
    
    The common code and only interpret a subarray string by passing it to
    "container_content" which will then return only the member for that
    subarray.
    
    So remove check_reshape and place similar logic explicitly at the two
    call-sites.  They are different enough that it is probably clearer to
    have explicit code.
    
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Grow.c b/Grow.c
index 598fd59..8df4ac4 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1358,36 +1358,6 @@ static int reshape_container(char *container, char *devname,
 			     char *backup_file,
 			     int quiet, int restart, int freeze_reshape);
 
-/*
- * helper routine to check metadata reshape avalability
- * 1. Do not "grow" arrays with volume activation blocked
- * 2. do not reshape containers with container reshape blocked
- *
- * IN:
- *	subarray - array name or NULL for container wide reshape
- *	content - md device info from container_content
- * OUT:
- *	0 - block reshape
- */
-static int check_reshape(char *subarray, struct mdinfo *content)
-{
-	char *ep;
-	unsigned int idx;
-
-	if (!subarray) {
-		if (content->array.state & (1<<MD_SB_BLOCK_CONTAINER_RESHAPE))
-			return 0;
-	} else {
-		/* do not "grow" arrays with volume activation blocked */
-		idx = strtoul(subarray, &ep, 10);
-		if (*ep == '\0'
-		    && content->container_member == (int) idx
-		    && (content->array.state & (1<<MD_SB_BLOCK_VOLUME)))
-			return 0;
-	}
-	return 1;
-}
-
 int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
 		 long long size,
 		 int level, char *layout_str, int chunksize, int raid_disks,
@@ -1505,12 +1475,16 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
 
 			cc = st->ss->container_content(st, subarray);
 			for (content = cc; content ; content = content->next) {
-				int allow_reshape;
+				int allow_reshape = 1;
 
 				/* check if reshape is allowed based on metadata
 				 * indications stored in content.array.status
 				 */
-				allow_reshape = check_reshape(subarray, content);
+				if (content->array.state & (1<<MD_SB_BLOCK_VOLUME))
+					allow_reshape = 0;
+				if (content->array.state
+				    & (1<<MD_SB_BLOCK_CONTAINER_RESHAPE))
+					allow_reshape = 0;
 				if (!allow_reshape) {
 					fprintf(stderr, Name
 						" cannot reshape arrays in"
@@ -3788,10 +3762,10 @@ int Grow_continue_command(char *devname, int fd,
 			goto Grow_continue_command_exit;
 		}
 
-		cc = st->ss->container_content(st, NULL);
+		cc = st->ss->container_content(st, subarray);
 		for (content = cc; content ; content = content->next) {
 			char *array;
-			int allow_reshape;
+			int allow_reshape = 1;
 
 			if (content->reshape_active == 0)
 				continue;
@@ -3800,10 +3774,14 @@ int Grow_continue_command(char *devname, int fd,
 			 * content->reshape_active state, therefore we
 			 * need to check_reshape based on
 			 * reshape_active and subarray name
-			*/
-			allow_reshape =
-			  check_reshape((content->reshape_active == CONTAINER_RESHAPE)? NULL : subarray,
-					content);
+			 */
+			if (content->array.state & (1<<MD_SB_BLOCK_VOLUME))
+				allow_reshape = 0;
+			if (content->reshape_active == CONTAINER_RESHAPE &&
+			    (content->array.state
+			     & (1<<MD_SB_BLOCK_CONTAINER_RESHAPE)))
+				allow_reshape = 0;
+
 			if (!allow_reshape) {
 				fprintf(stderr, Name
 					": cannot continue reshape of an array"
diff --git a/super-intel.c b/super-intel.c
index 1caee70..34a4b34 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5759,8 +5759,8 @@ static struct mdinfo *container_content_imsm(struct supertype *st, char *subarra
 						 map->num_members, /* raid disks */
 						 &chunk,
 						 1 /* verbose */)) {
-			fprintf(stderr, Name ": IMSM RAID gemetry validation failed. "
-				"Array %s activation is blocked.\n",
+			fprintf(stderr, Name ": IMSM RAID geometry validation"
+				" failed.  Array %s activation is blocked.\n",
 				dev->volume);
 			this->array.state |=
 			  (1<<MD_SB_BLOCK_CONTAINER_RESHAPE) |

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