[PATCH 0/3] Enable mdadm UT execution for imsm

[PATCH 0/3] Enable mdadm UT execution for imsm

[Adam Kwolek]

3 mdadm UT (08, 09, 15) are not working now. 
This series fixes those UT execution.

BR
Adam

---

Adam Kwolek (3):
      imsm: FIX: UT '08imsm-overlap' fails
      imsm: FIX: 'UT 09imsm-assemble' fails
      imsm: FIX: Chunk size migration is not possible


 Assemble.c    |    2 ++
 Incremental.c |    3 +++
 super-intel.c |   52 +++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 46 insertions(+), 11 deletions(-)

-- 
Signature

[PATCH 1/3] imsm: FIX: Chunk size migration is not possible

[Adam Kwolek]

Chunk size only migration for raid0 and raid5 is not possible.
(mdadm UT 15* fails). Mdadm exits with information:
   mdadm: imsm unknown layout 0xffffffff for this raid level 0

Problem was introduced in patch (2011-11-16):
   imsm: platform capabilities are not validated during level migration

During chunk size migration layout variable is not set correctly.
Set it correctly for this migration type.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 3a34f5a..e1073ef 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -9050,8 +9050,10 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 	int change = -1;
 	int check_devs = 0;
 	int chunk;
-	int devNumChange=0;
-	int layout = -1;
+	/* number of added/removed disks in operation result */
+	int devNumChange = 0;
+	/* imsm compatible layout value for array geometry verification */
+	int imsm_layout = -1;
 
 	getinfo_super_imsm_volume(st, &info, NULL);
 	if ((geo->level != info.array.level) &&
@@ -9069,14 +9071,14 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 					change = -1;
 					goto analyse_change_exit;
 				}
-				layout =  geo->layout;
+				imsm_layout =  geo->layout;
 				check_devs = 1;
 				devNumChange = 1; /* parity disk added */
 			} else if (geo->level == 10) {
 				change = CH_TAKEOVER;
 				check_devs = 1;
 				devNumChange = 2; /* two mirrors added */
-				layout = 0x102; /* imsm supported layout */
+				imsm_layout = 0x102; /* imsm supported layout */
 			}
 			break;
 		case 1:
@@ -9085,7 +9087,7 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 				change = CH_TAKEOVER;
 				check_devs = 1;
 				devNumChange = -(geo->raid_disks/2);
-				layout = 0; /* imsm raid0 layout */
+				imsm_layout = 0; /* imsm raid0 layout */
 			}
 			break;
 		}
@@ -9120,8 +9122,11 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 			change = -1;
 			goto analyse_change_exit;
 		}
-	} else
+	} else {
 		geo->layout = info.array.layout;
+		if (imsm_layout == -1)
+			imsm_layout = info.array.layout;
+	}
 
 	if ((geo->chunksize > 0) && (geo->chunksize != UnSet)
 	    && (geo->chunksize != info.array.chunk_size))
@@ -9132,7 +9137,7 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
 	chunk = geo->chunksize / 1024;
 	if (!validate_geometry_imsm(st,
 				    geo->level,
-				    layout,
+				    imsm_layout,
 				    geo->raid_disks + devNumChange,
 				    &chunk,
 				    geo->size,

Re: [PATCH 1/3] imsm: FIX: Chunk size migration is not possible

[NeilBrown]

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

On Wed, 14 Dec 2011 16:07:05 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> Chunk size only migration for raid0 and raid5 is not possible.
> (mdadm UT 15* fails). Mdadm exits with information:
>    mdadm: imsm unknown layout 0xffffffff for this raid level 0
> 
> Problem was introduced in patch (2011-11-16):
>    imsm: platform capabilities are not validated during level migration
> 
> During chunk size migration layout variable is not set correctly.
> Set it correctly for this migration type.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>


Applied, thanks.

NeilBrown







> ---
> 
>  super-intel.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index 3a34f5a..e1073ef 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -9050,8 +9050,10 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
>  	int change = -1;
>  	int check_devs = 0;
>  	int chunk;
> -	int devNumChange=0;
> -	int layout = -1;
> +	/* number of added/removed disks in operation result */
> +	int devNumChange = 0;
> +	/* imsm compatible layout value for array geometry verification */
> +	int imsm_layout = -1;
>  
>  	getinfo_super_imsm_volume(st, &info, NULL);
>  	if ((geo->level != info.array.level) &&
> @@ -9069,14 +9071,14 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
>  					change = -1;
>  					goto analyse_change_exit;
>  				}
> -				layout =  geo->layout;
> +				imsm_layout =  geo->layout;
>  				check_devs = 1;
>  				devNumChange = 1; /* parity disk added */
>  			} else if (geo->level == 10) {
>  				change = CH_TAKEOVER;
>  				check_devs = 1;
>  				devNumChange = 2; /* two mirrors added */
> -				layout = 0x102; /* imsm supported layout */
> +				imsm_layout = 0x102; /* imsm supported layout */
>  			}
>  			break;
>  		case 1:
> @@ -9085,7 +9087,7 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
>  				change = CH_TAKEOVER;
>  				check_devs = 1;
>  				devNumChange = -(geo->raid_disks/2);
> -				layout = 0; /* imsm raid0 layout */
> +				imsm_layout = 0; /* imsm raid0 layout */
>  			}
>  			break;
>  		}
> @@ -9120,8 +9122,11 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
>  			change = -1;
>  			goto analyse_change_exit;
>  		}
> -	} else
> +	} else {
>  		geo->layout = info.array.layout;
> +		if (imsm_layout == -1)
> +			imsm_layout = info.array.layout;
> +	}
>  
>  	if ((geo->chunksize > 0) && (geo->chunksize != UnSet)
>  	    && (geo->chunksize != info.array.chunk_size))
> @@ -9132,7 +9137,7 @@ enum imsm_reshape_type imsm_analyze_change(struct supertype *st,
>  	chunk = geo->chunksize / 1024;
>  	if (!validate_geometry_imsm(st,
>  				    geo->level,
> -				    layout,
> +				    imsm_layout,
>  				    geo->raid_disks + devNumChange,
>  				    &chunk,
>  				    geo->size,


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

[PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails

[Adam Kwolek]

Problem was introduced by patch (2011-06-08):
   getinfo_super now clears the 'info' structure before filling it in.

Field update private is not managed here and pointer associated outside
is cleaned up.
Add code for field update_private cleaning preservation.
In places where in patch
  'getinfo_super now clears the 'info' structure before filling it in.'
cleaning structure was removed, cleaning update_private field was added
as getinfo_super() cannot be responsible for this pointer management.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 Assemble.c    |    2 ++
 Incremental.c |    3 +++
 super-intel.c |    9 +++++++++
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index fac2bad..c8b538f 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -422,6 +422,7 @@ int Assemble(struct supertype *st, char *mddev,
 					int uuid[4];
 
 					content = &info;
+					info.update_private = NULL
 					tst->ss->getinfo_super(tst, content, NULL);
 
 					if (!parse_uuid(ident->container, uuid) ||
@@ -485,6 +486,7 @@ int Assemble(struct supertype *st, char *mddev,
 		} else {
 
 			content = &info;
+			info.update_private = NULL
 			tst->ss->getinfo_super(tst, content, NULL);
 
 			if (!ident_matches(ident, content, tst,
diff --git a/Incremental.c b/Incremental.c
index d3724a4..112a1ec 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -205,6 +205,7 @@ int Incremental(char *devname, int verbose, int runstop,
 	}
 	close (dfd); dfd = -1;
 
+	info.update_private = NULL
 	st->ss->getinfo_super(st, &info, NULL);
 
 	/* 3/ Check if there is a match in mdadm.conf */
@@ -404,6 +405,7 @@ int Incremental(char *devname, int verbose, int runstop,
 				goto out_unlock;
 			}
 			close(dfd2);
+			info.update_private = NULL
 			st2->ss->getinfo_super(st2, &info2, NULL);
 			st2->ss->free_super(st2);
 			if (info.array.level != info2.array.level ||
@@ -1382,6 +1384,7 @@ static int Incremental_container(struct supertype *st, char *devname,
 	int ra_blocked = 0;
 	int ra_all = 0;
 
+	info.update_private = NULL
 	st->ss->getinfo_super(st, &info, NULL);
 
 	if ((runstop > 0 && info.container_enough >= 0) ||
diff --git a/super-intel.c b/super-intel.c
index e1073ef..5e1d278 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -2365,8 +2365,13 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
 	char *devname;
 	unsigned int component_size_alligment;
 	int map_disks = info->array.raid_disks;
+	void *update_private_saver = info->update_private;
 
 	memset(info, 0, sizeof(*info));
+	/* preserve pointer cleanup, as someone elese is pointer owner
+	 */
+	info->update_private = update_private_saver;
+
 	if (prev_map)
 		map_to_analyse = prev_map;
 
@@ -2601,12 +2606,16 @@ static void getinfo_super_imsm(struct supertype *st, struct mdinfo *info, char *
 	int max_enough = -1;
 	int i;
 	struct imsm_super *mpb;
+	void *update_private_saver = info->update_private;
 
 	if (super->current_vol >= 0) {
 		getinfo_super_imsm_volume(st, info, map);
 		return;
 	}
 	memset(info, 0, sizeof(*info));
+	/* preserve pointer cleanup, as someone elese is pointer owner
+	 */
+	info->update_private = update_private_saver;
 
 	/* Set raid_disks to zero so that Assemble will always pull in valid
 	 * spares

Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails

[Peter W. Morreale]

On Wed, 2011-12-14 at 16:07 +0100, Adam Kwolek wrote: 
> Problem was introduced by patch (2011-06-08):
>    getinfo_super now clears the 'info' structure before filling it in.
> 
> Field update private is not managed here and pointer associated outside
> is cleaned up.
> Add code for field update_private cleaning preservation.
> In places where in patch
>   'getinfo_super now clears the 'info' structure before filling it in.'
> cleaning structure was removed, cleaning update_private field was added
> as getinfo_super() cannot be responsible for this pointer management.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  Assemble.c    |    2 ++
>  Incremental.c |    3 +++
>  super-intel.c |    9 +++++++++
>  3 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/Assemble.c b/Assemble.c
> index fac2bad..c8b538f 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -422,6 +422,7 @@ int Assemble(struct supertype *st, char *mddev,
>  					int uuid[4];
>  
>  					content = &info;
> +					info.update_private = NULL
>  					tst->ss->getinfo_super(tst, content, NULL);
>  
>  					if (!parse_uuid(ident->container, uuid) ||
> @@ -485,6 +486,7 @@ int Assemble(struct supertype *st, char *mddev,
>  		} else {
>  
>  			content = &info;
> +			info.update_private = NULL
>  			tst->ss->getinfo_super(tst, content, NULL);
>  
>  			if (!ident_matches(ident, content, tst,
> diff --git a/Incremental.c b/Incremental.c
> index d3724a4..112a1ec 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -205,6 +205,7 @@ int Incremental(char *devname, int verbose, int runstop,
>  	}
>  	close (dfd); dfd = -1;
>  
> +	info.update_private = NULL
>  	st->ss->getinfo_super(st, &info, NULL);
>  
>  	/* 3/ Check if there is a match in mdadm.conf */
> @@ -404,6 +405,7 @@ int Incremental(char *devname, int verbose, int runstop,
>  				goto out_unlock;
>  			}
>  			close(dfd2);
> +			info.update_private = NULL
>  			st2->ss->getinfo_super(st2, &info2, NULL);
>  			st2->ss->free_super(st2);
>  			if (info.array.level != info2.array.level ||
> @@ -1382,6 +1384,7 @@ static int Incremental_container(struct supertype *st, char *devname,
>  	int ra_blocked = 0;
>  	int ra_all = 0;
>  
> +	info.update_private = NULL
>  	st->ss->getinfo_super(st, &info, NULL);
>  
>  	if ((runstop > 0 && info.container_enough >= 0) ||
> diff --git a/super-intel.c b/super-intel.c
> index e1073ef..5e1d278 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -2365,8 +2365,13 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
>  	char *devname;
>  	unsigned int component_size_alligment;
>  	int map_disks = info->array.raid_disks;
> +	void *update_private_saver = info->update_private;
>  
>  	memset(info, 0, sizeof(*info));
> +	/* preserve pointer cleanup, as someone elese is pointer owner
> +	 */

Comment formatting and typo.


> +	info->update_private = update_private_saver;
> +
>  	if (prev_map)
>  		map_to_analyse = prev_map;
>  
> @@ -2601,12 +2606,16 @@ static void getinfo_super_imsm(struct supertype *st, struct mdinfo *info, char *
>  	int max_enough = -1;
>  	int i;
>  	struct imsm_super *mpb;
> +	void *update_private_saver = info->update_private;
>  
>  	if (super->current_vol >= 0) {
>  		getinfo_super_imsm_volume(st, info, map);
>  		return;
>  	}
>  	memset(info, 0, sizeof(*info));
> +	/* preserve pointer cleanup, as someone elese is pointer owner
> +	 */

Comment formatting and typo...

-PWM


> +	info->update_private = update_private_saver;
>  
>  	/* Set raid_disks to zero so that Assemble will always pull in valid
>  	 * spares
> 
> --
> 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 2/3] imsm: FIX: 'UT 09imsm-assemble' fails

[NeilBrown]

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

On Wed, 14 Dec 2011 16:07:12 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> Problem was introduced by patch (2011-06-08):
>    getinfo_super now clears the 'info' structure before filling it in.
> 
> Field update private is not managed here and pointer associated outside
> is cleaned up.
> Add code for field update_private cleaning preservation.
> In places where in patch
>   'getinfo_super now clears the 'info' structure before filling it in.'
> cleaning structure was removed, cleaning update_private field was added
> as getinfo_super() cannot be responsible for this pointer management.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  Assemble.c    |    2 ++
>  Incremental.c |    3 +++
>  super-intel.c |    9 +++++++++
>  3 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/Assemble.c b/Assemble.c
> index fac2bad..c8b538f 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -422,6 +422,7 @@ int Assemble(struct supertype *st, char *mddev,
>  					int uuid[4];
>  
>  					content = &info;
> +					info.update_private = NULL
>  					tst->ss->getinfo_super(tst, content, NULL);
>  
>  					if (!parse_uuid(ident->container, uuid) ||
> @@ -485,6 +486,7 @@ int Assemble(struct supertype *st, char *mddev,
>  		} else {
>  
>  			content = &info;
> +			info.update_private = NULL
>  			tst->ss->getinfo_super(tst, content, NULL);
>  
>  			if (!ident_matches(ident, content, tst,
> diff --git a/Incremental.c b/Incremental.c
> index d3724a4..112a1ec 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -205,6 +205,7 @@ int Incremental(char *devname, int verbose, int runstop,
>  	}
>  	close (dfd); dfd = -1;
>  
> +	info.update_private = NULL
>  	st->ss->getinfo_super(st, &info, NULL);
>  
>  	/* 3/ Check if there is a match in mdadm.conf */
> @@ -404,6 +405,7 @@ int Incremental(char *devname, int verbose, int runstop,
>  				goto out_unlock;
>  			}
>  			close(dfd2);
> +			info.update_private = NULL
>  			st2->ss->getinfo_super(st2, &info2, NULL);
>  			st2->ss->free_super(st2);
>  			if (info.array.level != info2.array.level ||
> @@ -1382,6 +1384,7 @@ static int Incremental_container(struct supertype *st, char *devname,
>  	int ra_blocked = 0;
>  	int ra_all = 0;
>  
> +	info.update_private = NULL
>  	st->ss->getinfo_super(st, &info, NULL);
>  
>  	if ((runstop > 0 && info.container_enough >= 0) ||
> diff --git a/super-intel.c b/super-intel.c
> index e1073ef..5e1d278 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -2365,8 +2365,13 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
>  	char *devname;
>  	unsigned int component_size_alligment;
>  	int map_disks = info->array.raid_disks;
> +	void *update_private_saver = info->update_private;
>  
>  	memset(info, 0, sizeof(*info));
> +	/* preserve pointer cleanup, as someone elese is pointer owner
> +	 */
> +	info->update_private = update_private_saver;
> +
>  	if (prev_map)
>  		map_to_analyse = prev_map;
>  
> @@ -2601,12 +2606,16 @@ static void getinfo_super_imsm(struct supertype *st, struct mdinfo *info, char *
>  	int max_enough = -1;
>  	int i;
>  	struct imsm_super *mpb;
> +	void *update_private_saver = info->update_private;
>  
>  	if (super->current_vol >= 0) {
>  		getinfo_super_imsm_volume(st, info, map);
>  		return;
>  	}
>  	memset(info, 0, sizeof(*info));
> +	/* preserve pointer cleanup, as someone elese is pointer owner
> +	 */
> +	info->update_private = update_private_saver;
>  
>  	/* Set raid_disks to zero so that Assemble will always pull in valid
>  	 * spares
> 

You didn't really think I'd let that through, did you :-)

I've written an alternate which gets rid of update_private.

Could you and Dan please review and check it performs as required.

Thanks,
NeilBrown


commit 8606be19835abaf888d0fbd1729dcda82a8b2815
Author: NeilBrown <neilb@suse.de>
Date:   Thu Dec 15 14:59:12 2011 +1100

    Remove update_private
    
    This fields doesn't work any more as ->getinfo_super clears the info
    structure at an awkward time.  So get rid of it and do it differently.
    
    The issue is that the metadata handler cannot tell if the uuid it has
    was randomly generated or explicitly requested, except on the first
    call.
    And we don't want to accept explicit requests for IMSM.
    So when it was auto-generated, make it look distinctive by having the
    same int copied in all 4 positions.  If someone requests a uuid like
    that, I guess they get away with it.
    
    Reported-by: Adam Kwolek <adam.kwolek@intel.com>
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Assemble.c b/Assemble.c
index fac2bad..74fb6a3 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -706,7 +706,6 @@ int Assemble(struct supertype *st, char *mddev,
 	bitmap_done = 0;
 #endif
 	/* Ok, no bad inconsistancy, we can try updating etc */
-	content->update_private = NULL;
 	devices = malloc(num_devs * sizeof(*devices));
 	devmap = calloc(num_devs * content->array.raid_disks, 1);
 	for (tmpdev = devlist; tmpdev; tmpdev=tmpdev->next) if (tmpdev->used == 1) {
@@ -891,8 +890,6 @@ int Assemble(struct supertype *st, char *mddev,
 		}
 		devcnt++;
 	}
-	free(content->update_private);
-	content->update_private = NULL;
 
 	if (devcnt == 0) {
 		fprintf(stderr, Name ": no devices found for %s\n",
diff --git a/mdadm.h b/mdadm.h
index 1351d42..4f3533f 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -217,11 +217,6 @@ struct mdinfo {
 	unsigned long		cache_size; /* size of raid456 stripe cache*/
 	int			mismatch_cnt;
 	char			text_version[50];
-	void 			*update_private; /* for passing metadata-format
-						  * specific update data
-						  * between successive calls to
-						  * update_super()
-						  */
 
 	int container_member; /* for assembling external-metatdata arrays
 			       * This is to be used internally by metadata
diff --git a/super-intel.c b/super-intel.c
index e1073ef..78781c7 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -2791,25 +2791,30 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
 
 	mpb = super->anchor;
 
-	if (strcmp(update, "uuid") == 0 && uuid_set && !info->update_private)
-		rv = -1;
-	else if (strcmp(update, "uuid") == 0 && uuid_set && info->update_private) {
-		mpb->orig_family_num = *((__u32 *) info->update_private);
-		rv = 0;
-	} else if (strcmp(update, "uuid") == 0) {
-		__u32 *new_family = malloc(sizeof(*new_family));
-
-		/* update orig_family_number with the incoming random
-		 * data, report the new effective uuid, and store the
-		 * new orig_family_num for future updates.
+	if (strcmp(update, "uuid") == 0) {
+		/* We take this to mean that the family_num should be updated.
+		 * However that is much smaller than the uuid so we cannot really
+		 * allow an explicit uuid to be given.  And it is hard to reliably
+		 * know if one was.
+		 * So if !uuid_set we know the current uuid is random and just used
+		 * the first 'int' and copy it to the other 3 positions.
+		 * Otherwise we require the 4 'int's to be the same as would be the
+		 * case if we are using a random uuid.  So an explicit uuid will be
+		 * accepted as long as all for ints are the same... which shouldn't hurt
 		 */
-		if (new_family) {
-			memcpy(&mpb->orig_family_num, info->uuid, sizeof(__u32));
-			uuid_from_super_imsm(st, info->uuid);
-			*new_family = mpb->orig_family_num;
-			info->update_private = new_family;
+		if (uuid_set) {
+			info->uuid[1] = info->uuid[2] = info->uuid[3] = info->uuid[0];
 			rv = 0;
+		} else {
+			if (info->uuid[0] != info->uuid[1] ||
+			    info->uuid[1] != info->uuid[2] ||
+			    info->uuid[2] != info->uuid[3])
+				rv = -1;
+			else
+				rv = 0;
 		}
+		if (rv == 0)
+			mpb->orig_family_num = info->uuid[0];
 	} else if (strcmp(update, "assemble") == 0)
 		rv = 0;
 	else

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

RE: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, December 15, 2011 5:01 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams,
> Dan J
> Subject: Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails
> 
> On Wed, 14 Dec 2011 16:07:12 +0100 Adam Kwolek
> <adam.kwolek@intel.com> wrote:
> 
> > Problem was introduced by patch (2011-06-08):
> >    getinfo_super now clears the 'info' structure before filling it in.
> >
> > Field update private is not managed here and pointer associated
> > outside is cleaned up.
> > Add code for field update_private cleaning preservation.
> > In places where in patch
> >   'getinfo_super now clears the 'info' structure before filling it in.'
> > cleaning structure was removed, cleaning update_private field was
> > added as getinfo_super() cannot be responsible for this pointer
> management.
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  Assemble.c    |    2 ++
> >  Incremental.c |    3 +++
> >  super-intel.c |    9 +++++++++
> >  3 files changed, 14 insertions(+), 0 deletions(-)
> >
> > diff --git a/Assemble.c b/Assemble.c
> > index fac2bad..c8b538f 100644
> > --- a/Assemble.c
> > +++ b/Assemble.c
> > @@ -422,6 +422,7 @@ int Assemble(struct supertype *st, char *mddev,
> >  					int uuid[4];
> >
> >  					content = &info;
> > +					info.update_private = NULL
> >  					tst->ss->getinfo_super(tst, content,
> NULL);
> >
> >  					if (!parse_uuid(ident->container,
> uuid) || @@ -485,6 +486,7 @@
> > int Assemble(struct supertype *st, char *mddev,
> >  		} else {
> >
> >  			content = &info;
> > +			info.update_private = NULL
> >  			tst->ss->getinfo_super(tst, content, NULL);
> >
> >  			if (!ident_matches(ident, content, tst, diff --git
> a/Incremental.c
> > b/Incremental.c index d3724a4..112a1ec 100644
> > --- a/Incremental.c
> > +++ b/Incremental.c
> > @@ -205,6 +205,7 @@ int Incremental(char *devname, int verbose, int
> runstop,
> >  	}
> >  	close (dfd); dfd = -1;
> >
> > +	info.update_private = NULL
> >  	st->ss->getinfo_super(st, &info, NULL);
> >
> >  	/* 3/ Check if there is a match in mdadm.conf */ @@ -404,6 +405,7
> @@
> > int Incremental(char *devname, int verbose, int runstop,
> >  				goto out_unlock;
> >  			}
> >  			close(dfd2);
> > +			info.update_private = NULL
> >  			st2->ss->getinfo_super(st2, &info2, NULL);
> >  			st2->ss->free_super(st2);
> >  			if (info.array.level != info2.array.level || @@ -1382,6
> +1384,7 @@
> > static int Incremental_container(struct supertype *st, char *devname,
> >  	int ra_blocked = 0;
> >  	int ra_all = 0;
> >
> > +	info.update_private = NULL
> >  	st->ss->getinfo_super(st, &info, NULL);
> >
> >  	if ((runstop > 0 && info.container_enough >= 0) || diff --git
> > a/super-intel.c b/super-intel.c index e1073ef..5e1d278 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -2365,8 +2365,13 @@ static void getinfo_super_imsm_volume(struct
> supertype *st, struct mdinfo *info,
> >  	char *devname;
> >  	unsigned int component_size_alligment;
> >  	int map_disks = info->array.raid_disks;
> > +	void *update_private_saver = info->update_private;
> >
> >  	memset(info, 0, sizeof(*info));
> > +	/* preserve pointer cleanup, as someone elese is pointer owner
> > +	 */
> > +	info->update_private = update_private_saver;
> > +
> >  	if (prev_map)
> >  		map_to_analyse = prev_map;
> >
> > @@ -2601,12 +2606,16 @@ static void getinfo_super_imsm(struct
> supertype *st, struct mdinfo *info, char *
> >  	int max_enough = -1;
> >  	int i;
> >  	struct imsm_super *mpb;
> > +	void *update_private_saver = info->update_private;
> >
> >  	if (super->current_vol >= 0) {
> >  		getinfo_super_imsm_volume(st, info, map);
> >  		return;
> >  	}
> >  	memset(info, 0, sizeof(*info));
> > +	/* preserve pointer cleanup, as someone elese is pointer owner
> > +	 */
> > +	info->update_private = update_private_saver;
> >
> >  	/* Set raid_disks to zero so that Assemble will always pull in valid
> >  	 * spares
> >
> 
> You didn't really think I'd let that through, did you :-)
> 
> I've written an alternate which gets rid of update_private.
> 
> Could you and Dan please review and check it performs as required.
> 
> Thanks,
> NeilBrown



Don't you think that condition should set rv variable in opposite way? E.g.:
 +		} else {
 +			if (info->uuid[0] != info->uuid[1] ||
 +			    info->uuid[1] != info->uuid[2] ||
 +			    info->uuid[2] != info->uuid[3])
 +				rv = 0;
 +			else
 +				rv = -1;
  		}

I think that when condition is true, this means "random" value and update should be performed.

BR
Adam


> 
> 
> commit 8606be19835abaf888d0fbd1729dcda82a8b2815
> Author: NeilBrown <neilb@suse.de>
> Date:   Thu Dec 15 14:59:12 2011 +1100
> 
>     Remove update_private
> 
>     This fields doesn't work any more as ->getinfo_super clears the info
>     structure at an awkward time.  So get rid of it and do it differently.
> 
>     The issue is that the metadata handler cannot tell if the uuid it has
>     was randomly generated or explicitly requested, except on the first
>     call.
>     And we don't want to accept explicit requests for IMSM.
>     So when it was auto-generated, make it look distinctive by having the
>     same int copied in all 4 positions.  If someone requests a uuid like
>     that, I guess they get away with it.
> 
>     Reported-by: Adam Kwolek <adam.kwolek@intel.com>
>     Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/Assemble.c b/Assemble.c
> index fac2bad..74fb6a3 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -706,7 +706,6 @@ int Assemble(struct supertype *st, char *mddev,
>  	bitmap_done = 0;
>  #endif
>  	/* Ok, no bad inconsistancy, we can try updating etc */
> -	content->update_private = NULL;
>  	devices = malloc(num_devs * sizeof(*devices));
>  	devmap = calloc(num_devs * content->array.raid_disks, 1);
>  	for (tmpdev = devlist; tmpdev; tmpdev=tmpdev->next) if (tmpdev-
> >used == 1) { @@ -891,8 +890,6 @@ int Assemble(struct supertype *st, char
> *mddev,
>  		}
>  		devcnt++;
>  	}
> -	free(content->update_private);
> -	content->update_private = NULL;
> 
>  	if (devcnt == 0) {
>  		fprintf(stderr, Name ": no devices found for %s\n", diff --git
> a/mdadm.h b/mdadm.h index 1351d42..4f3533f 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -217,11 +217,6 @@ struct mdinfo {
>  	unsigned long		cache_size; /* size of raid456 stripe cache*/
>  	int			mismatch_cnt;
>  	char			text_version[50];
> -	void 			*update_private; /* for passing metadata-
> format
> -						  * specific update data
> -						  * between successive calls
> to
> -						  * update_super()
> -						  */
> 
>  	int container_member; /* for assembling external-metatdata arrays
>  			       * This is to be used internally by metadata diff --
> git a/super-intel.c b/super-intel.c index e1073ef..78781c7 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -2791,25 +2791,30 @@ static int update_super_imsm(struct supertype
> *st, struct mdinfo *info,
> 
>  	mpb = super->anchor;
> 
> -	if (strcmp(update, "uuid") == 0 && uuid_set && !info-
> >update_private)
> -		rv = -1;
> -	else if (strcmp(update, "uuid") == 0 && uuid_set && info-
> >update_private) {
> -		mpb->orig_family_num = *((__u32 *) info-
> >update_private);
> -		rv = 0;
> -	} else if (strcmp(update, "uuid") == 0) {
> -		__u32 *new_family = malloc(sizeof(*new_family));
> -
> -		/* update orig_family_number with the incoming random
> -		 * data, report the new effective uuid, and store the
> -		 * new orig_family_num for future updates.
> +	if (strcmp(update, "uuid") == 0) {
> +		/* We take this to mean that the family_num should be
> updated.
> +		 * However that is much smaller than the uuid so we cannot
> really
> +		 * allow an explicit uuid to be given.  And it is hard to reliably
> +		 * know if one was.
> +		 * So if !uuid_set we know the current uuid is random and
> just used
> +		 * the first 'int' and copy it to the other 3 positions.
> +		 * Otherwise we require the 4 'int's to be the same as would
> be the
> +		 * case if we are using a random uuid.  So an explicit uuid will
> be
> +		 * accepted as long as all for ints are the same... which
> shouldn't
> +hurt
>  		 */
> -		if (new_family) {
> -			memcpy(&mpb->orig_family_num, info->uuid,
> sizeof(__u32));
> -			uuid_from_super_imsm(st, info->uuid);
> -			*new_family = mpb->orig_family_num;
> -			info->update_private = new_family;
> +		if (uuid_set) {
> +			info->uuid[1] = info->uuid[2] = info->uuid[3] = info-
> >uuid[0];
>  			rv = 0;
> +		} else {
> +			if (info->uuid[0] != info->uuid[1] ||
> +			    info->uuid[1] != info->uuid[2] ||
> +			    info->uuid[2] != info->uuid[3])
> +				rv = -1;
> +			else
> +				rv = 0;
>  		}
> +		if (rv == 0)
> +			mpb->orig_family_num = info->uuid[0];
>  	} else if (strcmp(update, "assemble") == 0)
>  		rv = 0;
>  	else

Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails

[NeilBrown]

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

On Thu, 15 Dec 2011 10:28:08 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Thursday, December 15, 2011 5:01 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams,
> > Dan J
> > Subject: Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails
> > 
> > On Wed, 14 Dec 2011 16:07:12 +0100 Adam Kwolek
> > <adam.kwolek@intel.com> wrote:
> > 
> > > Problem was introduced by patch (2011-06-08):
> > >    getinfo_super now clears the 'info' structure before filling it in.
> > >
> > > Field update private is not managed here and pointer associated
> > > outside is cleaned up.
> > > Add code for field update_private cleaning preservation.
> > > In places where in patch
> > >   'getinfo_super now clears the 'info' structure before filling it in.'
> > > cleaning structure was removed, cleaning update_private field was
> > > added as getinfo_super() cannot be responsible for this pointer
> > management.
> > >
> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > ---
> > >
> > >  Assemble.c    |    2 ++
> > >  Incremental.c |    3 +++
> > >  super-intel.c |    9 +++++++++
> > >  3 files changed, 14 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/Assemble.c b/Assemble.c
> > > index fac2bad..c8b538f 100644
> > > --- a/Assemble.c
> > > +++ b/Assemble.c
> > > @@ -422,6 +422,7 @@ int Assemble(struct supertype *st, char *mddev,
> > >  					int uuid[4];
> > >
> > >  					content = &info;
> > > +					info.update_private = NULL
> > >  					tst->ss->getinfo_super(tst, content,
> > NULL);
> > >
> > >  					if (!parse_uuid(ident->container,
> > uuid) || @@ -485,6 +486,7 @@
> > > int Assemble(struct supertype *st, char *mddev,
> > >  		} else {
> > >
> > >  			content = &info;
> > > +			info.update_private = NULL
> > >  			tst->ss->getinfo_super(tst, content, NULL);
> > >
> > >  			if (!ident_matches(ident, content, tst, diff --git
> > a/Incremental.c
> > > b/Incremental.c index d3724a4..112a1ec 100644
> > > --- a/Incremental.c
> > > +++ b/Incremental.c
> > > @@ -205,6 +205,7 @@ int Incremental(char *devname, int verbose, int
> > runstop,
> > >  	}
> > >  	close (dfd); dfd = -1;
> > >
> > > +	info.update_private = NULL
> > >  	st->ss->getinfo_super(st, &info, NULL);
> > >
> > >  	/* 3/ Check if there is a match in mdadm.conf */ @@ -404,6 +405,7
> > @@
> > > int Incremental(char *devname, int verbose, int runstop,
> > >  				goto out_unlock;
> > >  			}
> > >  			close(dfd2);
> > > +			info.update_private = NULL
> > >  			st2->ss->getinfo_super(st2, &info2, NULL);
> > >  			st2->ss->free_super(st2);
> > >  			if (info.array.level != info2.array.level || @@ -1382,6
> > +1384,7 @@
> > > static int Incremental_container(struct supertype *st, char *devname,
> > >  	int ra_blocked = 0;
> > >  	int ra_all = 0;
> > >
> > > +	info.update_private = NULL
> > >  	st->ss->getinfo_super(st, &info, NULL);
> > >
> > >  	if ((runstop > 0 && info.container_enough >= 0) || diff --git
> > > a/super-intel.c b/super-intel.c index e1073ef..5e1d278 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -2365,8 +2365,13 @@ static void getinfo_super_imsm_volume(struct
> > supertype *st, struct mdinfo *info,
> > >  	char *devname;
> > >  	unsigned int component_size_alligment;
> > >  	int map_disks = info->array.raid_disks;
> > > +	void *update_private_saver = info->update_private;
> > >
> > >  	memset(info, 0, sizeof(*info));
> > > +	/* preserve pointer cleanup, as someone elese is pointer owner
> > > +	 */
> > > +	info->update_private = update_private_saver;
> > > +
> > >  	if (prev_map)
> > >  		map_to_analyse = prev_map;
> > >
> > > @@ -2601,12 +2606,16 @@ static void getinfo_super_imsm(struct
> > supertype *st, struct mdinfo *info, char *
> > >  	int max_enough = -1;
> > >  	int i;
> > >  	struct imsm_super *mpb;
> > > +	void *update_private_saver = info->update_private;
> > >
> > >  	if (super->current_vol >= 0) {
> > >  		getinfo_super_imsm_volume(st, info, map);
> > >  		return;
> > >  	}
> > >  	memset(info, 0, sizeof(*info));
> > > +	/* preserve pointer cleanup, as someone elese is pointer owner
> > > +	 */
> > > +	info->update_private = update_private_saver;
> > >
> > >  	/* Set raid_disks to zero so that Assemble will always pull in valid
> > >  	 * spares
> > >
> > 
> > You didn't really think I'd let that through, did you :-)
> > 
> > I've written an alternate which gets rid of update_private.
> > 
> > Could you and Dan please review and check it performs as required.
> > 
> > Thanks,
> > NeilBrown
> 
> 
> 
> Don't you think that condition should set rv variable in opposite way? E.g.:
>  +		} else {
>  +			if (info->uuid[0] != info->uuid[1] ||
>  +			    info->uuid[1] != info->uuid[2] ||
>  +			    info->uuid[2] != info->uuid[3])
>  +				rv = 0;
>  +			else
>  +				rv = -1;
>   		}
> 
> I think that when condition is true, this means "random" value and update should be performed.

If all 4 numbers are the same, then it is almost certain that uuid_set was
true the first time through and so we made them all the same.
In that case we want to succeed and used the first number as the family.
If the number are not all the same, then uuid_set was never true so the uuid
was given on the command line and we cannot handle that case so we give an
error.

So I think the original code is correct.

Thanks,
NeilBrown



> 
> BR
> Adam
> 
> 
> > 
> > 
> > commit 8606be19835abaf888d0fbd1729dcda82a8b2815
> > Author: NeilBrown <neilb@suse.de>
> > Date:   Thu Dec 15 14:59:12 2011 +1100
> > 
> >     Remove update_private
> > 
> >     This fields doesn't work any more as ->getinfo_super clears the info
> >     structure at an awkward time.  So get rid of it and do it differently.
> > 
> >     The issue is that the metadata handler cannot tell if the uuid it has
> >     was randomly generated or explicitly requested, except on the first
> >     call.
> >     And we don't want to accept explicit requests for IMSM.
> >     So when it was auto-generated, make it look distinctive by having the
> >     same int copied in all 4 positions.  If someone requests a uuid like
> >     that, I guess they get away with it.
> > 
> >     Reported-by: Adam Kwolek <adam.kwolek@intel.com>
> >     Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/Assemble.c b/Assemble.c
> > index fac2bad..74fb6a3 100644
> > --- a/Assemble.c
> > +++ b/Assemble.c
> > @@ -706,7 +706,6 @@ int Assemble(struct supertype *st, char *mddev,
> >  	bitmap_done = 0;
> >  #endif
> >  	/* Ok, no bad inconsistancy, we can try updating etc */
> > -	content->update_private = NULL;
> >  	devices = malloc(num_devs * sizeof(*devices));
> >  	devmap = calloc(num_devs * content->array.raid_disks, 1);
> >  	for (tmpdev = devlist; tmpdev; tmpdev=tmpdev->next) if (tmpdev-
> > >used == 1) { @@ -891,8 +890,6 @@ int Assemble(struct supertype *st, char
> > *mddev,
> >  		}
> >  		devcnt++;
> >  	}
> > -	free(content->update_private);
> > -	content->update_private = NULL;
> > 
> >  	if (devcnt == 0) {
> >  		fprintf(stderr, Name ": no devices found for %s\n", diff --git
> > a/mdadm.h b/mdadm.h index 1351d42..4f3533f 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -217,11 +217,6 @@ struct mdinfo {
> >  	unsigned long		cache_size; /* size of raid456 stripe cache*/
> >  	int			mismatch_cnt;
> >  	char			text_version[50];
> > -	void 			*update_private; /* for passing metadata-
> > format
> > -						  * specific update data
> > -						  * between successive calls
> > to
> > -						  * update_super()
> > -						  */
> > 
> >  	int container_member; /* for assembling external-metatdata arrays
> >  			       * This is to be used internally by metadata diff --
> > git a/super-intel.c b/super-intel.c index e1073ef..78781c7 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -2791,25 +2791,30 @@ static int update_super_imsm(struct supertype
> > *st, struct mdinfo *info,
> > 
> >  	mpb = super->anchor;
> > 
> > -	if (strcmp(update, "uuid") == 0 && uuid_set && !info-
> > >update_private)
> > -		rv = -1;
> > -	else if (strcmp(update, "uuid") == 0 && uuid_set && info-
> > >update_private) {
> > -		mpb->orig_family_num = *((__u32 *) info-
> > >update_private);
> > -		rv = 0;
> > -	} else if (strcmp(update, "uuid") == 0) {
> > -		__u32 *new_family = malloc(sizeof(*new_family));
> > -
> > -		/* update orig_family_number with the incoming random
> > -		 * data, report the new effective uuid, and store the
> > -		 * new orig_family_num for future updates.
> > +	if (strcmp(update, "uuid") == 0) {
> > +		/* We take this to mean that the family_num should be
> > updated.
> > +		 * However that is much smaller than the uuid so we cannot
> > really
> > +		 * allow an explicit uuid to be given.  And it is hard to reliably
> > +		 * know if one was.
> > +		 * So if !uuid_set we know the current uuid is random and
> > just used
> > +		 * the first 'int' and copy it to the other 3 positions.
> > +		 * Otherwise we require the 4 'int's to be the same as would
> > be the
> > +		 * case if we are using a random uuid.  So an explicit uuid will
> > be
> > +		 * accepted as long as all for ints are the same... which
> > shouldn't
> > +hurt
> >  		 */
> > -		if (new_family) {
> > -			memcpy(&mpb->orig_family_num, info->uuid,
> > sizeof(__u32));
> > -			uuid_from_super_imsm(st, info->uuid);
> > -			*new_family = mpb->orig_family_num;
> > -			info->update_private = new_family;
> > +		if (uuid_set) {
> > +			info->uuid[1] = info->uuid[2] = info->uuid[3] = info-
> > >uuid[0];
> >  			rv = 0;
> > +		} else {
> > +			if (info->uuid[0] != info->uuid[1] ||
> > +			    info->uuid[1] != info->uuid[2] ||
> > +			    info->uuid[2] != info->uuid[3])
> > +				rv = -1;
> > +			else
> > +				rv = 0;
> >  		}
> > +		if (rv == 0)
> > +			mpb->orig_family_num = info->uuid[0];
> >  	} else if (strcmp(update, "assemble") == 0)
> >  		rv = 0;
> >  	else


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

RE: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, December 15, 2011 12:08 PM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams,
> Dan J
> Subject: Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails
> 
> On Thu, 15 Dec 2011 10:28:08 +0000 "Kwolek, Adam"
> <adam.kwolek@intel.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Thursday, December 15, 2011 5:01 AM
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin;
> > > Williams, Dan J
> > > Subject: Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails
> > >
> > > On Wed, 14 Dec 2011 16:07:12 +0100 Adam Kwolek
> > > <adam.kwolek@intel.com> wrote:
> > >
> > > > Problem was introduced by patch (2011-06-08):
> > > >    getinfo_super now clears the 'info' structure before filling it in.
> > > >
> > > > Field update private is not managed here and pointer associated
> > > > outside is cleaned up.
> > > > Add code for field update_private cleaning preservation.
> > > > In places where in patch
> > > >   'getinfo_super now clears the 'info' structure before filling it in.'
> > > > cleaning structure was removed, cleaning update_private field was
> > > > added as getinfo_super() cannot be responsible for this pointer
> > > management.
> > > >
> > > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > > ---
> > > >
> > > >  Assemble.c    |    2 ++
> > > >  Incremental.c |    3 +++
> > > >  super-intel.c |    9 +++++++++
> > > >  3 files changed, 14 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/Assemble.c b/Assemble.c index fac2bad..c8b538f 100644
> > > > --- a/Assemble.c
> > > > +++ b/Assemble.c
> > > > @@ -422,6 +422,7 @@ int Assemble(struct supertype *st, char
> *mddev,
> > > >  					int uuid[4];
> > > >
> > > >  					content = &info;
> > > > +					info.update_private = NULL
> > > >  					tst->ss->getinfo_super(tst, content,
> > > NULL);
> > > >
> > > >  					if (!parse_uuid(ident->container,
> > > uuid) || @@ -485,6 +486,7 @@
> > > > int Assemble(struct supertype *st, char *mddev,
> > > >  		} else {
> > > >
> > > >  			content = &info;
> > > > +			info.update_private = NULL
> > > >  			tst->ss->getinfo_super(tst, content, NULL);
> > > >
> > > >  			if (!ident_matches(ident, content, tst, diff --git
> > > a/Incremental.c
> > > > b/Incremental.c index d3724a4..112a1ec 100644
> > > > --- a/Incremental.c
> > > > +++ b/Incremental.c
> > > > @@ -205,6 +205,7 @@ int Incremental(char *devname, int verbose,
> > > > int
> > > runstop,
> > > >  	}
> > > >  	close (dfd); dfd = -1;
> > > >
> > > > +	info.update_private = NULL
> > > >  	st->ss->getinfo_super(st, &info, NULL);
> > > >
> > > >  	/* 3/ Check if there is a match in mdadm.conf */ @@ -404,6
> > > > +405,7
> > > @@
> > > > int Incremental(char *devname, int verbose, int runstop,
> > > >  				goto out_unlock;
> > > >  			}
> > > >  			close(dfd2);
> > > > +			info.update_private = NULL
> > > >  			st2->ss->getinfo_super(st2, &info2, NULL);
> > > >  			st2->ss->free_super(st2);
> > > >  			if (info.array.level != info2.array.level || @@ -1382,6
> > > +1384,7 @@
> > > > static int Incremental_container(struct supertype *st, char *devname,
> > > >  	int ra_blocked = 0;
> > > >  	int ra_all = 0;
> > > >
> > > > +	info.update_private = NULL
> > > >  	st->ss->getinfo_super(st, &info, NULL);
> > > >
> > > >  	if ((runstop > 0 && info.container_enough >= 0) || diff --git
> > > > a/super-intel.c b/super-intel.c index e1073ef..5e1d278 100644
> > > > --- a/super-intel.c
> > > > +++ b/super-intel.c
> > > > @@ -2365,8 +2365,13 @@ static void
> > > > getinfo_super_imsm_volume(struct
> > > supertype *st, struct mdinfo *info,
> > > >  	char *devname;
> > > >  	unsigned int component_size_alligment;
> > > >  	int map_disks = info->array.raid_disks;
> > > > +	void *update_private_saver = info->update_private;
> > > >
> > > >  	memset(info, 0, sizeof(*info));
> > > > +	/* preserve pointer cleanup, as someone elese is pointer owner
> > > > +	 */
> > > > +	info->update_private = update_private_saver;
> > > > +
> > > >  	if (prev_map)
> > > >  		map_to_analyse = prev_map;
> > > >
> > > > @@ -2601,12 +2606,16 @@ static void getinfo_super_imsm(struct
> > > supertype *st, struct mdinfo *info, char *
> > > >  	int max_enough = -1;
> > > >  	int i;
> > > >  	struct imsm_super *mpb;
> > > > +	void *update_private_saver = info->update_private;
> > > >
> > > >  	if (super->current_vol >= 0) {
> > > >  		getinfo_super_imsm_volume(st, info, map);
> > > >  		return;
> > > >  	}
> > > >  	memset(info, 0, sizeof(*info));
> > > > +	/* preserve pointer cleanup, as someone elese is pointer owner
> > > > +	 */
> > > > +	info->update_private = update_private_saver;
> > > >
> > > >  	/* Set raid_disks to zero so that Assemble will always pull in valid
> > > >  	 * spares
> > > >
> > >
> > > You didn't really think I'd let that through, did you :-)
> > >
> > > I've written an alternate which gets rid of update_private.
> > >
> > > Could you and Dan please review and check it performs as required.
> > >
> > > Thanks,
> > > NeilBrown
> >
> >
> >
> > Don't you think that condition should set rv variable in opposite way? E.g.:
> >  +		} else {
> >  +			if (info->uuid[0] != info->uuid[1] ||
> >  +			    info->uuid[1] != info->uuid[2] ||
> >  +			    info->uuid[2] != info->uuid[3])
> >  +				rv = 0;
> >  +			else
> >  +				rv = -1;
> >   		}
> >
> > I think that when condition is true, this means "random" value and update
> should be performed.
> 
> If all 4 numbers are the same, then it is almost certain that uuid_set was true
> the first time through and so we made them all the same.
> In that case we want to succeed and used the first number as the family.
> If the number are not all the same, then uuid_set was never true so the uuid
> was given on the command line and we cannot handle that case so we give
> an error.
> 
> So I think the original code is correct.
> 
> Thanks,
> NeilBrown


... what do you think about adding '!' in condition 'if (uuid_set) {'
It is like  your comment in code directs to.  With this change test looks ok, but I can miss something this time also ;).

BR
Adam


> 
> 
> 
> >
> > BR
> > Adam
> >
> >
> > >
> > >
> > > commit 8606be19835abaf888d0fbd1729dcda82a8b2815
> > > Author: NeilBrown <neilb@suse.de>
> > > Date:   Thu Dec 15 14:59:12 2011 +1100
> > >
> > >     Remove update_private
> > >
> > >     This fields doesn't work any more as ->getinfo_super clears the info
> > >     structure at an awkward time.  So get rid of it and do it differently.
> > >
> > >     The issue is that the metadata handler cannot tell if the uuid it has
> > >     was randomly generated or explicitly requested, except on the first
> > >     call.
> > >     And we don't want to accept explicit requests for IMSM.
> > >     So when it was auto-generated, make it look distinctive by having the
> > >     same int copied in all 4 positions.  If someone requests a uuid like
> > >     that, I guess they get away with it.
> > >
> > >     Reported-by: Adam Kwolek <adam.kwolek@intel.com>
> > >     Signed-off-by: NeilBrown <neilb@suse.de>
> > >
> > > diff --git a/Assemble.c b/Assemble.c index fac2bad..74fb6a3 100644
> > > --- a/Assemble.c
> > > +++ b/Assemble.c
> > > @@ -706,7 +706,6 @@ int Assemble(struct supertype *st, char *mddev,
> > >  	bitmap_done = 0;
> > >  #endif
> > >  	/* Ok, no bad inconsistancy, we can try updating etc */
> > > -	content->update_private = NULL;
> > >  	devices = malloc(num_devs * sizeof(*devices));
> > >  	devmap = calloc(num_devs * content->array.raid_disks, 1);
> > >  	for (tmpdev = devlist; tmpdev; tmpdev=tmpdev->next) if (tmpdev-
> > > >used == 1) { @@ -891,8 +890,6 @@ int Assemble(struct supertype *st,
> > > >char
> > > *mddev,
> > >  		}
> > >  		devcnt++;
> > >  	}
> > > -	free(content->update_private);
> > > -	content->update_private = NULL;
> > >
> > >  	if (devcnt == 0) {
> > >  		fprintf(stderr, Name ": no devices found for %s\n", diff --git
> > > a/mdadm.h b/mdadm.h index 1351d42..4f3533f 100644
> > > --- a/mdadm.h
> > > +++ b/mdadm.h
> > > @@ -217,11 +217,6 @@ struct mdinfo {
> > >  	unsigned long		cache_size; /* size of raid456 stripe cache*/
> > >  	int			mismatch_cnt;
> > >  	char			text_version[50];
> > > -	void 			*update_private; /* for passing metadata-
> > > format
> > > -						  * specific update data
> > > -						  * between successive calls
> > > to
> > > -						  * update_super()
> > > -						  */
> > >
> > >  	int container_member; /* for assembling external-metatdata arrays
> > >  			       * This is to be used internally by metadata diff --
> git
> > > a/super-intel.c b/super-intel.c index e1073ef..78781c7 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -2791,25 +2791,30 @@ static int update_super_imsm(struct
> > > supertype *st, struct mdinfo *info,
> > >
> > >  	mpb = super->anchor;
> > >
> > > -	if (strcmp(update, "uuid") == 0 && uuid_set && !info-
> > > >update_private)
> > > -		rv = -1;
> > > -	else if (strcmp(update, "uuid") == 0 && uuid_set && info-
> > > >update_private) {
> > > -		mpb->orig_family_num = *((__u32 *) info-
> > > >update_private);
> > > -		rv = 0;
> > > -	} else if (strcmp(update, "uuid") == 0) {
> > > -		__u32 *new_family = malloc(sizeof(*new_family));
> > > -
> > > -		/* update orig_family_number with the incoming random
> > > -		 * data, report the new effective uuid, and store the
> > > -		 * new orig_family_num for future updates.
> > > +	if (strcmp(update, "uuid") == 0) {
> > > +		/* We take this to mean that the family_num should be
> > > updated.
> > > +		 * However that is much smaller than the uuid so we cannot
> > > really
> > > +		 * allow an explicit uuid to be given.  And it is hard to reliably
> > > +		 * know if one was.
> > > +		 * So if !uuid_set we know the current uuid is random and
> > > just used
> > > +		 * the first 'int' and copy it to the other 3 positions.
> > > +		 * Otherwise we require the 4 'int's to be the same as would
> > > be the
> > > +		 * case if we are using a random uuid.  So an explicit uuid will
> > > be
> > > +		 * accepted as long as all for ints are the same... which
> > > shouldn't
> > > +hurt
> > >  		 */
> > > -		if (new_family) {
> > > -			memcpy(&mpb->orig_family_num, info->uuid,
> > > sizeof(__u32));
> > > -			uuid_from_super_imsm(st, info->uuid);
> > > -			*new_family = mpb->orig_family_num;
> > > -			info->update_private = new_family;
> > > +		if (uuid_set) {
> > > +			info->uuid[1] = info->uuid[2] = info->uuid[3] = info-
> > > >uuid[0];
> > >  			rv = 0;
> > > +		} else {
> > > +			if (info->uuid[0] != info->uuid[1] ||
> > > +			    info->uuid[1] != info->uuid[2] ||
> > > +			    info->uuid[2] != info->uuid[3])
> > > +				rv = -1;
> > > +			else
> > > +				rv = 0;
> > >  		}
> > > +		if (rv == 0)
> > > +			mpb->orig_family_num = info->uuid[0];
> > >  	} else if (strcmp(update, "assemble") == 0)
> > >  		rv = 0;
> > >  	else

Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails

[NeilBrown]

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

On Thu, 15 Dec 2011 15:29:50 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> 
> ... what do you think about adding '!' in condition 'if (uuid_set) {'
> It is like  your comment in code directs to.  With this change test looks ok, but I can miss something this time also ;).

Yes, you are right.  I need a '!' in there.  Thanks!

NeilBrown

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

[PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails

[Adam Kwolek]

Problem was introduced by patch (2011-09-19):
   Create: Allow to create two volumes of different sizes within one container

To resolve problem check requested disks number not against all disks
recorded in metadata, but against disks number set in array map structure.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 5e1d278..3c10d29 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5318,10 +5318,26 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
 
 	mpb = super->anchor;
 
-	if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
-		fprintf(stderr, Name ": the option-rom requires all "
-			"member disks to be a member of all volumes.\n");
-		return 0;
+	/* check if currently verified volume spans the same disks number
+	 * as all other arrays that belongs to metadata.
+	 * Do not allow to create volume with different disks number
+	 * than curently is used.
+	 */
+	for (i = 0; i < mpb->num_raid_devs; i++) {
+		/* There is any already existing array in metadata
+		 */
+		struct imsm_dev *dev = get_imsm_dev(super, i);
+		struct imsm_map *map = NULL;
+		if (dev)
+			map = get_imsm_map(dev, MAP_0);
+		if (map) {
+			if (raiddisks != map->num_members) {
+				fprintf(stderr, Name ": the option-rom requires"
+					" all member disks to be a member of "
+					"all volumes.\n");
+				return 0;
+			}
+		}
 	}
 
 	if (!validate_geometry_imsm_orom(super, level, layout, raiddisks, chunk, verbose)) {

Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails

[Williams, Dan J]

On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek <adam.kwolek@intel.com> wrote:
> Problem was introduced by patch (2011-09-19):
>   Create: Allow to create two volumes of different sizes within one container
>
> To resolve problem check requested disks number not against all disks
> recorded in metadata, but against disks number set in array map structure.
>
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
>
>  super-intel.c |   24 ++++++++++++++++++++----
>  1 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 5e1d278..3c10d29 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5318,10 +5318,26 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
>
>        mpb = super->anchor;
>
> -       if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
> -               fprintf(stderr, Name ": the option-rom requires all "
> -                       "member disks to be a member of all volumes.\n");
> -               return 0;
> +       /* check if currently verified volume spans the same disks number
> +        * as all other arrays that belongs to metadata.
> +        * Do not allow to create volume with different disks number
> +        * than curently is used.
> +        */
> +       for (i = 0; i < mpb->num_raid_devs; i++) {
> +               /* There is any already existing array in metadata
> +                */
> +               struct imsm_dev *dev = get_imsm_dev(super, i);
> +               struct imsm_map *map = NULL;
> +               if (dev)
> +                       map = get_imsm_map(dev, MAP_0);
> +               if (map) {
> +                       if (raiddisks != map->num_members) {
> +                               fprintf(stderr, Name ": the option-rom requires"
> +                                       " all member disks to be a member of "
> +                                       "all volumes.\n");
> +                               return 0;
> +                       }
> +               }

I'd prefer to move this check back to the other 'orom' checks in
validate_geometry_imsm_volume(), and make it dependent on the presence
of the orom.

diff --git a/super-intel.c b/super-intel.c
index 07d47b5..9885b98 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5122,12 +5122,6 @@ static int validate_geometry_imsm_volume(struct
supertype *st, int level,
 	if (!super)
 		return 0;

-	if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
-		fprintf(stderr, Name ": the option-rom requires all "
-			"member disks to be a member of all volumes.\n");
-		return 0;
-	}
-
 	if (!validate_geometry_imsm_orom(super, level, layout, raiddisks,
chunk, verbose)) {
 		fprintf(stderr, Name ": RAID gemetry validation failed. "
 			"Cannot proceed with the action(s).\n");
@@ -5206,6 +5200,11 @@ static int validate_geometry_imsm_volume(struct
supertype *st, int level,
 		fprintf(stderr, Name ": The option-rom requires all member"
 			" disks to be a member of all volumes\n");
 		return 0;
+	} else if (super->orom && mpb->num_raid_devs > 0 &&
+		   mpb->num_disks != raiddisks) {
+		fprintf(stderr, Name ": The option-rom requires all member"
+			" disks to be a member of all volumes\n");
+		return 0;
 	}

 	/* retrieve the largest free space block */
--
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 3/3] imsm: FIX: UT '08imsm-overlap' fails

[NeilBrown]

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

On Wed, 14 Dec 2011 10:21:07 -0800 "Williams, Dan J"
<dan.j.williams@intel.com> wrote:

> On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek <adam.kwolek@intel.com> wrote:
> > Problem was introduced by patch (2011-09-19):
> >   Create: Allow to create two volumes of different sizes within one container
> >
> > To resolve problem check requested disks number not against all disks
> > recorded in metadata, but against disks number set in array map structure.
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  super-intel.c |   24 ++++++++++++++++++++----
> >  1 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index 5e1d278..3c10d29 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -5318,10 +5318,26 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
> >
> >        mpb = super->anchor;
> >
> > -       if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
> > -               fprintf(stderr, Name ": the option-rom requires all "
> > -                       "member disks to be a member of all volumes.\n");
> > -               return 0;
> > +       /* check if currently verified volume spans the same disks number
> > +        * as all other arrays that belongs to metadata.
> > +        * Do not allow to create volume with different disks number
> > +        * than curently is used.
> > +        */
> > +       for (i = 0; i < mpb->num_raid_devs; i++) {
> > +               /* There is any already existing array in metadata
> > +                */
> > +               struct imsm_dev *dev = get_imsm_dev(super, i);
> > +               struct imsm_map *map = NULL;
> > +               if (dev)
> > +                       map = get_imsm_map(dev, MAP_0);
> > +               if (map) {
> > +                       if (raiddisks != map->num_members) {
> > +                               fprintf(stderr, Name ": the option-rom requires"
> > +                                       " all member disks to be a member of "
> > +                                       "all volumes.\n");
> > +                               return 0;
> > +                       }
> > +               }
> 
> I'd prefer to move this check back to the other 'orom' checks in
> validate_geometry_imsm_volume(), and make it dependent on the presence
> of the orom.
> 
> diff --git a/super-intel.c b/super-intel.c
> index 07d47b5..9885b98 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5122,12 +5122,6 @@ static int validate_geometry_imsm_volume(struct
> supertype *st, int level,
>  	if (!super)
>  		return 0;
> 
> -	if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
> -		fprintf(stderr, Name ": the option-rom requires all "
> -			"member disks to be a member of all volumes.\n");
> -		return 0;
> -	}
> -
>  	if (!validate_geometry_imsm_orom(super, level, layout, raiddisks,
> chunk, verbose)) {
>  		fprintf(stderr, Name ": RAID gemetry validation failed. "
>  			"Cannot proceed with the action(s).\n");
> @@ -5206,6 +5200,11 @@ static int validate_geometry_imsm_volume(struct
> supertype *st, int level,
>  		fprintf(stderr, Name ": The option-rom requires all member"
>  			" disks to be a member of all volumes\n");
>  		return 0;
> +	} else if (super->orom && mpb->num_raid_devs > 0 &&
> +		   mpb->num_disks != raiddisks) {
> +		fprintf(stderr, Name ": The option-rom requires all member"
> +			" disks to be a member of all volumes\n");
> +		return 0;
>  	}
> 
>  	/* retrieve the largest free space block */


Hi Dan,
 I think you are presenting this as an alternate - is that correct?
It seems a lit simpler - does it do enough?

Or is it just a precursor, and then we apply Adam's patch changing the same
code but in a different location?

Slightly confused...

NeilBrown


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

RE: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails

[Kwolek, Adam]

From: Williams, Dan J [mailto:dan.j.williams@intel.com] 
Sent: Thursday, December 15, 2011 6:16 AM
To: NeilBrown
Cc: Labun, Marcin; Ciechanowski, Ed; linux-raid@vger.kernel.org; Kwolek, Adam
Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails


On Dec 14, 2011 8:03 PM, "NeilBrown" <neilb@suse.de> wrote:
>
> On Wed, 14 Dec 2011 10:21:07 -0800 "Williams, Dan J"
> <dan.j.williams@intel.com> wrote:
>
> > On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek <adam.kwolek@intel.com> wrote:
> > > Problem was introduced by patch (2011-09-19):
> > >   Create: Allow to create two volumes of different sizes within one container
> > >
> > > To resolve problem check requested disks number not against all disks
> > > recorded in metadata, but against disks number set in array map structure.
> > >
> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > ---
> > >
> > >  super-intel.c |   24 ++++++++++++++++++++----
> > >  1 files changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/super-intel.c b/super-intel.c
> > > index 5e1d278..3c10d29 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -5318,10 +5318,26 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
> > >
> > >        mpb = super->anchor;
> > >
> > > -       if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
> > > -               fprintf(stderr, Name ": the option-rom requires all "
> > > -                       "member disks to be a member of all volumes.\n");
> > > -               return 0;
> > > +       /* check if currently verified volume spans the same disks number
> > > +        * as all other arrays that belongs to metadata.
> > > +        * Do not allow to create volume with different disks number
> > > +        * than curently is used.
> > > +        */
> > > +       for (i = 0; i < mpb->num_raid_devs; i++) {
> > > +               /* There is any already existing array in metadata
> > > +                */
> > > +               struct imsm_dev *dev = get_imsm_dev(super, i);
> > > +               struct imsm_map *map = NULL;
> > > +               if (dev)
> > > +                       map = get_imsm_map(dev, MAP_0);
> > > +               if (map) {
> > > +                       if (raiddisks != map->num_members) {
> > > +                               fprintf(stderr, Name ": the option-rom requires"
> > > +                                       " all member disks to be a member of "
> > > +                                       "all volumes.\n");
> > > +                               return 0;
> > > +                       }
> > > +               }
> >
> > I'd prefer to move this check back to the other 'orom' checks in
> > validate_geometry_imsm_volume(), and make it dependent on the presence
> > of the orom.
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index 07d47b5..9885b98 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -5122,12 +5122,6 @@ static int validate_geometry_imsm_volume(struct
> > supertype *st, int level,
> >       if (!super)
> >               return 0;
> >
> > -     if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
> > -             fprintf(stderr, Name ": the option-rom requires all "
> > -                     "member disks to be a member of all volumes.\n");
> > -             return 0;
> > -     }
> > -
> >       if (!validate_geometry_imsm_orom(super, level, layout, raiddisks,
> > chunk, verbose)) {
> >               fprintf(stderr, Name ": RAID gemetry validation failed. "
> >                       "Cannot proceed with the action(s).\n");
> > @@ -5206,6 +5200,11 @@ static int validate_geometry_imsm_volume(struct
> > supertype *st, int level,
> >               fprintf(stderr, Name ": The option-rom requires all member"
> >                       " disks to be a member of all volumes\n");
> >               return 0;
> > +     } else if (super->orom && mpb->num_raid_devs > 0 &&
> > +                mpb->num_disks != raiddisks) {
> > +             fprintf(stderr, Name ": The option-rom requires all member"
> > +                     " disks to be a member of all volumes\n");
> > +             return 0;
> >       }
> >
> >       /* retrieve the largest free space block */
>
>
> Hi Dan,
>  I think you are presenting this as an alternate - is that correct?
Yes.
> It seems a lit simpler - does it do enough?
I believe so, I'd like Adam to confirm.

I'm not sure if checking mpb->num_disks != raiddisks is enough /or it is rather too much/.
In disk list could be different number of members than in map (e.g. rebuild when disks list is longer).
It was UT08* failure cause and I thing using maps length is better.

I'm not sure, if making it dependent on the presence of the orom makes us happy from compatibility point of view.
We shouldn't make metadata OROM incompatible. Every array can be mounted on OROM platform later, what scenario for such case?

BR
Adam

(forgive brevity replying from phone)
--
Dan
--
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 3/3] imsm: FIX: UT '08imsm-overlap' fails

[Williams, Dan J]

2011/12/15 Kwolek, Adam <adam.kwolek@intel.com>:
>
>
> From: Williams, Dan J [mailto:dan.j.williams@intel.com]
> Sent: Thursday, December 15, 2011 6:16 AM
> To: NeilBrown
> Cc: Labun, Marcin; Ciechanowski, Ed; linux-raid@vger.kernel.org; Kwolek, Adam
> Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails
>
>
> On Dec 14, 2011 8:03 PM, "NeilBrown" <neilb@suse.de> wrote:
>>
>> On Wed, 14 Dec 2011 10:21:07 -0800 "Williams, Dan J"
>> <dan.j.williams@intel.com> wrote:
>>
>> > On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek <adam.kwolek@intel.com> wrote:
>> > > Problem was introduced by patch (2011-09-19):
>> > >   Create: Allow to create two volumes of different sizes within one container
>> > >
>> > > To resolve problem check requested disks number not against all disks
>> > > recorded in metadata, but against disks number set in array map structure.
>> > >
>> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
>> > > ---
>> > >
>> > >  super-intel.c |   24 ++++++++++++++++++++----
>> > >  1 files changed, 20 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/super-intel.c b/super-intel.c
>> > > index 5e1d278..3c10d29 100644
>> > > --- a/super-intel.c
>> > > +++ b/super-intel.c
>> > > @@ -5318,10 +5318,26 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
>> > >
>> > >        mpb = super->anchor;
>> > >
>> > > -       if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
>> > > -               fprintf(stderr, Name ": the option-rom requires all "
>> > > -                       "member disks to be a member of all volumes.\n");
>> > > -               return 0;
>> > > +       /* check if currently verified volume spans the same disks number
>> > > +        * as all other arrays that belongs to metadata.
>> > > +        * Do not allow to create volume with different disks number
>> > > +        * than curently is used.
>> > > +        */
>> > > +       for (i = 0; i < mpb->num_raid_devs; i++) {
>> > > +               /* There is any already existing array in metadata
>> > > +                */
>> > > +               struct imsm_dev *dev = get_imsm_dev(super, i);
>> > > +               struct imsm_map *map = NULL;
>> > > +               if (dev)
>> > > +                       map = get_imsm_map(dev, MAP_0);
>> > > +               if (map) {
>> > > +                       if (raiddisks != map->num_members) {
>> > > +                               fprintf(stderr, Name ": the option-rom requires"
>> > > +                                       " all member disks to be a member of "
>> > > +                                       "all volumes.\n");
>> > > +                               return 0;
>> > > +                       }
>> > > +               }
>> >
>> > I'd prefer to move this check back to the other 'orom' checks in
>> > validate_geometry_imsm_volume(), and make it dependent on the presence
>> > of the orom.
>> >
>> > diff --git a/super-intel.c b/super-intel.c
>> > index 07d47b5..9885b98 100644
>> > --- a/super-intel.c
>> > +++ b/super-intel.c
>> > @@ -5122,12 +5122,6 @@ static int validate_geometry_imsm_volume(struct
>> > supertype *st, int level,
>> >       if (!super)
>> >               return 0;
>> >
>> > -     if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
>> > -             fprintf(stderr, Name ": the option-rom requires all "
>> > -                     "member disks to be a member of all volumes.\n");
>> > -             return 0;
>> > -     }
>> > -
>> >       if (!validate_geometry_imsm_orom(super, level, layout, raiddisks,
>> > chunk, verbose)) {
>> >               fprintf(stderr, Name ": RAID gemetry validation failed. "
>> >                       "Cannot proceed with the action(s).\n");
>> > @@ -5206,6 +5200,11 @@ static int validate_geometry_imsm_volume(struct
>> > supertype *st, int level,
>> >               fprintf(stderr, Name ": The option-rom requires all member"
>> >                       " disks to be a member of all volumes\n");
>> >               return 0;
>> > +     } else if (super->orom && mpb->num_raid_devs > 0 &&
>> > +                mpb->num_disks != raiddisks) {
>> > +             fprintf(stderr, Name ": The option-rom requires all member"
>> > +                     " disks to be a member of all volumes\n");
>> > +             return 0;
>> >       }
>> >
>> >       /* retrieve the largest free space block */
>>
>>
>> Hi Dan,
>>  I think you are presenting this as an alternate - is that correct?
> Yes.
>> It seems a lit simpler - does it do enough?
> I believe so, I'd like Adam to confirm.
>
> I'm not sure if checking mpb->num_disks != raiddisks is enough /or it is rather too much/.
> In disk list could be different number of members than in map (e.g. rebuild when disks list is longer).

Yes but num_disks should always be authoritative for the container.

> It was UT08* failure cause and I thing using maps length is better.
>
> I'm not sure, if making it dependent on the presence of the orom makes us happy from compatibility point of view.

That's exactly the point this restriction only matters when the orom
is present.  Which is why 08imsm-overlap specifies IMSM_NO_PLATFORM=1.
 Bypassing this restriction allows more stressful testing of the
auto-layout code.

> We shouldn't make metadata OROM incompatible. Every array can be mounted on OROM platform later, what scenario for such case?

Specifying IMSM_NO_PLATFORM means all orom / platform compatibility
checks are turned off.
--
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 3/3] imsm: FIX: UT '08imsm-overlap' fails

[Kwolek, Adam]

> -----Original Message-----
> From: Williams, Dan J [mailto:dan.j.williams@intel.com]
> Sent: Friday, December 16, 2011 5:19 AM
> To: Kwolek, Adam
> Cc: NeilBrown; Labun, Marcin; Ciechanowski, Ed; linux-raid@vger.kernel.org
> Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails
> 
> 2011/12/15 Kwolek, Adam <adam.kwolek@intel.com>:
> >
> >
> > From: Williams, Dan J [mailto:dan.j.williams@intel.com]
> > Sent: Thursday, December 15, 2011 6:16 AM
> > To: NeilBrown
> > Cc: Labun, Marcin; Ciechanowski, Ed; linux-raid@vger.kernel.org;
> > Kwolek, Adam
> > Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails
> >
> >
> > On Dec 14, 2011 8:03 PM, "NeilBrown" <neilb@suse.de> wrote:
> >>
> >> On Wed, 14 Dec 2011 10:21:07 -0800 "Williams, Dan J"
> >> <dan.j.williams@intel.com> wrote:
> >>
> >> > On Wed, Dec 14, 2011 at 7:07 AM, Adam Kwolek
> <adam.kwolek@intel.com> wrote:
> >> > > Problem was introduced by patch (2011-09-19):
> >> > >   Create: Allow to create two volumes of different sizes within
> >> > > one container
> >> > >
> >> > > To resolve problem check requested disks number not against all
> >> > > disks recorded in metadata, but against disks number set in array map
> structure.
> >> > >
> >> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> >> > > ---
> >> > >
> >> > >  super-intel.c |   24 ++++++++++++++++++++----
> >> > >  1 files changed, 20 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/super-intel.c b/super-intel.c index 5e1d278..3c10d29
> >> > > 100644
> >> > > --- a/super-intel.c
> >> > > +++ b/super-intel.c
> >> > > @@ -5318,10 +5318,26 @@ static int
> >> > > validate_geometry_imsm_volume(struct supertype *st, int level,
> >> > >
> >> > >        mpb = super->anchor;
> >> > >
> >> > > -       if (mpb->num_raid_devs > 0 && mpb->num_disks !=
> >> > > raiddisks) {
> >> > > -               fprintf(stderr, Name ": the option-rom requires all "
> >> > > -                       "member disks to be a member of all
> >> > > volumes.\n");
> >> > > -               return 0;
> >> > > +       /* check if currently verified volume spans the same
> >> > > + disks number
> >> > > +        * as all other arrays that belongs to metadata.
> >> > > +        * Do not allow to create volume with different disks
> >> > > + number
> >> > > +        * than curently is used.
> >> > > +        */
> >> > > +       for (i = 0; i < mpb->num_raid_devs; i++) {
> >> > > +               /* There is any already existing array in
> >> > > + metadata
> >> > > +                */
> >> > > +               struct imsm_dev *dev = get_imsm_dev(super, i);
> >> > > +               struct imsm_map *map = NULL;
> >> > > +               if (dev)
> >> > > +                       map = get_imsm_map(dev, MAP_0);
> >> > > +               if (map) {
> >> > > +                       if (raiddisks != map->num_members) {
> >> > > +                               fprintf(stderr, Name ": the option-rom requires"
> >> > > +                                       " all member disks to be a member of "
> >> > > +                                       "all volumes.\n");
> >> > > +                               return 0;
> >> > > +                       }
> >> > > +               }
> >> >
> >> > I'd prefer to move this check back to the other 'orom' checks in
> >> > validate_geometry_imsm_volume(), and make it dependent on the
> >> > presence of the orom.
> >> >
> >> > diff --git a/super-intel.c b/super-intel.c index 07d47b5..9885b98
> >> > 100644
> >> > --- a/super-intel.c
> >> > +++ b/super-intel.c
> >> > @@ -5122,12 +5122,6 @@ static int
> >> > validate_geometry_imsm_volume(struct
> >> > supertype *st, int level,
> >> >       if (!super)
> >> >               return 0;
> >> >
> >> > -     if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
> >> > -             fprintf(stderr, Name ": the option-rom requires all "
> >> > -                     "member disks to be a member of all
> >> > volumes.\n");
> >> > -             return 0;
> >> > -     }
> >> > -
> >> >       if (!validate_geometry_imsm_orom(super, level, layout,
> >> > raiddisks, chunk, verbose)) {
> >> >               fprintf(stderr, Name ": RAID gemetry validation failed. "
> >> >                       "Cannot proceed with the action(s).\n"); @@
> >> > -5206,6 +5200,11 @@ static int validate_geometry_imsm_volume(struct
> >> > supertype *st, int level,
> >> >               fprintf(stderr, Name ": The option-rom requires all member"
> >> >                       " disks to be a member of all volumes\n");
> >> >               return 0;
> >> > +     } else if (super->orom && mpb->num_raid_devs > 0 &&
> >> > +                mpb->num_disks != raiddisks) {
> >> > +             fprintf(stderr, Name ": The option-rom requires all member"
> >> > +                     " disks to be a member of all volumes\n");
> >> > +             return 0;
> >> >       }
> >> >
> >> >       /* retrieve the largest free space block */
> >>
> >>
> >> Hi Dan,
> >>  I think you are presenting this as an alternate - is that correct?
> > Yes.
> >> It seems a lit simpler - does it do enough?
> > I believe so, I'd like Adam to confirm.
> >
> > I'm not sure if checking mpb->num_disks != raiddisks is enough /or it is
> rather too much/.
> > In disk list could be different number of members than in map (e.g. rebuild
> when disks list is longer).
> 
> Yes but num_disks should always be authoritative for the container.
> 
> > It was UT08* failure cause and I thing using maps length is better.
> >
> > I'm not sure, if making it dependent on the presence of the orom makes us
> happy from compatibility point of view.
> 
> That's exactly the point this restriction only matters when the orom is
> present.  Which is why 08imsm-overlap specifies IMSM_NO_PLATFORM=1.
>  Bypassing this restriction allows more stressful testing of the auto-layout
> code.
> 
> > We shouldn't make metadata OROM incompatible. Every array can be
> mounted on OROM platform later, what scenario for such case?
> 
> Specifying IMSM_NO_PLATFORM means all orom / platform compatibility
> checks are turned off.

I've checked, you are right. OROM check will resolve problem.
Do you post your patch or I should do it? We are short of time regarding Neil's plans about v3.2.3

BR
Adam
--
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 3/3] imsm: FIX: UT '08imsm-overlap' fails

[NeilBrown]

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

On Fri, 16 Dec 2011 08:32:07 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:


> I've checked, you are right. OROM check will resolve problem.
> Do you post your patch or I should do it? We are short of time regarding Neil's plans about v3.2.3
> 
>

I've created a commit myself.  See below.

Thanks,
NeilBrown

From 5fe62b9455b6b43f050f3a52610ce1048a44623c Mon Sep 17 00:00:00 2001
From: "Williams, Dan J" <dan.j.williams@intel.com>
Date: Wed, 14 Dec 2011 18:21:07 -0800
Subject: [PATCH] imsm: FIX: UT '08imsm-overlap' fails

Make test for all sub arrays having the same number of devices
dependant on the option ROM requirements being checked.
08imsm-overlap disables the OROM check but then fails because this
test causes it to.

Reported-by: Adam Kwolek <adam.kwolek@intel.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/super-intel.c b/super-intel.c
index 9074485..0e77537 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5314,12 +5314,6 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
 
 	mpb = super->anchor;
 
-	if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
-		fprintf(stderr, Name ": the option-rom requires all "
-			"member disks to be a member of all volumes.\n");
-		return 0;
-	}
-
 	if (!validate_geometry_imsm_orom(super, level, layout, raiddisks, chunk, verbose)) {
 		fprintf(stderr, Name ": RAID gemetry validation failed. "
 			"Cannot proceed with the action(s).\n");
@@ -5398,6 +5392,11 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
 		fprintf(stderr, Name ": The option-rom requires all member"
 			" disks to be a member of all volumes\n");
 		return 0;
+	} else if (super->orom && mpb->num_raid_devs > 0 &&
+		   mpb->num_disks != raiddisks) {
+		fprintf(stderr, Name ": The option-rom requires all member"
+			" disks to be a member of all volumes\n");
+		return 0;
 	}
 
 	/* retrieve the largest free space block */

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

RE: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Tuesday, December 20, 2011 12:39 AM
> To: Kwolek, Adam
> Cc: Williams, Dan J; Labun, Marcin; Ciechanowski, Ed; linux-
> raid@vger.kernel.org
> Subject: Re: [PATCH 3/3] imsm: FIX: UT '08imsm-overlap' fails
> 
> On Fri, 16 Dec 2011 08:32:07 +0000 "Kwolek, Adam"
> <adam.kwolek@intel.com>
> wrote:
> 
> 
> > I've checked, you are right. OROM check will resolve problem.
> > Do you post your patch or I should do it? We are short of time
> > regarding Neil's plans about v3.2.3
> >
> >
> 
> I've created a commit myself.  See below.


Thanks :)

> 
> Thanks,
> NeilBrown
> 
> From 5fe62b9455b6b43f050f3a52610ce1048a44623c Mon Sep 17 00:00:00
> 2001
> From: "Williams, Dan J" <dan.j.williams@intel.com>
> Date: Wed, 14 Dec 2011 18:21:07 -0800
> Subject: [PATCH] imsm: FIX: UT '08imsm-overlap' fails
> 
> Make test for all sub arrays having the same number of devices dependant
> on the option ROM requirements being checked.
> 08imsm-overlap disables the OROM check but then fails because this test
> causes it to.
> 
> Reported-by: Adam Kwolek <adam.kwolek@intel.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/super-intel.c b/super-intel.c index 9074485..0e77537 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5314,12 +5314,6 @@ static int validate_geometry_imsm_volume(struct
> supertype *st, int level,
> 
>  	mpb = super->anchor;
> 
> -	if (mpb->num_raid_devs > 0 && mpb->num_disks != raiddisks) {
> -		fprintf(stderr, Name ": the option-rom requires all "
> -			"member disks to be a member of all volumes.\n");
> -		return 0;
> -	}
> -
>  	if (!validate_geometry_imsm_orom(super, level, layout, raiddisks,
> chunk, verbose)) {
>  		fprintf(stderr, Name ": RAID gemetry validation failed. "
>  			"Cannot proceed with the action(s).\n"); @@ -5398,6
> +5392,11 @@ static int validate_geometry_imsm_volume(struct supertype
> *st, int level,
>  		fprintf(stderr, Name ": The option-rom requires all member"
>  			" disks to be a member of all volumes\n");
>  		return 0;
> +	} else if (super->orom && mpb->num_raid_devs > 0 &&
> +		   mpb->num_disks != raiddisks) {
> +		fprintf(stderr, Name ": The option-rom requires all member"
> +			" disks to be a member of all volumes\n");
> +		return 0;
>  	}
> 
>  	/* retrieve the largest free space block */