[PATCH 1/2] select_devices: fix scanning of container members with dev list

[PATCH 1/2] select_devices: fix scanning of container members with dev list

[mwilck]

commit b3908491 "Detail: fix --brief --verbose" introduced a
problem when a mdadm.conf file generated with
"mdadm --Detail --brief --verbose" is later scanned with
"mdadm --Assemble --scan --config=mdadm.conf"

mdadm -Dbv will print a "devices" list now, but because the
container device is not in that list, it won't be considered
for assembly.

This patch fixes that by moving the test for member devices
further down, after the check for a container.

Signed-off-by: Martin Wilck <mwilck@arcor.de>
---
 Assemble.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index c927c20..1f023e8 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -170,13 +170,6 @@ static int select_devices(struct mddev_dev *devlist,
 		if (tmpdev->used > 1)
 			continue;
 
-		if (ident->devices &&
-		    !match_oneof(ident->devices, devname)) {
-			if (report_mismatch)
-				pr_err("%s is not one of %s\n", devname, ident->devices);
-			continue;
-		}
-
 		tst = dup_super(st);
 
 		dfd = dev_open(devname, O_RDONLY);
@@ -365,6 +358,14 @@ static int select_devices(struct mddev_dev *devlist,
 			int rv = 0;
 			struct mddev_ident *match;
 
+			if (ident->devices &&
+			    !match_oneof(ident->devices, devname)) {
+				if (report_mismatch)
+					pr_err("%s is not one of %s\n", devname,
+					       ident->devices);
+				goto loop;
+			}
+
 			content = *contentp;
 			tst->ss->getinfo_super(tst, content, NULL);
 
-- 
1.7.1

[PATCH 2/2] Detail: deterministic ordering in --brief --verbose

[mwilck]

Have mdadm --Detail --brief --verbose print the list of devices in
alphabetical order.

This is useful for debugging purposes. E.g. the test script
10ddf-create compares the output of two mdadm -Dbv calls which
may be different if the order is not deterministic.

(I confess: I use a modified "test" script that always runs
"mdadm --verbose" rather than "mdadm --quiet", otherwise this
wouldn't happen in 10ddf-create).

Signed-off-by: Martin Wilck <mwilck@arcor.de>
---
 Detail.c |   33 +++++++++++++++++++++++++--------
 1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/Detail.c b/Detail.c
index 33b3a18..031219a 100644
--- a/Detail.c
+++ b/Detail.c
@@ -27,6 +27,11 @@
 #include	"md_u.h"
 #include	<dirent.h>
 
+static int cmpstringp(const void *p1, const void *p2)
+{
+	return strcmp(* (char * const *) p1, * (char * const *) p2);
+}
+
 int Detail(char *dev, struct context *c)
 {
 	/*
@@ -42,7 +47,8 @@ int Detail(char *dev, struct context *c)
 	int d;
 	time_t atime;
 	char *str;
-	char *devices = NULL;
+	char **devices = NULL;
+	int max_devices = 0, n_devices = 0;
 	int spares = 0;
 	struct stat stb;
 	int is_26 = get_linux_version() >= 2006000;
@@ -636,12 +642,15 @@ This is pretty boring
 		dv=map_dev_preferred(disk.major, disk.minor, 0, c->prefer);
 		if (dv != NULL) {
 			if (c->brief) {
-				if (devices) {
-					devices = xrealloc(devices,
-							  strlen(devices)+1+strlen(dv)+1);
-					strcat(strcat(devices,","),dv);
-				} else
-					devices = xstrdup(dv);
+				if (n_devices + 1 >= max_devices) {
+					max_devices += 16;
+					devices = xrealloc(devices, max_devices
+							   *sizeof(*devices));
+					if (!devices)
+						goto out;
+				};
+				devices[n_devices] = xstrdup(dv);
+				n_devices++;
 			} else
 				printf("   %s", dv);
 		}
@@ -653,7 +662,12 @@ This is pretty boring
 	if (st)
 		st->ss->free_super(st);
 
-	if (c->brief && c->verbose > 0 && devices) printf("\n   devices=%s", devices);
+	if (c->brief && c->verbose > 0 && devices) {
+		qsort(devices, n_devices, sizeof(*devices), cmpstringp);
+		printf("\n   devices=%s", devices[0]);
+		for (d = 1; d < n_devices; d++)
+			printf(",%s", devices[d]);
+	}
 	if (c->brief)
 		printf("\n");
 	if (c->test &&
@@ -666,6 +680,9 @@ out:
 	close(fd);
 	free(subarray);
 	free(avail);
+	for (d = 0; d < n_devices; d++)
+		free(devices[d]);
+	free(devices);
 	sysfs_free(sra);
 	return rv;
 }
-- 
1.7.1

Re: [PATCH 2/2] Detail: deterministic ordering in --brief --verbose

[Martin Wilck]

On 06/20/2013 10:21 PM, mwilck@arcor.de wrote:

> (I confess: I use a modified "test" script that always runs
> "mdadm --verbose" rather than "mdadm --quiet", otherwise this
> wouldn't happen in 10ddf-create).

... and this test failed after applying commit b3908491,
that's the background for these two patches.

The standard test case that runs mdadm --quiet would not fail. But I
still think these patches are useful. They simplify at least my testing
& debugging.

Regards
Martin

Re: [PATCH 1/2] select_devices: fix scanning of container members with dev list

[NeilBrown]

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

On Thu, 20 Jun 2013 22:21:04 +0200 mwilck@arcor.de wrote:

> commit b3908491 "Detail: fix --brief --verbose" introduced a
> problem when a mdadm.conf file generated with
> "mdadm --Detail --brief --verbose" is later scanned with
> "mdadm --Assemble --scan --config=mdadm.conf"
> 
> mdadm -Dbv will print a "devices" list now, but because the
> container device is not in that list, it won't be considered
> for assembly.
> 
> This patch fixes that by moving the test for member devices
> further down, after the check for a container.
> 
> Signed-off-by: Martin Wilck <mwilck@arcor.de>

Hi Martin,
 I really don't like this.  If there is a "device=" entry then it should not
 even open anything not listed.

 Can you give me more details about the problem you are experiencing?  Maybe
 the problem is in "mdadm -Dbv".

Thanks,
NeilBrown

> ---
>  Assemble.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/Assemble.c b/Assemble.c
> index c927c20..1f023e8 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -170,13 +170,6 @@ static int select_devices(struct mddev_dev *devlist,
>  		if (tmpdev->used > 1)
>  			continue;
>  
> -		if (ident->devices &&
> -		    !match_oneof(ident->devices, devname)) {
> -			if (report_mismatch)
> -				pr_err("%s is not one of %s\n", devname, ident->devices);
> -			continue;
> -		}
> -
>  		tst = dup_super(st);
>  
>  		dfd = dev_open(devname, O_RDONLY);
> @@ -365,6 +358,14 @@ static int select_devices(struct mddev_dev *devlist,
>  			int rv = 0;
>  			struct mddev_ident *match;
>  
> +			if (ident->devices &&
> +			    !match_oneof(ident->devices, devname)) {
> +				if (report_mismatch)
> +					pr_err("%s is not one of %s\n", devname,
> +					       ident->devices);
> +				goto loop;
> +			}
> +
>  			content = *contentp;
>  			tst->ss->getinfo_super(tst, content, NULL);
>  


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

Re: [PATCH 2/2] Detail: deterministic ordering in --brief --verbose

[NeilBrown]

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

On Thu, 20 Jun 2013 22:21:05 +0200 mwilck@arcor.de wrote:

> Have mdadm --Detail --brief --verbose print the list of devices in
> alphabetical order.
> 
> This is useful for debugging purposes. E.g. the test script
> 10ddf-create compares the output of two mdadm -Dbv calls which
> may be different if the order is not deterministic.
> 
> (I confess: I use a modified "test" script that always runs
> "mdadm --verbose" rather than "mdadm --quiet", otherwise this
> wouldn't happen in 10ddf-create).
> 
> Signed-off-by: Martin Wilck <mwilck@arcor.de>
> ---
>  Detail.c |   33 +++++++++++++++++++++++++--------
>  1 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/Detail.c b/Detail.c
> index 33b3a18..031219a 100644
> --- a/Detail.c
> +++ b/Detail.c
> @@ -27,6 +27,11 @@
>  #include	"md_u.h"
>  #include	<dirent.h>
>  
> +static int cmpstringp(const void *p1, const void *p2)
> +{
> +	return strcmp(* (char * const *) p1, * (char * const *) p2);
> +}
> +
>  int Detail(char *dev, struct context *c)
>  {
>  	/*
> @@ -42,7 +47,8 @@ int Detail(char *dev, struct context *c)
>  	int d;
>  	time_t atime;
>  	char *str;
> -	char *devices = NULL;
> +	char **devices = NULL;
> +	int max_devices = 0, n_devices = 0;
>  	int spares = 0;
>  	struct stat stb;
>  	int is_26 = get_linux_version() >= 2006000;
> @@ -636,12 +642,15 @@ This is pretty boring
>  		dv=map_dev_preferred(disk.major, disk.minor, 0, c->prefer);
>  		if (dv != NULL) {
>  			if (c->brief) {
> -				if (devices) {
> -					devices = xrealloc(devices,
> -							  strlen(devices)+1+strlen(dv)+1);
> -					strcat(strcat(devices,","),dv);
> -				} else
> -					devices = xstrdup(dv);
> +				if (n_devices + 1 >= max_devices) {
> +					max_devices += 16;
> +					devices = xrealloc(devices, max_devices
> +							   *sizeof(*devices));
> +					if (!devices)
> +						goto out;
> +				};
> +				devices[n_devices] = xstrdup(dv);
> +				n_devices++;
>  			} else
>  				printf("   %s", dv);
>  		}
> @@ -653,7 +662,12 @@ This is pretty boring
>  	if (st)
>  		st->ss->free_super(st);
>  
> -	if (c->brief && c->verbose > 0 && devices) printf("\n   devices=%s", devices);
> +	if (c->brief && c->verbose > 0 && devices) {
> +		qsort(devices, n_devices, sizeof(*devices), cmpstringp);
> +		printf("\n   devices=%s", devices[0]);
> +		for (d = 1; d < n_devices; d++)
> +			printf(",%s", devices[d]);
> +	}
>  	if (c->brief)
>  		printf("\n");
>  	if (c->test &&
> @@ -666,6 +680,9 @@ out:
>  	close(fd);
>  	free(subarray);
>  	free(avail);
> +	for (d = 0; d < n_devices; d++)
> +		free(devices[d]);
> +	free(devices);
>  	sysfs_free(sra);
>  	return rv;
>  }


Applied, thanks.

NeilBrown

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

Re: [PATCH 1/2] select_devices: fix scanning of container members with dev list

[Martin Wilck]

Hi Neil,

>> commit b3908491 "Detail: fix --brief --verbose" introduced a
>> problem when a mdadm.conf file generated with
>> "mdadm --Detail --brief --verbose" is later scanned with
>> "mdadm --Assemble --scan --config=mdadm.conf"
>>
>> mdadm -Dbv will print a "devices" list now, but because the
>> container device is not in that list, it won't be considered
>> for assembly.
>>
>> This patch fixes that by moving the test for member devices
>> further down, after the check for a container.
>>
>> Signed-off-by: Martin Wilck <mwilck@arcor.de>
> 
> Hi Martin,
>  I really don't like this.  If there is a "device=" entry then it should not
>  even open anything not listed.

That doesn't work for containers. The "device" in the container case is
the container itself. Only by parsing the container and its subarrays
does mdadm -As succeed.

>  Can you give me more details about the problem you are experiencing?  Maybe
>  the problem is in "mdadm -Dbv".

Well, the problem might be fixed differently by having "mdadm -Dbv" in
the container case print the container device. I am not sure if that's
what you meant.

Below is the output of mdadm -Dbv from the DDF test case. Without my
patch, it can't be assembled with -As because /dev/md/ddf0 is not in the
devices list. If the "devices=" lines are deleted from the file, it
works. It would also work if the container device was a member of the
devices list.

Note that even if the "devices=" line is missing, select_devices() will
discard all the devices listed for subarrays ("/dev/loop10 has wrong
UUID"). This is because getinfo_super() will return the UUID of the
container, not of any subarray, in line 370 of Assemble.c. But the
scanning of the container device works.

ARRAY /dev/md/ddf0 level=container num-devices=5 metadata=ddf
UUID=0cadff6e:56f14bc0:5ac6bed5:b602c0a9
   devices=/dev/loop10,/dev/loop11,/dev/loop12,/dev/loop8,/dev/loop9
ARRAY /dev/md/r0 level=raid0 num-devices=5 container=/dev/md/ddf0
member=0 UUID=6c900c64:1873e2d0:ee78718b:7f156c44
   devices=/dev/loop10,/dev/loop11,/dev/loop12,/dev/loop8,/dev/loop9
ARRAY /dev/md/r1 level=raid1 num-devices=2 container=/dev/md/ddf0
member=1 UUID=caae05ca:e07a497a:bd066562:b5164eea
   devices=/dev/loop8,/dev/loop9
ARRAY /dev/md/r5 level=raid5 num-devices=3 container=/dev/md/ddf0
member=2 UUID=29f6e6ea:16d4a6f7:1808b76e:33c50480
   devices=/dev/loop10,/dev/loop11,/dev/loop12

Regards
Martin

Re: [PATCH 1/2] select_devices: fix scanning of container members with dev list

[Martin Wilck]

Hi Neil,

I wrote:
> Well, the problem might be fixed differently by having "mdadm -Dbv" in
> the container case print the container device.

I have come up with another patch, based on that idea, that I'll submit
in a minute. Do you like it better?

Regards,
Martin

Re: [PATCH 1/2] select_devices: fix scanning of container members with dev list

[NeilBrown]

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

On Thu, 27 Jun 2013 21:38:15 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> Hi Neil,
> 
> I wrote:
> > Well, the problem might be fixed differently by having "mdadm -Dbv" in
> > the container case print the container device.
> 
> I have come up with another patch, based on that idea, that I'll submit
> in a minute. Do you like it better?
> 

No I don't really.  The container name is already there is "container=" so
shouldn't be in "device=" as wel.  I did like the factored out "add_device",
so I kept that :-)

However you have helped me understand what the issue is and I know how I want
to fix it.

What I have ended up with is similar to your original patch, except that it
only ignores "device=" early if "container=" is set, as that is the only case
that is really a problem.

I've included my patch below and pushed it out into my git tree.

Thanks for your persistence.

NeilBrown


From c39b2e633fd6eb82a8a8e822ef01339806b05bfa Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Tue, 2 Jul 2013 11:07:38 +1000
Subject: [PATCH] Assemble: ignore devices= if container= is present.

If "container=" is present, then we are going to assemble from the
given container where that container is made of those devices or not.
So in this case the "devices=" is purely documentation and is best
ignored.

As part of this, move the test on the "container=" value when that
start with "/" up before the device is opened.  There sooner we test
things, the better.

Reported-by: Martin Wilck <mwilck@arcor.de>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Assemble.c b/Assemble.c
index a0041c6..afe5b05 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -171,8 +171,20 @@ static int select_devices(struct mddev_dev *devlist,
 		if (tmpdev->used > 1)
 			continue;
 
-		if (ident->devices &&
-		    !match_oneof(ident->devices, devname)) {
+		if (ident->container) {
+			if (ident->container[0] == '/' &&
+			    !same_dev(ident->container, devname)) {
+				if (report_mismatch)
+					pr_err("%s is not the container required (%s)\n",
+					       devname, ident->container);
+				continue;
+			}
+		} else if (ident->devices &&
+			   !match_oneof(ident->devices, devname)) {
+			/* Note that we ignore the "device=" identifier if a
+			 * "container=" is given.  Checking both is unnecessarily
+			 * complicated.
+			 */
 			if (report_mismatch)
 				pr_err("%s is not one of %s\n", devname, ident->devices);
 			continue;
@@ -289,29 +301,20 @@ static int select_devices(struct mddev_dev *devlist,
 			}
 			close(dfd);
 
-			if (ident->container) {
-				if (ident->container[0] == '/' &&
-				    !same_dev(ident->container, devname)) {
+			if (ident->container && ident->container[0] != '/') {
+				/* we have a uuid */
+				int uuid[4];
+
+				content = *contentp;
+				tst->ss->getinfo_super(tst, content, NULL);
+
+				if (!parse_uuid(ident->container, uuid) ||
+				    !same_uuid(content->uuid, uuid, tst->ss->swapuuid)) {
 					if (report_mismatch)
-						pr_err("%s is not the container required (%s)\n",
-						       devname, ident->container);
+						pr_err("%s has wrong UUID to be required container\n",
+						       devname);
 					goto loop;
 				}
-				if (ident->container[0] != '/') {
-					/* we have a uuid */
-					int uuid[4];
-
-					content = *contentp;
-					tst->ss->getinfo_super(tst, content, NULL);
-
-					if (!parse_uuid(ident->container, uuid) ||
-					    !same_uuid(content->uuid, uuid, tst->ss->swapuuid)) {
-						if (report_mismatch)
-							pr_err("%s has wrong UUID to be required container\n",
-							       devname);
-						goto loop;
-					}
-				}
 			}
 			/* It is worth looking inside this container.
 			 */

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