On Mon, 31 Oct 2011 16:52:22 +0000 "Labun, Marcin" <Marcin.Labun@xxxxxxxxx> wrote: > > > > -----Original Message----- > > From: NeilBrown [mailto:neilb@xxxxxxx] > > Sent: Monday, October 31, 2011 1:32 AM > > To: Labun, Marcin > > Cc: linux-raid@xxxxxxxxxxxxxxx; 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@xxxxxxxxx> > > 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@xxxxxxx> 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@xxxxxxx> 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:
signature.asc
Description: PGP signature
