Minor mdadm fixes

I have several minor fixes against the 3.1.2 mdadm.

First attachment: mdadm-3.1.1-endian.patch
Problem: on ppc arches mdadm would fail to assemble arrays if you used mdadm -Db /dev/<array> to populate the ARRAY line in mdadm.conf. This is because the code in Detail.c calls fname_from_uuid() which honors the swapuuid setting in the superblock switch struct. This is great for ddf and imsm superblocks which call fname_from_uuid() all over the place and are therefore consistent. However, super1.c does *not* call fname_from_uuid()...ever. And super1.c does *not* perform the byte swapping as per swapuuid in the switch struct. At least not on *display*, but it *does* honor swapuuid on actual analysis of the uuid in terms of figuring out matches. So, if you attempt to change the value of swapuuid in the super1 switch struct, then uuid's never match what's printed out (aka, the output of -D and -E both fail to match against the detected uuid when assembling, although at least -D and -E are now consistent). Instead, I did a simple (and gross) hack to ignore swapuuid in fname_from_uuid() only for version 1 superblocks. The other alternative might be to make super1.c call fname_from_uuid() all over the place instead of printing things out itself, but that would involve struct changes as the way it stores/accesses uuids in super1 is not per char like fname_from_uuid and that's precisely part of the problem.

Second: mdadm-3.1.2-mapfile.patch
Problem: Neil's support for putting the mdadm map file wherever you need it is nice, but one place in particular needs a special case. Specifically, mdadm already creates /dev/md if needed to store symlinks, or in the case of mdmon needing to create pid/sock files (if ALT_RUN is set to /dev/md). This creates an expectation in udev and friends that mdadm will create /dev/md when needed, period. We need it if we place the mapfile in /dev/md prior to any symlinks or mdmon files being created, so special case that one location in order to make us consistent with both mdmon and mdopen. If we don't, we will fail to create our mapfile.

Third: mdadm-3.1.2-rebuild.patch
Problem: The above issue pointed out a different issue. Specifically, if we can't open our mapfile, then we call RebuildMap, and at the end of the rebuild, we trigger a new change event on the raid device. Well, if the write of the new map file failed because the directory we wanted to write into didn't exist, this creates an infinite loop where udev calls mdadm, mdadm fails to open file, mdadm rebuils map, mdadm fails to write new map, but mdadm triggers a change event to cause udev to rerun mdadm on the belief that the map file *was* updated. So, if we fail to write the new mapfile, don't signal a change event, let the situation drop. This avoids the infinite loop.

Fourth: mdadm-3.1.2-mapname.patch
Feature: If we are able to easily select the location of the mapfile via the use of ALT_RUN at compile time, it makes sense to also be able to set the filename. This way you can make it something obvious such as /dev/md + md-device-map which makes the file name both unlikely to conflict with a device name (where as I could see map conflicting in certain specialized scenarios, like the array holding map data at the local governmental office) and obvious in purpose.

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

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

@@ -395,7 +395,12 @@ char *__fname_from_uuid(int id[4], int s
 
 char *fname_from_uuid(struct supertype *st, struct mdinfo *info, char *buf, char sep)
 {
-	return __fname_from_uuid(info->uuid, st->ss->swapuuid, buf, sep);
+	// dirty hack to work around an issue with super1 superblocks...
+	// super1 superblocks need swapuuid set in order for assembly to
+	// work, but can't have it set if we want this printout to match
+	// all the other uuid printouts in super1.c, so we force swapuuid
+	// to 1 to make our printout match the rest of super1
+	return __fname_from_uuid(info->uuid, (st->ss == &super1) ? 1 : st->ss->swapuuid, buf, sep);
 }
 
 #ifndef MDASSEMBLE

commit b25462c1bee1a01c6aae3c215e040bfb2f1c2fb7

Author: Doug Ledford <[email protected]>
Date:   Tue Mar 16 23:00:11 2010 -0400

Create /dev/md in mapfile open like we do in mdopen if our ALT_RUN is set to be /dev/md. This keeps udev happy as it won't have to special case our /dev/md directory needs.

index 366ebe3..eed17c8 100644
@@ -29,7 +29,7 @@
  */
 
 /* /var/run/mdadm.map is used to track arrays being created in --incremental
- * more.  It particularly allows lookup from UUID to array device, but
+ * mode.  It particularly allows lookup from UUID to array device, but
  * also allows the array device name to be easily found.
  *
  * The map file is line based with space separated fields.  The fields are:

@@ -64,6 +64,16 @@ char *mapsmode[3] = { "r", "w", "w"};
 FILE *open_map(int modenum, int *choice)
 {
 	int i;
+	struct stat sbuf;
+
+	/* Special case...if ALT_RUN is selected to be /dev/md, then
+	 * because we would normally create /dev/md ourselves in order to
+	 * stuff array symlinks in there as needed, udev and friends
+	 * expect us to create our own tree.  So, do so.
+	 */
+	if (strcmp(ALT_RUN, "/dev/md") == 0 && stat(ALT_RUN, &sbuf) != 0)
+		mkdir(ALT_RUN, 0755);
+
 	for (i = 0 ; i < 3 ; i++) {
 		int fd = open(mapname[i][modenum], mapmode[modenum], 0600);
 		if (fd >= 0) {

commit 0d196a9dcab2e4eb0bce49b63c971d1e06cd9300

Author: Doug Ledford <[email protected]>
Date:   Wed Mar 17 09:28:07 2010 -0400

Only signal a udev change event if we actually write a mapfile in RebuildMap

index eed17c8..950cf62 100644
@@ -472,12 +472,14 @@ void RebuildMap(void)
 		}
 		sysfs_free(sra);
 	}
-	map_write(map);
+	/* Only trigger a change if we wrote a new map file */
+	if (map_write(map))
+		for (md = mdstat ; md ; md = md->next) {
+			struct mdinfo *sra = sysfs_read(-1, md->devnum,
+							GET_VERSION);
+			sysfs_uevent(sra, "change");
+			sysfs_free(sra);
+		}
 	map_free(map);
-	for (md = mdstat ; md ; md = md->next) {
-		struct mdinfo *sra = sysfs_read(-1, md->devnum, GET_VERSION);
-		sysfs_uevent(sra, "change");
-		sysfs_free(sra);
-	}
 	free_mdstat(mdstat);
 }

commit 9649a16e4c0e679c0e5bc3e33d816943b85dd910

Author: Doug Ledford <[email protected]>
Date:   Wed Mar 17 10:52:22 2010 -0400

mapfile: if we putting the mapfile in a custom location via ALT_RUN, allow a custom filename too.

index 1035ea8..2aafad0 100644
@@ -63,8 +63,9 @@ CONFFILEFLAGS = -DCONFFILE=\"$(CONFFILE)\" -DCONFFILE2=\"$(CONFFILE2)\"
 # If you don't have /lib/init/rw you might want to use /dev/.something
 #  e.g. make ALT_RUN=/dev/.mdadm
 ALT_RUN = /lib/init/rw
+ALT_MAPFILE = map
 VAR_RUN = /var/run
-ALTFLAGS = -DALT_RUN=\"$(ALT_RUN)\"
+ALTFLAGS = -DALT_RUN=\"$(ALT_RUN)\" -DALT_MAPFILE=\"$(ALT_MAPFILE)\"
 VARFLAGS = -DVAR_RUN=\"$(VAR_RUN)\"
 CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(ALTFLAGS) $(VARFLAGS)
 

index 950cf62..51d2b14 100644
@@ -55,7 +55,7 @@
 char *mapname[3][3] = {
 	mapnames(VAR_RUN "/map"),
 	mapnames("/var/run/mdadm.map"),
-	mapnames(ALT_RUN "/map")
+	mapnames(ALT_RUN "/" ALT_MAPFILE)
 };
 
 int mapmode[3] = { O_RDONLY, O_RDWR|O_CREAT, O_RDWR|O_CREAT | O_TRUNC };

why on earth do you want to set ALT_RUN=/dev/md ?
it is messy enough as it is, let's keep assuming /dev/md contains only array device files.

besides that you will discover that the only way to make mdmon work is setting VAR_RUN and ALT_RUN to the same value, something we can guarantee to be writable even in the direst situation, and i'd keep
/dev/.mdadm

this "hunt the mapfile" thing is already ugly as it is, why do we need an ALT_MAPNAME? if there was any value (except for allowing to set ALT_RUN to /dev/md) to having the mapfile named differently, just force it to be called mdadm.map everywere (or incremental.map so the name reflects the need for the file existance)

L.

Luca Berra -- [email protected]
        Communication Media & Services S.r.l.
 /"\
 \ /     ASCII RIBBON CAMPAIGN
  X        AGAINST HTML MAIL
 / \

Because on Fedora /lib/init/rw doesn't exist, and neither does any other early rw area other than /dev, and it's too late to create a new one, so I need to use someplace in /dev. However, I agree with Neil that if you are hiding things under . names then you are admitting something isn't right. So, I'm perfectly happy putting the map file in /dev/md as I think it has a perfect right to exist there.

I don't know how many arrays you have on your system, but I would hardly call /dev/md messy.

[ citation needed ]

There is no guarantee /dev/.mdadm exists on a clean install, where as I have made sure that all possible places that might need /dev/md will create it if it doesn't exist. Hence, the direst of situations is handled better with /dev/md than with /dev/.mdadm

Because /dev/md/map is too easy a name to conflict with a legitimate md array device name, and that's what it defaults to if you set ALT_RUN to /dev/md.

Except I set it to md-device-map. Aside from not conflicting with md device names, the map file name is mostly irrelevant except to indicate to people perusing the directory what the file is for as nothing uses it besides mdadm and mdadm will gladly use it with whatever name you call it as it both creates and uses the file from the same compile time configuration setting.

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

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

Neither in Mandriva for that matter.

besides the fact that it is perfectly clear that something is wrong, i am not vehemently opposing that.

I did not mean /dev/md is messy, I meant the code for locating the map file is messy, and the code for handling the mdmon pid and socket is messy....

for that matter on my test rig i have quite a few file in there /dev/md/imsm0
/dev/md/Volume0
/dev/md/Volume0p1
....
/dev/md/Volume1
/dev/md/Volume1p1
...

two issues:
1) see the thread on this ml starting with:
[mdadm PATCH 5/9] mdmon: mdmon_pid should return pid from either dir in practice mdadm looks for the mdmon pid and socket in VAR_RUN, but until takeover the pid and socket are in ALT_RUN, this means you cannot do array maintainance until /var is mounted rw.
(been there, done that, had to mount a tmpfs on /var and mount --bind /dev/.mdadm /var/run/mdadm to fix the array)
Neil refused that patch, but allowed redefinition of VAR_RUN, and i did not insist.

2) having to keep /var/run mounted during shutdown is a PITA, and iirc you (redhat) were opposed to having mdmon files in /var/run/mdadm, last time the issue was discussed here.

yes, in mdmon.c ALT_RUN is created before usage, in mapfile.c it is not, adding an mkdir there would be useful, independent of what we set ALT_RUN to.

I know all this, what i meant is:
why do you feel there is a need for ALT_MAPNAME, would not it be easier to just define a MAPFILE_NAME with a fixed path and avoid the open_map juggling in mapfile.c, there is no need for that code, since, as you
state: "mdadm both creates and uses the file from the same compile time configuration setting."

L.

Luca Berra -- [email protected]
        Communication Media & Services S.r.l.
 /"\
 \ /     ASCII RIBBON CAMPAIGN
  X        AGAINST HTML MAIL
 / \

Thanks!

Ohh, that is horrible isn't it.
I have applied your patch but I very much hope to "fix" the problem in a more comprehensive way so that your fix will disappear.
I suspect that I will change the common code to use an array of bytes and do any conversion from u32[4] to u8[1] in the per-metadata code.

I thought udev always created the directory if it was needed, and always removed it if it became empty after deleting an entry. But I have no strong basis for believing that.

However I do think that mdadm should create the directory in this case - not any parents, but if they exist and are writeable, then create the directory.

So I modified you patch to do that when creating the mapfile (not when reading it).

Very appropriate - thanks.

Seems reasonable. I've included that patch too.

Thanks a lot,
NeilBrown

I don't know about the removing of a directory, but it will only create a directory if we indicate it should place a symlink in it (speaking of /dev/md/). And we only do that if mdadm -D --export prints out MD_DEVNAME, and we only print that out if the name field of the superblock includes /dev/md/. So, if the name is something shorthand, like host:0 or host:md0, we never print out MD_DEVNAME and so udev doesn't create the symlinks in /dev/md.

But, regardless of that, we need the mapfile prior to devices being brought up, so prior to any possible directory creation by udev. Hence we must take care of the issue ourselves.

Makes sense, thanks.

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

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