Skip to contentSkip to conversation

Misc fixes for mdadm-2.6.7.1

PATCH In review Doug Ledford 2 participants 8 messages Oct 29, 2008 – Oct 30, 2008 MD RAID

lore
Share & tools
lore
#0 lore raw

cover Misc fixes for mdadm-2.6.7.1

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(-)
#1 lore raw

PATCH 1/3 Fix bad metadata formatting

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.

---
 Detail.c |    6 +++---
 super0.c |    5 ++++-
 super1.c |    3 +++
 3 files changed, 10 insertions(+), 4 deletions(-)
Detail.c +3 -3
index 2fb59a3..715d4d0 100644
@@ -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;
super0.c +4 -1
index 7e81482..42d8659 100644
@@ -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 ||
super1.c +3 -0
index e1d0219..a81ee6f 100644
@@ -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;
#2 lore raw

PATCH 2/3 Fix NULL pointer oops

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.

---
 Incremental.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
Incremental.c +10 -4
index 0fb9afd..9c6524f 100644
@@ -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;
#3 lore raw

PATCH 3/3 Make incremental mode work with partitionable arrays

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.

---
 Incremental.c |  122 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 mdopen.c      |    4 +-
 util.c        |    2 +-
 3 files changed, 117 insertions(+), 11 deletions(-)
Incremental.c +114 -8
index 9c6524f..806d192 100644
@@ -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, ':');
mdopen.c +2 -2
index 448a9eb..36233cf 100644
@@ -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;
util.c +1 -1
index 75f3706..baa1690 100644
@@ -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)
#4 lore raw
In reply to Doug Ledford
Quoted 9 lines · On Wednesday October 29, [email protected] 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 <[email protected]>
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.

---
 Incremental.c |   10 +++++-----
 util.c        |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)
Incremental.c +5 -5
index 9c6524f..5d26b77 100644
@@ -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. */
util.c +0 -0
index 2d51de0..a50036c 100644

--- a/util.c
From 4ef2f11e28800373f045e1f0c1336f13f89b79c9 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
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.

---
 Incremental.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
Incremental.c +10 -4
index 5d26b77..d61518a 100644
@@ -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)
Signature · 33 lines
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 <[email protected]>
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 <[email protected]>
---
 Incremental.c |   18 ++++++++++++------
 mdopen.c      |    2 +-
 2 files changed, 13 insertions(+), 7 deletions(-)
Incremental.c +12 -6
index d61518a..7148a73 100644
@@ -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,
mdopen.c +1 -1
index 4fbcb48..9250e4b 100644
@@ -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)
Signature · 1 line
1.5.6.5
#5 lore raw
Quoted 2 lines · Neil Brown

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:

Incremental.c int Incremental(char *devname, int verbose, int runstop, · line 225

Quoted 2 lines
Incremental.c +2 -0
+	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.

Signature · 6 lines
Doug Ledford <[email protected]>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband
#6 lore raw
Quoted 19 lines · On Wednesday October 29, [email protected] wrote:
Quoted 2 lines · Neil Brown

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:

Incremental.c int Incremental(char *devname, int verbose, int runstop, · line 225

Quoted 2 lines
Incremental.c +2 -0
+	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

Quoted 10 lines

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

#7 lore raw
Quoted 3 lines · Neil Brown

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.

Quoted 4 lines

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.

Quoted 6 lines

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 ;-)

Quoted 6 lines

Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 6 lines
Doug Ledford <[email protected]>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

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