[PATCH] Do not assemble a duplicate of a running array

[NeilBrown]

On Sun, 10 Jul 2011 21:08:53 -0400 Yury Polyanskiy <yp at mit.edu> wrote:

> Dear mdadm developers,
> 
> Consider the following scenario (based on a true story):

Hi,
 thanks for the bug report and the patch.

I think it is probably much simpler to fix than your attempt.
Something like this:

diff --git a/Assemble.c b/Assemble.c
index 25cfec1..86b6e95 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -616,6 +616,17 @@ int Assemble(struct supertype *st, char *mddev,
 	if (!st || !st->sb || !content)
 		return 2;
 
+	{
+		int uuid[4];
+		struct map_ent *mp = NULL;
+		st->ss->uuid_from_super(st, uuid);
+		if (map_by_uuid(&mp, uuid)) {
+			map_free(mp);
+			fprintf(stderr, Name ": Not assembling array as uuid already in use\n");
+			return 1;
+		}
+		map_free(mp);
+	}
 	/* Now need to open the array device.  Use create_mddev */
 	if (content == &info)
 		st->ss->getinfo_super(st, content, NULL);


is what I'm thinking of - it seems to work for me, I'd appreciate it if you
can confirm that it works for you.

My concern though is that someone might genuinely want to assemble two arrays
with the same uuid.
The thing that really caused a problem in your case was that the two arrays
had the same name so they confused udev.
And having two array with the same name is definitely the wrong thing to do.

So something like:

diff --git a/Assemble.c b/Assemble.c
index 25cfec1..72f2618 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -651,6 +651,15 @@ int Assemble(struct supertype *st, char *mddev,
 	    conf_name_is_free(name))
 		trustworthy = LOCAL;
 
+	{
+		int uuid[4];
+		struct map_ent *mp = NULL;
+		st->ss->uuid_from_super(st, uuid);
+		if (map_by_uuid(&mp, uuid))
+			trustworthy = FOREIGN;
+		map_free(mp);
+	}
+
 	if (trustworthy == LOCAL &&
 	    strchr(name, ':'))
 		/* Ignore 'host:' prefix of name */

might do the trick ... except that it doesn't seem to cause udev to do what I
expect.  /var/run/mdadm/map has properly unique names but they don't appear
properly in /dev/md/

And that doesn't really test for uniqueness of names.

So I'm going to have to think about this some more..

Thanks again,

NeilBrown


> 
> SETUP:
> * a raid1 MD array is configured with the name /dev/md/data (metadata=1.0).
> * /dev/md/data is identified in the /etc/mdadm/mdadm.conf by its UUID
> * /dev/md/data is assembled from the initramfs with minor /dev/md127
> 
> CRASH:
> * [GLITCH] at some point one of the disks (/dev/sda) comprising /dev/md/data
> experiences a bus-failure and is correctly removed from the array.
> * array runs for some time in degraded state, data is being written and read
> from the remaining disks (/dev/sdb, sdc etc)
> * [REBOOT] after some time, the server is rebooted
> * at boot time initramfs script starts /dev/md/data, but kernel correctly
> kicks out /dev/sda from the array because the Event Counter is too old
> ('kicking non-fresh sda from array').
> * when udev starts, "mdadm --detail --export /dev/md127" correctly
> identifies /dev/md127 and UDEV creates a correct symbolic link:
>    /dev/md/data -> /dev/md127
> 
> * startup proceeds to execute /etc/init.d/mdadm-raid which runs "mdadm
> --assemble --scan".
> * mdadm --assemble --scan discovers the description of /dev/md/data in
> mdadm.conf and finds a lonely /dev/sda with a matching UUID; /dev/md126 is
> created out of a single /dev/sda
> * now udev receives an ADD event for /sys/block/md126; again "mdadm --detail
> --export /dev/md126" says that this device must be named /dev/md/data. UDEV
> then re-creates the symbolic link, which now points to /dev/md126:
>    /dev/md/data -> /dev/md126
> 
> * startup proceeds and /dev/md/data is mounted as a /home partition.
> 
> CONCLUSION:
> * data written to /dev/md/data between GLITCH and REBOOT appears as lost now
> (not mounted to /home).
> * data written to /dev/md/data after REBOOT is stored on a buggy /dev/sda
> instead of a sane array (/dev/sdb,/dev/sdc)
> 
> 
> PATCH:
> * attached is a patch against the master of http://neil.brown.name/git/mdadm
> 
> 
> Best wishes,
> Yury Polyanskiy