[Patch mdadm-2.6.7.1 0/3] Misc fixes for mdadm-2.6.7.1

[Patch mdadm-2.6.7.1 0/3] Misc fixes for mdadm-2.6.7.1

[Doug Ledford]

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


This can also be pulled directly from

git://fedorapeople.org/~dledford/mdadm.git for-neil

Doug Ledford (3):
  Fix bad metadata formatting
  Fix NULL pointer oops
  Make incremental mode work with partitionable arrays

 Detail.c      |    6 +-
 Incremental.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 mdopen.c      |    4 +-
 super0.c      |    5 ++-
 super1.c      |    3 +
 util.c        |    2 +-
 6 files changed, 137 insertions(+), 19 deletions(-)

[Patch mdadm-2.6.7.1 1/3] Fix bad metadata formatting

[Doug Ledford]

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


Certain operations (Detail.c mainly) would print out the metadata of
an array in a format that the scan operation in super0.c and super1.c
would later reject as unknown when it was found in the mdadm.conf file.
Use a consistent format, but also modify the super0 and super1 match
methods to accept the other format without complaint.

Signed-off-by: Doug Ledford <dledford@redhat.com>
---
 Detail.c |    6 +++---
 super0.c |    5 ++++-
 super1.c |    3 +++
 3 files changed, 10 insertions(+), 4 deletions(-)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: c82952743d7177261512b107adcb101ecd035a21.diff --]
[-- Type: text/x-patch; name="c82952743d7177261512b107adcb101ecd035a21.diff", Size: 2484 bytes --]

diff --git a/Detail.c b/Detail.c
index 2fb59a3..715d4d0 100644
--- a/Detail.c
+++ b/Detail.c
@@ -138,7 +138,7 @@ int Detail(char *dev, int brief, int export, int test, char *homehost)
 		if (sra && sra->array.major_version < 0)
 			printf("MD_METADATA=%s\n", sra->text_version);
 		else
-			printf("MD_METADATA=%02d.%02d\n",
+			printf("MD_METADATA=%d.%02d\n",
 			       array.major_version, array.minor_version);
 
 		if (st && st->sb)
@@ -153,7 +153,7 @@ int Detail(char *dev, int brief, int export, int test, char *homehost)
 		if (sra && sra->array.major_version < 0)
 			printf(" metadata=%s", sra->text_version);
 		else
-			printf(" metadata=%02d.%02d",
+			printf(" metadata=%d.%02d",
 			       array.major_version, array.minor_version);
 	} else {
 		mdu_bitmap_file_t bmf;
@@ -175,7 +175,7 @@ int Detail(char *dev, int brief, int export, int test, char *homehost)
 		if (sra && sra->array.major_version < 0)
 			printf("        Version : %s\n", sra->text_version);
 		else
-			printf("        Version : %02d.%02d\n",
+			printf("        Version : %d.%02d\n",
 			       array.major_version, array.minor_version);
 
 		atime = array.ctime;
diff --git a/super0.c b/super0.c
index 7e81482..42d8659 100644
--- a/super0.c
+++ b/super0.c
@@ -93,7 +93,7 @@ static void examine_super0(struct supertype *st, char *homehost)
 	char *c;
 
 	printf("          Magic : %08x\n", sb->md_magic);
-	printf("        Version : %02d.%02d.%02d\n", sb->major_version, sb->minor_version,
+	printf("        Version : %d.%02d.%02d\n", sb->major_version, sb->minor_version,
 	       sb->patch_version);
 	if (sb->minor_version >= 90) {
 		printf("           UUID : %08x:%08x:%08x:%08x", sb->set_uuid0, sb->set_uuid1,
@@ -847,6 +847,9 @@ static struct supertype *match_metadata_desc0(char *arg)
 	st->minor_version = 90;
 	st->max_devs = MD_SB_DISKS;
 	st->sb = NULL;
+	/* Eliminate pointless leading 0 from some versions of mdadm -D */
+	if (strncmp(arg, "00.", 3) == 0)
+		arg++;
 	if (strcmp(arg, "0") == 0 ||
 	    strcmp(arg, "0.90") == 0 ||
 	    strcmp(arg, "0.91") == 0 ||
diff --git a/super1.c b/super1.c
index e1d0219..a81ee6f 100644
--- a/super1.c
+++ b/super1.c
@@ -1186,6 +1186,9 @@ static struct supertype *match_metadata_desc1(char *arg)
 	st->ss = &super1;
 	st->max_devs = 384;
 	st->sb = NULL;
+	/* Eliminate pointless leading 0 from some versions of mdadm -D */
+	if (strncmp(arg, "01.", 3) == 0)
+		arg++;
 	if (strcmp(arg, "1.0") == 0) {
 		st->minor_version = 0;
 		return st;

[Patch mdadm-2.6.7.1 2/3] Fix NULL pointer oops

[Doug Ledford]

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


RAID10 is the only raid level that uses the avail char array pointer
during the enough() operation, so it was the only one that saw this.
The code in incremental assumes unconditionally that count_active will
allocate the avail char array, that it might be used by enough, and that
it will need to be freed afterward.  Once you make count_active actually
do that, then the oops goes away.

Signed-off-by: Doug Ledford <dledford@redhat.com>
---
 Incremental.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 7c468095f2afc12245eb2988457f87440b3882e9.diff --]
[-- Type: text/x-patch; name="7c468095f2afc12245eb2988457f87440b3882e9.diff", Size: 726 bytes --]

diff --git a/Incremental.c b/Incremental.c
index 0fb9afd..9c6524f 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -543,12 +543,18 @@ static int count_active(struct supertype *st, int mdfd, char **availp,
 		if (ok != 0)
 			continue;
 		st->ss->getinfo_super(st, &info);
+		if (!avail) {
+			avail = malloc(info.array.raid_disks);
+			if (!avail) {
+				fprintf(stderr, Name ": out of memory.\n");
+				exit(1);
+			}
+			memset(avail, 0, info.array.raid_disks);
+			*availp = avail;
+		}
+
 		if (info.disk.state & (1<<MD_DISK_SYNC))
 		{
-			if (avail == NULL) {
-				avail = malloc(info.array.raid_disks);
-				memset(avail, 0, info.array.raid_disks);
-			}
 			if (cnt == 0) {
 				cnt++;
 				max_events = info.events;

[Patch mdadm-2.6.7.1 3/3] Make incremental mode work with partitionable arrays

[Doug Ledford]

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


There were multiple problems with incremental mode and partitionable
arrays.  Change the logic to pay more attention to what it finds in
mdadm.conf when setting up incremental arrays.  Modify is_standard to
differentiate between a required partitioned name format and an optionally
partitioned named format.  Modify open_mddev() to know about change to
is_standard.  Make Incremental differentiate between create default
autof setting, command line autof setting, and array specific autof
setting, with order of preference being command line, conf file, default.
Disable the homehost test in the event there is no match for an array
so we can hotplug new arrays into a running machine without them having
to exist in mdadm.conf first (a random array number will be picked for
the hot plugged array).  Honor any name from mdadm.conf that matches the
UUID of a device, not just standard names.  Verify that the array name
as matched in mdadm.conf allows the requested mode of operation and reject
attempts to start non-partitionable arrays as partitioned and vice versa.

Signed-off-by: Doug Ledford <dledford@redhat.com>
---
 Incremental.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 mdopen.c      |    4 +-
 util.c        |    2 +-
 3 files changed, 117 insertions(+), 11 deletions(-)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: c9389e3df675e805befb98a05927318f7f206031.diff --]
[-- Type: text/x-patch; name="c9389e3df675e805befb98a05927318f7f206031.diff", Size: 5607 bytes --]

diff --git a/Incremental.c b/Incremental.c
index 9c6524f..806d192 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -87,9 +87,6 @@ int Incremental(char *devname, int verbose, int runstop,
 
 	struct createinfo *ci = conf_get_create_info();
 
-	if (autof == 0)
-		autof = ci->autof;
-
 	/* 1/ Check if devices is permitted by mdadm.conf */
 
 	if (!conf_test_dev(devname)) {
@@ -204,6 +201,10 @@ int Incremental(char *devname, int verbose, int runstop,
 	}
 
 	/* 3a/ if not, check for homehost match.  If no match, reject. */
+	/* We want to support hot plugged arrays that aren't listed in
+	 * /etc/mdadm.conf, so we need to skip the homehost test and allow
+	 * unmatched devices through so we can simply pick a random entry
+	 * to start them under
 	if (!match) {
 		if (homehost == NULL ||
 		    st->ss->match_home(st, homehost) == 0) {
@@ -212,23 +213,128 @@ int Incremental(char *devname, int verbose, int runstop,
 	      ": not found in mdadm.conf and not identified by homehost.\n");
 			return 2;
 		}
-	}
+	} */
 	/* 4/ Determine device number. */
-	/* - If in mdadm.conf with std name, use that */
+	/* - If in mdadm.conf with any name, use that */
 	/* - UUID in /var/run/mdadm.map  use that */
 	/* - If name is suggestive, use that. unless in use with */
 	/*           different uuid. */
 	/* - Choose a free, high number. */
 	/* - Use a partitioned device unless strong suggestion not to. */
 	/*         e.g. auto=md */
-	if (match && is_standard(match->devname, &devnum))
-		/* We have devnum now */;
-	else if ((mp = map_by_uuid(&map, info.uuid)) != NULL)
+	if (match) {
+		int use_partitions;
+		if (autof != 0)
+			/* command line overrides config file */
+			match->autof = autof;
+		else if (match->autof == 0)
+			/* no specific autof setting in config for this */
+			/* array, so use the ci->autof instead */
+			match->autof = ci->autof;
+		switch(is_standard(match->devname, &devnum)) {
+		case 0: /* non-standard name */
+			/* Need to find a free devnum to use */
+			/* Need to know if we are supposed to be partitioned */
+			/* or not to find a free devnum in the right major */
+			switch(match->autof & 0x7) {
+			case 3:
+			case 5:
+				autof = match->autof;
+			case 0: /* this is what you get when you */
+				/* don't specify anything, leave it */
+				/* defaulting to non-partitioned for */
+				/* back compatibility */
+				use_partitions = 0;
+				break;
+			default:
+				autof = match->autof;
+				use_partitions = 1;
+			}
+			/* If we already have an assigned devnum find it */
+			if ((mp = map_by_uuid(&map, info.uuid)) != NULL)
+				devnum = mp->devnum;
+			else
+				devnum = find_free_devnum(use_partitions);
+
+			if (devnum == NoMdDev) {
+				fprintf(stderr, Name
+					": No spare md devices!!\n");
+				return 2;
+			}
+			break;
+		case -1: /* standard, non-partitioned name */
+			switch(match->autof & 0x7) {
+			case 3:
+			case 5:
+			case 0:
+				/* all good */
+				use_partitions = 0;
+				break;
+			default:
+				/* oops, attempting to set things to */
+				/* an invalid state */
+				fprintf(stderr, Name
+				": can't be a partitioned device by naming standards!!\n");
+				return 2;
+			}
+			break;
+		case 1: /* standard, partitioned name */
+			switch(match->autof & 0x7) {
+			case 3:
+			case 5:
+				/* oops, attempting to set things to */
+				/* an invalid state */
+				fprintf(stderr, Name
+				": must be a partitioned device by naming standards!!\n");
+				return 2;
+			case 0:
+				/* we haven't been told how many */
+				/* partitions to make on either the */
+				/* command line or the config file, */
+				/* set to the default and enable */
+				/* partitioned mode due to our name */
+				autof = match->autof = (4 << 3) | 2;
+			default:
+				/* all good */
+				use_partitions = 1;
+				break;
+			}
+			break;
+		case 2: /* may be either partitioned or not, devnum is set */
+			switch(match->autof & 0x7) {
+			case 0:
+				/* not specified in any way, default to */
+				/* unpartitioned for back compatibility */
+			case 1:
+			case 3:
+			case 5:
+				use_partitions = 0;
+				break;
+			case 2:
+				/* partitioned, but we didn't get a number */
+				/* of parititions, so use the default */
+				match->autof = autof |= 4 << 3;
+			case 4:
+			case 6:
+				/* the number of partitions is already in */
+				/* autof */
+				use_partitions = 1;
+				break;
+			}
+			break;
+		}
+		/* We have devnum now */
+		if (use_partitions && devnum > 0)
+			devnum = (-1-devnum);
+	} else if ((mp = map_by_uuid(&map, info.uuid)) != NULL)
 		devnum = mp->devnum;
 	else {
 		/* Have to guess a bit. */
 		int use_partitions = 1;
 		char *np, *ep;
+
+		if (autof == 0 && ci->autof)
+			autof = ci->autof;
 		if ((autof&7) == 3 || (autof&7) == 5)
 			use_partitions = 0;
 		np = strchr(info.name, ':');
diff --git a/mdopen.c b/mdopen.c
index 448a9eb..36233cf 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -147,8 +147,8 @@ int open_mddev(char *dev, int autof)
 				return -1;
 			}
 			break;
-		case 3: /* create md, reject std>0 */
-			if (std > 0) {
+		case 3: /* create md, reject std==1 */
+			if (std == 1) {
 				fprintf(stderr, Name ": that --auto option "
 				"not compatable with device named %s\n", dev);
 				return -1;
diff --git a/util.c b/util.c
index 75f3706..baa1690 100644
--- a/util.c
+++ b/util.c
@@ -396,7 +396,7 @@ int is_standard(char *dev, int *nump)
 	if (!d)
 		return 0;
 	if (strncmp(d, "/d",2)==0)
-		d += 2, type=1; /* /dev/md/dN{pM} */
+		d += 2, type=2; /* /dev/md/dN{pM} */
 	else if (strncmp(d, "/md_d", 5)==0)
 		d += 5, type=1; /* /dev/md_dNpM */
 	else if (strncmp(d, "/md", 3)==0)

Re: [Patch mdadm-2.6.7.1 0/3] Misc fixes for mdadm-2.6.7.1

[Neil Brown]

On Wednesday October 29, dledford@redhat.com wrote:
> 
> This can also be pulled directly from
> 
> git://fedorapeople.org/~dledford/mdadm.git for-neil
> 
> Doug Ledford (3):
>   Fix bad metadata formatting
>   Fix NULL pointer oops
>   Make incremental mode work with partitionable arrays


Thanks for these Doug.

The first two are definitely good and I've applied them.

The third I'm less comfortable with.  It combines a number of things
and I'm a bit confused by some.  So I've split it up a bit.

First there is the fix to use the autof setting from the ARRAY line in
mdadm.conf.  That is certainly a bug so I've got one patch for that.
Then there is the fact that the devnum returned by is_standard is used
even though it doesn't properly encode the 'partition-or-not'
information.  So I have a separate patch the fixes that bug.

Then there is the removal of the 'homehost' test.  I'm not comfortable
with simply removing it.  So I have a patch which changes the homehost
test so that instead of causing failure, it causes the name to be
ignored, and the array to be assembled 'read-auto'.
These three patches are below (I'll push them out later today I
suspect).

That leaves a largish slab of your code that I wasn't really sure
what it was adding.
And there is the bit about /dev/md_d0 having a different meaning to
/dev/md/d0.  I have never seen any difference between those other than
the obvious syntax.  There was a comment in is_standard that made them
look slightly different.  That was unintentional and has been fixed.

If you still need something more that what my three patches provide,
please explain what I missed.

Thanks again!

NeilBrown


From 2b4ca8f079335c1b3f345ec13da58699aaa0269d Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Thu, 30 Oct 2008 09:34:04 +1100
Subject: [PATCH] Fix --incremental assembly of partitions arrays.

If incremental assembly finds an array mentioned in mdadm.conf,
with a 'standard partitioned' name like /dev/md_d0 or /dev/md/d0,
it will not create a partitioned array like it should.
This is because it mishandled the 'devnum' returned by
is_standard.
That is a devnum that does not have the partition-or-not encoded
into it.  So we need to check the actual return value of
is_standard and encode the partition-or-not info into the devnum.

Also fix a couple of comments.


Signed-off-by: NeilBrown <neilb@suse.de>
---
 Incremental.c |   10 +++++-----
 util.c        |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index 9c6524f..5d26b77 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -214,16 +214,16 @@ int Incremental(char *devname, int verbose, int runstop,
 		}
 	}
 	/* 4/ Determine device number. */
-	/* - If in mdadm.conf with std name, use that */
-	/* - UUID in /var/run/mdadm.map  use that */
+	/* - If in mdadm.conf with std name, get number from name. */
+	/* - UUID in /var/run/mdadm.map  get number from mapping */
 	/* - If name is suggestive, use that. unless in use with */
 	/*           different uuid. */
 	/* - Choose a free, high number. */
 	/* - Use a partitioned device unless strong suggestion not to. */
 	/*         e.g. auto=md */
-	if (match && is_standard(match->devname, &devnum))
-		/* We have devnum now */;
-	else if ((mp = map_by_uuid(&map, info.uuid)) != NULL)
+	if (match && (rv = is_standard(match->devname, &devnum))) {
+		devnum = (rv > 0) ? (-1-devnum) : devnum;
+	} else if ((mp = map_by_uuid(&map, info.uuid)) != NULL)
 		devnum = mp->devnum;
 	else {
 		/* Have to guess a bit. */
diff --git a/util.c b/util.c
index 2d51de0..a50036c 100644
--- a/util.c
From 4ef2f11e28800373f045e1f0c1336f13f89b79c9 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Thu, 30 Oct 2008 09:34:06 +1100
Subject: [PATCH] Incremental:  fix setting of 'autof' flag.

When doing auto-assembly, the 'autof' flag from array lines
in mdadm.conf was being ignored.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 Incremental.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index 5d26b77..d61518a 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -83,12 +83,8 @@ int Incremental(char *devname, int verbose, int runstop,
 	int dfd, mdfd;
 	char *avail;
 	int active_disks;
-
-
 	struct createinfo *ci = conf_get_create_info();
 
-	if (autof == 0)
-		autof = ci->autof;
 
 	/* 1/ Check if devices is permitted by mdadm.conf */
 
@@ -221,6 +217,16 @@ int Incremental(char *devname, int verbose, int runstop,
 	/* - Choose a free, high number. */
 	/* - Use a partitioned device unless strong suggestion not to. */
 	/*         e.g. auto=md */
+
+	/* There are three possible sources for 'autof':  command line,
+	 * ARRAY line in mdadm.conf, or CREATE line in mdadm.conf.
+	 * They have precedence in that order.
+	 */
+	if (autof == 0 && match)
+		autof = match->autof;
+	if (autof == 0)
+		autof = ci->autof;
+
 	if (match && (rv = is_standard(match->devname, &devnum))) {
 		devnum = (rv > 0) ? (-1-devnum) : devnum;
 	} else if ((mp = map_by_uuid(&map, info.uuid)) != NULL)
-- 
1.5.6.5

+++ b/util.c
@@ -398,7 +398,7 @@ int is_standard(char *dev, int *nump)
 	if (strncmp(d, "/d",2)==0)
 		d += 2, type=1; /* /dev/md/dN{pM} */
 	else if (strncmp(d, "/md_d", 5)==0)
-		d += 5, type=1; /* /dev/md_dNpM */
+		d += 5, type=1; /* /dev/md_dN{pM} */
 	else if (strncmp(d, "/md", 3)==0)
 		d += 3, type=-1; /* /dev/mdN */
 	else if (d-dev > 3 && strncmp(d-2, "md/", 3)==0)
-- 
1.5.6.5


From 7b403fef7e97c16e1eb63773a278eb65c6dfd9a8 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Thu, 30 Oct 2008 09:48:18 +1100
Subject: [PATCH] Incremental:  allow assembly of foreign array.

If a foreign (i.e. not known to be local) array is discovered
by --incremental assembly, we now assemble it.  However we ignore
any name information in the array so as not to potentially create
a name that conflict with a 'local' array.
Also, foreign arrays are always assembled 'read-auto' to avoid writing
anything until the array is actually used.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 Incremental.c |   18 ++++++++++++------
 mdopen.c      |    2 +-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index d61518a..7148a73 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -84,6 +84,7 @@ int Incremental(char *devname, int verbose, int runstop,
 	char *avail;
 	int active_disks;
 	struct createinfo *ci = conf_get_create_info();
+	char *name;
 
 
 	/* 1/ Check if devices is permitted by mdadm.conf */
@@ -199,14 +200,18 @@ int Incremental(char *devname, int verbose, int runstop,
 		match = array_list;
 	}
 
-	/* 3a/ if not, check for homehost match.  If no match, reject. */
+	/* 3a/ if not, check for homehost match.  If no match, continue
+	 * but don't trust the 'name' in the array. Thus a 'random' minor
+	 * number will be assigned, and the device name will be based
+	 * on that. */
+	name = info.name;
 	if (!match) {
 		if (homehost == NULL ||
 		    st->ss->match_home(st, homehost) == 0) {
 			if (verbose >= 0)
 				fprintf(stderr, Name
 	      ": not found in mdadm.conf and not identified by homehost.\n");
-			return 2;
+			name = NULL;
 		}
 	}
 	/* 4/ Determine device number. */
@@ -237,11 +242,11 @@ int Incremental(char *devname, int verbose, int runstop,
 		char *np, *ep;
 		if ((autof&7) == 3 || (autof&7) == 5)
 			use_partitions = 0;
-		np = strchr(info.name, ':');
+		np = name ? strchr(name, ':') : ":NONAME";
 		if (np)
 			np++;
 		else
-			np = info.name;
+			np = name;
 		devnum = strtoul(np, &ep, 10);
 		if (ep > np && *ep == 0) {
 			/* This is a number.  Let check that it is unused. */
@@ -264,7 +269,7 @@ int Incremental(char *devname, int verbose, int runstop,
 	}
 	mdfd = open_mddev_devnum(match ? match->devname : NULL,
 				 devnum,
-				 info.name,
+				 name,
 				 chosen_name, autof >> 3);
 	if (mdfd < 0) {
 		fprintf(stderr, Name ": failed to open %s: %s.\n",
@@ -452,7 +457,8 @@ int Incremental(char *devname, int verbose, int runstop,
 			close(bmfd);
 		}
 		sra = sysfs_read(mdfd, devnum, 0);
-		if (sra == NULL || active_disks >= info.array.working_disks)
+		if ((sra == NULL || active_disks >= info.array.working_disks)
+		    && name != NULL)
 			rv = ioctl(mdfd, RUN_ARRAY, NULL);
 		else
 			rv = sysfs_set_str(sra, NULL,
diff --git a/mdopen.c b/mdopen.c
index 4fbcb48..9250e4b 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -282,7 +282,7 @@ int open_mddev_devnum(char *devname, int devnum, char *name,
 
 	if (devname)
 		strcpy(chosen_name, devname);
-	else if (name && strchr(name,'/') == NULL) {
+	else if (name && *name && strchr(name,'/') == NULL) {
 		char *n = strchr(name, ':');
 		if (n) n++; else n = name;
 		if (isdigit(*n) && devnum < 0)
-- 
1.5.6.5

Re: [Patch mdadm-2.6.7.1 0/3] Misc fixes for mdadm-2.6.7.1

[Doug Ledford]

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

On Thu, 2008-10-30 at 09:56 +1100, Neil Brown wrote:
> That leaves a largish slab of your code that I wasn't really sure
> what it was adding.

It was dealing with the fact that is_standard() has more than just two
return values.  It has -1, 0, and 1 (and 2 in my patch, but you've
changed yours in such a way that it's not necessary any more).  That
makes this test fail for certain ARRAY lines:

> +	if (match && (rv = is_standard(match->devname, &devnum))) {
> +		devnum = (rv > 0) ? (-1-devnum) : devnum;

As an example, create an array that doesn't use standard syntax, in my
case I was testing with /dev/md/root as a partitioned array.  The array
line in my mdadm.conf looked like this:

ARRAY /dev/md/root level=raid1 num-devices=2 metadata=0.90 auto=mdp4
UUID=e38b03c0:444d484e:910e8462:063f083e

Given a command invocation of mdadm -I <device>, and that device's uuid
matches the above array line, you really want to use the information in
the ARRAY line whether is_standard returns -1, 0, or 1.  So, that big
multiswitch statement was the possible invocations of mdadm -I with a
device that matches the array UUID in the config file but uses a
non-standard name, eg someone did a create on /dev/md0, then a -Eb >>
mdadm.conf, then vi mdadm.conf and add auto=mdp4 to the ARRAY /dev/md0
line.  Obviously, we would have refused to create the array with that
name and that auto setting, so I was preserving that behavior at run
time too.  Hence the size of that section of code.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Patch mdadm-2.6.7.1 0/3] Misc fixes for mdadm-2.6.7.1

[Neil Brown]

On Wednesday October 29, dledford@redhat.com wrote:
> On Thu, 2008-10-30 at 09:56 +1100, Neil Brown wrote:
> > That leaves a largish slab of your code that I wasn't really sure
> > what it was adding.
> 
> It was dealing with the fact that is_standard() has more than just two
> return values.  It has -1, 0, and 1 (and 2 in my patch, but you've
> changed yours in such a way that it's not necessary any more).  That
> makes this test fail for certain ARRAY lines:
> 
> > +	if (match && (rv = is_standard(match->devname, &devnum))) {
> > +		devnum = (rv > 0) ? (-1-devnum) : devnum;
> 
> As an example, create an array that doesn't use standard syntax, in my
> case I was testing with /dev/md/root as a partitioned array.  The array
> line in my mdadm.conf looked like this:
> 
> ARRAY /dev/md/root level=raid1 num-devices=2 metadata=0.90 auto=mdp4
> UUID=e38b03c0:444d484e:910e8462:063f083e
> 
> Given a command invocation of mdadm -I <device>, and that device's uuid
> matches the above array line, you really want to use the information in
> the ARRAY line whether is_standard returns -1, 0, or 1.  So, that big
> multiswitch statement was the possible invocations of mdadm -I with a
> device that matches the array UUID in the config file but uses a
> non-standard name, eg someone did a create on /dev/md0, then a -Eb >>
> mdadm.conf, then vi mdadm.conf and add auto=mdp4 to the ARRAY /dev/md0
> line.  Obviously, we would have refused to create the array with that
> name and that auto setting, so I was preserving that behavior at run
> time too.  Hence the size of that section of code.

Ok..... I think I see what you are getting at.

So if the name "/dev/md0" is given in mdadm.conf, we can only create a
non-partitioned array.  But what do we do if 'autof' suggests that a
partitioned array should be created (auto=mdp4)?

I think that if the "auto=mdp4" was on the ARRAY line, then we want to
reject that as a config error.  But if the auto=mdp4 was on the
command line or the CREATE line, then the device name over-rides.
Does that seem reasonable?
That makes me wonder if we have the precedence order of auto= right.
Maybe the ARRAY line should override as it is specific to the array.
Then the command line is next important.  Then the CREATE is the last
default.   I think that is different to the order that you had.
Is there some particular reason that you though the command line
should override in the --incremental case?

Thanks,
NeilBrown

Re: [Patch mdadm-2.6.7.1 0/3] Misc fixes for mdadm-2.6.7.1

[Doug Ledford]

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

On Thu, 2008-10-30 at 12:46 +1100, Neil Brown wrote:
> So if the name "/dev/md0" is given in mdadm.conf, we can only create a
> non-partitioned array.  But what do we do if 'autof' suggests that a
> partitioned array should be created (auto=mdp4)?

With my patch that would have thrown a config error.

> I think that if the "auto=mdp4" was on the ARRAY line, then we want to
> reject that as a config error.  But if the auto=mdp4 was on the
> command line or the CREATE line, then the device name over-rides.
> Does that seem reasonable?

Sure.

> That makes me wonder if we have the precedence order of auto= right.
> Maybe the ARRAY line should override as it is specific to the array.
> Then the command line is next important.  Then the CREATE is the last
> default.   I think that is different to the order that you had.
> Is there some particular reason that you though the command line
> should override in the --incremental case?

Truly, I had forgotten my changes were going to be --incremental
specific and instead was thinking to myself "Gee, if someone wants to
assemble an array differently than normal, then let them go ahead by
specifying what they want on the command line" when that line of thought
would obviously apply to assemble mode, not incremental mode.  Maybe
it's because I was running incremental manually to test all this that
this thinko slipped by ;-)

> Thanks,
> NeilBrown
> --
> 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
-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]