* mdmon fixes *

PATCH In review Luca Berra 3 participants 26 messages Feb 27, 2010 – Mar 3, 2010

Subsystems (2)

Kbuild MD RAID

lore

Share & tools

lore

Luca Berra

author

2010-02-27 15:05 UTC #0 lore raw

cover *** mdmon fixes ***

These close a couple of issues with mdmon.
I don't claim to understand all of the code yet, but a cursory look leaves me with the feeling that there are multiple ways it could still fail

Luca Berra (2):
fix mdmon option parsing
fix mdmon takeover

 mdmon.c |   23 ++++++++++++++++++-----
 msg.c   |    2 +-
 util.c  |    4 ++--
 3 files changed, 21 insertions(+), 8 deletions(-)

Signature · 6 lines

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

Luca Berra

author

2010-02-27 15:07 UTC #1 lore raw

PATCH 1/2 fix mdmon option parsing

running mdmon --takeover --all resulted in a segfault. also hide "--takeover" from ps and update usage message.

Signed-off-by Luca Berra <[email protected]>

---
 mdmon.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

mdmon.c +9 -4

index 70fbd0b..d593ebc 100644
@@ -244,7 +244,7 @@ static int do_fork(void)
 
 void usage(void)
 {
-	fprintf(stderr, "Usage: mdmon /device/name/for/container [target_dir]\n");
+	fprintf(stderr, "Usage: mdmon [--all] [--takeover] CONTAINER");
 	exit(2);
 }

@@ -262,15 +262,20 @@ int main(int argc, char *argv[])
 
 	for (arg = 1; arg < argc; arg++) {
 		if (strcmp(argv[arg], "--all") == 0 ||
-		    strcmp(argv[arg], "/proc/mdstat") == 0)
+		    strcmp(argv[arg], "/proc/mdstat") == 0) {
+			container_name = argv[arg];
 			all = 1;
-		else if (strcmp(argv[arg], "--takeover") == 0)
+		} else if (strcmp(argv[arg], "--takeover") == 0) {
+			/* hide the --takeover arg from commandline */
+			memset(argv[arg], 0, strlen(argv[arg]));
 			takeover = 1;
-		else if (container_name == NULL)
+		} else if (container_name == NULL)
 			container_name = argv[arg];
 		else
 			usage();
 	}
+	if (container_name == NULL)
+		usage();
 
 	if (all) {
 		struct mdstat_ent *mdstat, *e;

Signature · 9 lines

1.7.0

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

Luca Berra

author

2010-02-27 15:21 UTC #2 lore raw

Re: [mdadm PATCH 1/2] fix mdmon option parsing

mdmon.c static int do_fork(void) · line 246

Quoted 7 lines · Luca Berra

mdmon.c +1 -1

@@ -244,7 +244,7 @@ static int do_fork(void)
  void usage(void)
 {
-	fprintf(stderr, "Usage: mdmon /device/name/for/container [target_dir]\n");
+	fprintf(stderr, "Usage: mdmon [--all] [--takeover] CONTAINER");

oops, i missed a "\n" above

Quoted 3 lines

mdmon.c +0 -0

 	exit(2);
 }
 @@ -262,15 +262,20 @@ int main(int argc, char *argv[])

Signature · 6 lines

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

Neil Brown

2010-03-02 05:40 UTC #3 lore raw

Re: [mdadm PATCH 1/2] fix mdmon option parsing

In reply to Luca Berra

Quoted 2 lines · On Sat, 27 Feb 2010 16:07:04 +0100 Luca Berra <[email protected]> wrote:

running mdmon --takeover --all resulted in a segfault. also hide "--takeover" from ps and update usage message.

Why do you want to hide the "--takeover" ???

(the rest makes sense, thanks).

NeilBrown

Quoted 43 lines

Signed-off-by Luca Berra <[email protected]>

---
  mdmon.c |   13 +++++++++----
  1 files changed, 9 insertions(+), 4 deletions(-)

mdmon.c +9 -4

index 70fbd0b..d593ebc 100644
@@ -244,7 +244,7 @@ static int do_fork(void)
 
  void usage(void)
  {
-	fprintf(stderr, "Usage: mdmon /device/name/for/container [target_dir]\n");
+	fprintf(stderr, "Usage: mdmon [--all] [--takeover] CONTAINER");
  	exit(2);
  }

@@ -262,15 +262,20 @@ int main(int argc, char *argv[])
 
  	for (arg = 1; arg < argc; arg++) {
  		if (strcmp(argv[arg], "--all") == 0 ||
-		    strcmp(argv[arg], "/proc/mdstat") == 0)
+		    strcmp(argv[arg], "/proc/mdstat") == 0) {
+			container_name = argv[arg];
  			all = 1;
-		else if (strcmp(argv[arg], "--takeover") == 0)
+		} else if (strcmp(argv[arg], "--takeover") == 0) {
+			/* hide the --takeover arg from commandline */
+			memset(argv[arg], 0, strlen(argv[arg]));
  			takeover = 1;
-		else if (container_name == NULL)
+		} else if (container_name == NULL)
  			container_name = argv[arg];
  		else
  			usage();
  	}
+	if (container_name == NULL)
+		usage();
 
  	if (all) {
  		struct mdstat_ent *mdstat, *e;

Luca Berra

author

2010-03-02 07:42 UTC #4 lore raw

Re: [mdadm PATCH 1/2] fix mdmon option parsing

Quoted 7 lines · Neil Brown

Quoted 2 lines · On Sat, 27 Feb 2010 16:07:04 +0100 Luca Berra <[email protected]> wrote:
running mdmon --takeover --all resulted in a segfault. also hide "--takeover" from ps and update usage message.
Why do you want to hide the "--takeover" ???

After takeover there is no difference between a normal mdmon and one that took over, but thinking twice it does not hide very well if we run "mdmon --takeover --all" versus "mdmon --all --takeover", and maybe someone is interested in seeing if takeover did work, so you can ignore this.

Signature · 6 lines

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

Luca Berra

author

2010-02-27 15:07 UTC #5 lore raw

PATCH 2/2 fix mdmon takeover

In reply to Luca Berra

there are two cases that would result in two mdmon processes for the same array
takeover not detecting pid file in ALT_RUN
if running mdmon a second time without "--takeover"

Signed-off-by Luca Berra <[email protected]>

---
 mdmon.c |   10 +++++++++-
 msg.c   |    2 +-
 util.c  |    4 ++--
 3 files changed, 12 insertions(+), 4 deletions(-)

mdmon.c +9 -1

index d593ebc..a2b7946 100644
@@ -441,7 +441,6 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 	sigaction(SIGPIPE, &act, NULL);
 
 	if (takeover) {
-		pid_dir = VAR_RUN;
 		victim = mdmon_pid(container->devnum);
 		if (victim < 0) {
 			pid_dir = ALT_RUN;

@@ -453,6 +452,15 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 
 	ignore = chdir("/");
 	if (victim < 0) {
+		pid_dir = ALT_RUN;
+		if (ping_monitor(container->devname) == 0) {
+			fprintf(stderr, "mdmon: %s already managed\n",
+				container->devname);
+			if (!takeover)
+				fprintf(stderr, "\trun mdmon --takeover instead\n");
+			exit(3);
+		}
+		pid_dir = VAR_RUN;
 		if (ping_monitor(container->devname) == 0) {
 			fprintf(stderr, "mdmon: %s already managed\n",
 				container->devname);

msg.c +1 -1

index cc07b96..d2d8445 100644
@@ -147,7 +147,7 @@ int connect_monitor(char *devname)
 	int pos;
 	char *c;
 
-	pos = sprintf(path, "%s/", VAR_RUN);
+	pos = sprintf(path, "%s/", pid_dir);
 	if (is_subarray(devname)) {
 		devname++;
 		c = strchr(devname, '/');

util.c +2 -2

index 1def2a0..2fe566d 100644
@@ -1475,11 +1475,11 @@ int mdmon_pid(int devnum)
 	fd = open(path, O_RDONLY | O_NOATIME, 0);
 
 	if (fd < 0)
-		return 0;
+		return -1;
 	n = read(fd, pid, 9);
 	close(fd);
 	if (n <= 0)
-		return 0;
+		return -1;
 	return atoi(pid);
 }

Signature · 9 lines

1.7.0

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

Neil Brown

2010-03-03 00:52 UTC #6 lore raw

Re: [mdadm PATCH 2/2] fix mdmon takeover

Quoted 17 lines · On Sat, 27 Feb 2010 16:07:51 +0100 Luca Berra <[email protected]> wrote:

there are two cases that would result in two mdmon processes for the same array
takeover not detecting pid file in ALT_RUN
if running mdmon a second time without "--takeover"
Signed-off-by Luca Berra <[email protected]>
---
  mdmon.c |   10 +++++++++-
  msg.c   |    2 +-
  util.c  |    4 ++--
  3 files changed, 12 insertions(+), 4 deletions(-)
mdmon.c +0 -0
index d593ebc..a2b7946 100644

mdmon.c static int mdmon(char *devname, int devnum, int must_fork, int takeover) · line 446

Quoted 7 lines

mdmon.c +0 -1

@@ -441,7 +441,6 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  	sigaction(SIGPIPE, &act, NULL);
 
  	if (takeover) {
-		pid_dir = VAR_RUN;
  		victim = mdmon_pid(container->devnum);
  		if (victim < 0) {
  			pid_dir = ALT_RUN;

Is this just a micro-optimisation? or does it have some real functional significance?

Quoted 6 lines

mdmon.c +2 -0

@@ -453,6 +452,15 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 
  	ignore = chdir("/");
  	if (victim < 0) {
+		pid_dir = ALT_RUN;
+		if (ping_monitor(container->devname) == 0) {

mdmon.c static int mdmon(char *devname, int devnum, int must_fork, int takeover) · line 466

Quoted 10 lines

mdmon.c +7 -0

+			fprintf(stderr, "mdmon: %s already managed\n",
+				container->devname);
+			if (!takeover)
+				fprintf(stderr, "\trun mdmon --takeover instead\n");
+			exit(3);
+		}
+		pid_dir = VAR_RUN;
  		if (ping_monitor(container->devname) == 0) {
  			fprintf(stderr, "mdmon: %s already managed\n",
  				container->devname);

I don't understand this. We always know where the pid file is, why do we have to check again?

msg.c int connect_monitor(char *devname) · line 153

Quoted 13 lines

msg.c +1 -1

index cc07b96..d2d8445 100644
@@ -147,7 +147,7 @@ int connect_monitor(char *devname)
  	int pos;
  	char *c;
 
-	pos = sprintf(path, "%s/", VAR_RUN);
+	pos = sprintf(path, "%s/", pid_dir);
  	if (is_subarray(devname)) {
  		devname++;
  		c = strchr(devname, '/');

This is correct, thanks.

Quoted 8 lines

util.c +0 -0

index 1def2a0..2fe566d 100644
@@ -1475,11 +1475,11 @@ int mdmon_pid(int devnum)
  	fd = open(path, O_RDONLY | O_NOATIME, 0);
 
  	if (fd < 0)

util.c int mdmon_pid(int devnum) · line 1485

Quoted 10 lines

util.c +2 -2

-		return 0;
+		return -1;
  	n = read(fd, pid, 9);
  	close(fd);
  	if (n <= 0)
-		return 0;
+		return -1;
  	return atoi(pid);
  }

This is needed too.
For now I'll keep just the last two chunks.

Thanks.
NeilBrown

Luca Berra

author

2010-02-28 14:41 UTC #7 lore raw

In reply to Luca Berra

Quoted 3 lines · Luca Berra

These close a couple of issues with mdmon.
I don't claim to understand all of the code yet, but a cursory look leaves me with the feeling that there are multiple ways it could still fail

As foreseen it does actually fail...

someone might remember my flaky dual boot system with imsm array it crashed again, badly
result
mdmon is still running from ALT_RUN, since / is corrupted and cannot be mounted read-write
but mdadm command looks for mdmon only in VAR_RUN
so a new bunch of patches is due.
these apply over the two preceding patches

3/9 add missing newline to mdmon usage message
4/9 mdmon: check select a writable pid_dir
5/9 mdmon: mdmon_pid should return pid from either dir 6/9 mdmon: connect_monitor should use socket from either dir 7/9 mdmon: move pid_dir to mdmon.c
8/9 mdmon: rework startup and takeover logic

and after I spent a good deal of time fixing the above I was bitten by the mdmon socket in /var/run/mdadm at shutdown time again. So i added:

9/9 allow redefinition of VAR_RUN

But I respectfully ask to reconsider the placement of mdmon socket and keep it somewere under /, allowing people to unmount /var.
I don't get why putting sockets in /dev is considered wrong and we must run through multiple hoops to put those under /var when we already have sockets in /dev and no-one ever questioned syslogd for doing so.

Regards,
L.

Signature · 6 lines

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

Bill Davidsen

2010-02-28 15:41 UTC #8 lore raw

Quoted 6 lines · Luca Berra

Quoted 3 lines · Luca Berra
These close a couple of issues with mdmon.
I don't claim to understand all of the code yet, but a cursory look leaves me with the feeling that there are multiple ways it could still fail
As foreseen it does actually fail...

Quoted 8 lines

someone might remember my flaky dual boot system with imsm array it crashed again, badly
result
mdmon is still running from ALT_RUN, since / is corrupted and cannot be mounted read-write
but mdadm command looks for mdmon only in VAR_RUN
so a new bunch of patches is due.
these apply over the two preceding patches

Rather than adding complexity to the code, would it not be easier to just mount a tmpfs on VAR_RUN since it is ephemeral anyway and is not valid across boots? It seems more reasonable to put the exception handling in the system with the exception than in production code.

Signature · 3 lines

Bill Davidsen <[email protected]>
 "We can't solve today's problems by using the same thinking we
  used in creating them." - Einstein

Luca Berra

author

2010-02-28 17:37 UTC #9 lore raw

Quoted 16 lines · Bill Davidsen

Quoted 6 lines · Luca Berra
Quoted 3 lines · Luca Berra
These close a couple of issues with mdmon.
I don't claim to understand all of the code yet, but a cursory look leaves me with the feeling that there are multiple ways it could still fail
As foreseen it does actually fail...
Quoted 8 lines
someone might remember my flaky dual boot system with imsm array it crashed again, badly
result
mdmon is still running from ALT_RUN, since / is corrupted and cannot be mounted read-write
but mdadm command looks for mdmon only in VAR_RUN
so a new bunch of patches is due.
these apply over the two preceding patches

Quoted 1 line

Rather than adding complexity to the code, would it not be easier to just

I too feel this code is complex and difficult to get right and maintain. But my patch set just fixes the problems with the current implementation, it does not deal with Neil's design choiches. (well, except last one which opens a way out for people that don't agree with Neil on the ALT_RUN thing)

Quoted 1 line

mount a tmpfs on VAR_RUN since it is ephemeral anyway and is not valid across boots? It seems more reasonable to put the exception handling in the system with the exception than in production code.

I don't get your point, are we trying to address the same issue?

If it were me i'd put the socket in /dev and the pid file in /var/run. Leaving the job of moving state files from initramfs /var/run to real /var/run to initramfs and init scripts distro specific code.

Signature · 6 lines

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

Luca Berra

author

2010-02-28 14:42 UTC #10 lore raw

PATCH 3/9 add missing newline to mdmon usage message

In reply to Luca Berra

Signed-off-by Luca Berra <[email protected]>

---
 mdmon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

mdmon.c +1 -1

index a2b7946..11b4f32 100644
@@ -244,7 +244,7 @@ static int do_fork(void)
 
 void usage(void)
 {
-	fprintf(stderr, "Usage: mdmon [--all] [--takeover] CONTAINER");
+	fprintf(stderr, "Usage: mdmon [--all] [--takeover] CONTAINER\n");
 	exit(2);
 }

Signature · 10 lines

1.7.0


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

Luca Berra

author

2010-02-28 14:42 UTC #11 lore raw

PATCH 4/9 mdmon: check select a writable pid_dir

In reply to Luca Berra

Check that either VAR_DIR or ALT_DIR is actually writable before selecting it.

Signed-off-by Luca Berra <[email protected]>

---
 mdmon.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mdmon.c +2 -2

index 11b4f32..57fd492 100644
@@ -480,9 +480,9 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 	 */
 	if (victim > 0)
 		remove_pidfile(devname);
-	if (mkdir(VAR_RUN, 0600) >= 0 || errno == EEXIST)
+	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0))
 		pid_dir = VAR_RUN;
-	else if (mkdir(ALT_RUN, 0600) >= 0 || errno == EEXIST)
+	else if (mkdir(ALT_RUN, 0600) >= 0 || (errno == EEXIST && access(ALT_RUN, W_OK) >= 0))
 		pid_dir = ALT_RUN;
 	else {
 		fprintf(stderr, "mdmon: Neither %s nor %s are writable\n"

Signature · 10 lines

1.7.0


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

Neil Brown

2010-03-03 01:07 UTC #12 lore raw

Re: [mdadm PATCH 4/9] mdmon: check select a writable pid_dir

Quoted 2 lines · On Sun, 28 Feb 2010 15:42:57 +0100 Luca Berra <[email protected]> wrote:

Check that either VAR_DIR or ALT_DIR is actually writable before selecting it.

Why would e.g. /var/run/mdadm exist but not be writable? Sounds like a serious mis-configuration.

Nevertheless, it is safer this way.
I personally avoid using 'access' unless I am running setuid and want to check what the real-uid would be allowed to do, as that is what 'access' is for.
So I changed this code to test writability by calling make_pidfile - i.e. actually doing the write.

Thanks,
NeilBrown

Quoted 22 lines

Signed-off-by Luca Berra <[email protected]>

---
  mdmon.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

mdmon.c +2 -2

index 11b4f32..57fd492 100644
@@ -480,9 +480,9 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  	 */
  	if (victim > 0)
  		remove_pidfile(devname);
-	if (mkdir(VAR_RUN, 0600) >= 0 || errno == EEXIST)
+	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0))
  		pid_dir = VAR_RUN;
-	else if (mkdir(ALT_RUN, 0600) >= 0 || errno == EEXIST)
+	else if (mkdir(ALT_RUN, 0600) >= 0 || (errno == EEXIST && access(ALT_RUN, W_OK) >= 0))
  		pid_dir = ALT_RUN;
  	else {
  		fprintf(stderr, "mdmon: Neither %s nor %s are writable\n"

Luca Berra

author

2010-02-28 14:43 UTC #13 lore raw

PATCH 5/9 mdmon: mdmon_pid should return pid from either dir

In reply to Luca Berra

mdmon_pid is called by mdmon_running to check if mdmon is running, so the pid file should be checked in either VAR_RUN or ALT_RUN

Signed-off-by Luca Berra <[email protected]>

---
 mdadm.h |    2 +-
 mdmon.c |    6 +++---
 util.c  |   14 ++++++++++++--
 3 files changed, 16 insertions(+), 6 deletions(-)

mdadm.h +1 -1

index df3a056..7efa8bf 100644
@@ -897,7 +897,7 @@ extern int open_container(int fd);
 
 extern char *pid_dir;
 extern int mdmon_running(int devnum);
-extern int mdmon_pid(int devnum);
+extern int mdmon_pid(int devnum, const char *pid_dir);
 extern int check_env(char *name);
 extern __u32 random32(void);
 extern int start_mdmon(int devnum);

mdmon.c +3 -3

index 57fd492..3410e84 100644
@@ -441,10 +441,10 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 	sigaction(SIGPIPE, &act, NULL);
 
 	if (takeover) {
-		victim = mdmon_pid(container->devnum);
+		victim = mdmon_pid(container->devnum, pid_dir);
 		if (victim < 0) {
 			pid_dir = ALT_RUN;
-			victim = mdmon_pid(container->devnum);
+			victim = mdmon_pid(container->devnum, pid_dir);
 		}
 		if (victim >= 0)
 			victim_sock = connect_monitor(container->devname);

@@ -467,7 +467,7 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 			exit(3);
 		}
 		/* if there is a pid file, kill whoever is there just in case */
-		victim = mdmon_pid(container->devnum);
+		victim = mdmon_pid(container->devnum, NULL);
 	}
 	if (container->ss->load_super(container, mdfd, devname)) {
 		fprintf(stderr, "mdmon: Cannot load metadata for %s\n",

util.c +12 -2

index 2fe566d..50e04bf 100644
@@ -1465,12 +1465,22 @@ int fd2devnum(int fd)
 
 char *pid_dir = VAR_RUN;
 
-int mdmon_pid(int devnum)
+int mdmon_pid(int devnum, const char *pid_dir)
 {
 	char path[100];
 	char pid[10];
 	int fd;
 	int n;
+
+	/* if pid_dir is null try to detect it */
+	if (!pid_dir) {
+		n = mdmon_pid(devnum, VAR_RUN);
+		if (n >= 0)
+			return n;
+		else
+			return mdmon_pid(devnum, ALT_RUN);
+	}
+
 	sprintf(path, "%s/%s.pid", pid_dir, devnum2devname(devnum));
 	fd = open(path, O_RDONLY | O_NOATIME, 0);

@@ -1485,7 +1495,7 @@ int mdmon_pid(int devnum)
 
 int mdmon_running(int devnum)
 {
-	int pid = mdmon_pid(devnum);
+	int pid = mdmon_pid(devnum, NULL);
 	if (pid <= 0)
 		return 0;
 	if (kill(pid, 0) == 0)

Signature · 10 lines

1.7.0


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

Neil Brown

2010-03-03 01:50 UTC #14 lore raw

Re: [mdadm PATCH 5/9] mdmon: mdmon_pid should return pid from either dir

Quoted 2 lines · On Sun, 28 Feb 2010 15:43:40 +0100 Luca Berra <[email protected]> wrote:

mdmon_pid is called by mdmon_running to check if mdmon is running, so the pid file should be checked in either VAR_RUN or ALT_RUN

I don't agree with this.

The only time that any code should care about ALT_RUN is when mdmon is starting. It chooses VAR_RUN or ALT_RUN to use, and possibly looks for an old instance to kill.

When starting mdmon without "--takeover", if it decided to use VAR_RUN, it would not check ALT_RUN to see if an old mdmon was running. So I have replaced you patch with a patch to include that extra check when starting mdmon without --takeover.
Maybe that is the case you intended to address??

Thanks,
NeilBrown

Quoted 84 lines

Signed-off-by Luca Berra <[email protected]>

---
  mdadm.h |    2 +-
  mdmon.c |    6 +++---
  util.c  |   14 ++++++++++++--
  3 files changed, 16 insertions(+), 6 deletions(-)

mdadm.h +1 -1

index df3a056..7efa8bf 100644
@@ -897,7 +897,7 @@ extern int open_container(int fd);
 
  extern char *pid_dir;
  extern int mdmon_running(int devnum);
-extern int mdmon_pid(int devnum);
+extern int mdmon_pid(int devnum, const char *pid_dir);
  extern int check_env(char *name);
  extern __u32 random32(void);
  extern int start_mdmon(int devnum);

mdmon.c +3 -3

index 57fd492..3410e84 100644
@@ -441,10 +441,10 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  	sigaction(SIGPIPE, &act, NULL);
 
  	if (takeover) {
-		victim = mdmon_pid(container->devnum);
+		victim = mdmon_pid(container->devnum, pid_dir);
  		if (victim < 0) {
  			pid_dir = ALT_RUN;
-			victim = mdmon_pid(container->devnum);
+			victim = mdmon_pid(container->devnum, pid_dir);
  		}
  		if (victim >= 0)
  			victim_sock = connect_monitor(container->devname);

@@ -467,7 +467,7 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  			exit(3);
  		}
  		/* if there is a pid file, kill whoever is there just in case */
-		victim = mdmon_pid(container->devnum);
+		victim = mdmon_pid(container->devnum, NULL);
  	}
  	if (container->ss->load_super(container, mdfd, devname)) {
  		fprintf(stderr, "mdmon: Cannot load metadata for %s\n",

util.c +12 -2

index 2fe566d..50e04bf 100644
@@ -1465,12 +1465,22 @@ int fd2devnum(int fd)
 
  char *pid_dir = VAR_RUN;
 
-int mdmon_pid(int devnum)
+int mdmon_pid(int devnum, const char *pid_dir)
  {
  	char path[100];
  	char pid[10];
  	int fd;
  	int n;
+
+	/* if pid_dir is null try to detect it */
+	if (!pid_dir) {
+		n = mdmon_pid(devnum, VAR_RUN);
+		if (n >= 0)
+			return n;
+		else
+			return mdmon_pid(devnum, ALT_RUN);
+	}
+
  	sprintf(path, "%s/%s.pid", pid_dir, devnum2devname(devnum));
  	fd = open(path, O_RDONLY | O_NOATIME, 0);

@@ -1485,7 +1495,7 @@ int mdmon_pid(int devnum)
 
  int mdmon_running(int devnum)
  {
-	int pid = mdmon_pid(devnum);
+	int pid = mdmon_pid(devnum, NULL);
  	if (pid <= 0)
  		return 0;
  	if (kill(pid, 0) == 0)

Luca Berra

author

2010-03-03 06:44 UTC #15 lore raw

Re: [mdadm PATCH 5/9] mdmon: mdmon_pid should return pid from either dir

Quoted 12 lines · Neil Brown

Quoted 2 lines · On Sun, 28 Feb 2010 15:43:40 +0100 Luca Berra <[email protected]> wrote:
mdmon_pid is called by mdmon_running to check if mdmon is running, so the pid file should be checked in either VAR_RUN or ALT_RUN
I don't agree with this.
The only time that any code should care about ALT_RUN is when mdmon is starting. It chooses VAR_RUN or ALT_RUN to use, and possibly looks for an old instance to kill.

Quoted 5 lines

When starting mdmon without "--takeover", if it decided to use VAR_RUN, it would not check ALT_RUN to see if an old mdmon was running. So I have replaced you patch with a patch to include that extra check when starting mdmon without --takeover.
Maybe that is the case you intended to address??

No,
the problem i try to address is in mdadm, not mdmon
When mdadm needs to manage arrays with external metadata it will look for mdmon using mdmon_running(). If mdmon is running but did not yet take over mdadm will refuse to operate stating that mdmon is not running.

I had to mount --bind ALT_RUN VAR_RUN in order to repair my imsm on sunday morning.

Regards,
L.

Signature · 6 lines

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

Neil Brown

2010-03-03 06:55 UTC #16 lore raw

Re: [mdadm PATCH 5/9] mdmon: mdmon_pid should return pid from either dir

Quoted 26 lines · On Wed, 3 Mar 2010 07:44:03 +0100 Luca Berra <[email protected]> wrote:

Quoted 12 lines · Neil Brown
Quoted 2 lines · On Sun, 28 Feb 2010 15:43:40 +0100 Luca Berra <[email protected]> wrote:
mdmon_pid is called by mdmon_running to check if mdmon is running, so the pid file should be checked in either VAR_RUN or ALT_RUN
I don't agree with this.
The only time that any code should care about ALT_RUN is when mdmon is starting. It chooses VAR_RUN or ALT_RUN to use, and possibly looks for an old instance to kill.
Quoted 5 lines
When starting mdmon without "--takeover", if it decided to use VAR_RUN, it would not check ALT_RUN to see if an old mdmon was running. So I have replaced you patch with a patch to include that extra check when starting mdmon without --takeover.
Maybe that is the case you intended to address??
No,
the problem i try to address is in mdadm, not mdmon
When mdadm needs to manage arrays with external metadata it will look for mdmon using mdmon_running(). If mdmon is running but did not yet take over mdadm will refuse to operate stating that mdmon is not running.

Quoted 2 lines

I had to mount --bind ALT_RUN VAR_RUN in order to repair my imsm on sunday morning.

Ahhh..
I would rather you did
mdmon --takeover --all

which would effectively move the key fines from ALT_RUN to VAR_RUN.

Would that have worked in your case?

NeilBrown

Luca Berra

author

2010-03-03 07:09 UTC #17 lore raw

Re: [mdadm PATCH 5/9] mdmon: mdmon_pid should return pid from either dir

Quoted 32 lines · Neil Brown

Quoted 26 lines · On Wed, 3 Mar 2010 07:44:03 +0100 Luca Berra <[email protected]> wrote:
Quoted 12 lines · Neil Brown
Quoted 2 lines · On Sun, 28 Feb 2010 15:43:40 +0100 Luca Berra <[email protected]> wrote:
mdmon_pid is called by mdmon_running to check if mdmon is running, so the pid file should be checked in either VAR_RUN or ALT_RUN
I don't agree with this.
The only time that any code should care about ALT_RUN is when mdmon is starting. It chooses VAR_RUN or ALT_RUN to use, and possibly looks for an old instance to kill.
Quoted 5 lines
When starting mdmon without "--takeover", if it decided to use VAR_RUN, it would not check ALT_RUN to see if an old mdmon was running. So I have replaced you patch with a patch to include that extra check when starting mdmon without --takeover.
Maybe that is the case you intended to address??
No,
the problem i try to address is in mdadm, not mdmon
When mdadm needs to manage arrays with external metadata it will look for mdmon using mdmon_running(). If mdmon is running but did not yet take over mdadm will refuse to operate stating that mdmon is not running.
Quoted 2 lines
I had to mount --bind ALT_RUN VAR_RUN in order to repair my imsm on sunday morning.

Quoted 8 lines

Ahhh..
I would rather you did
mdmon --takeover --all
which would effectively move the key fines from ALT_RUN to VAR_RUN.
Would that have worked in your case?

Unfortunately I did not have the fortune of a writable /var/run at the moment.
(that's what prompted me to add the check in another patch)

Signature · 6 lines

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

Luca Berra

author

2010-02-28 14:44 UTC #18 lore raw

PATCH 6/9 mdmon: connect_monitor should use socket from either dir

In reply to Luca Berra

connect_monitor is called by ping_monitor in various places the socket should be checked in either VAR_RUN or ALT_RUN

Signed-off-by Luca Berra <[email protected]>

---
 mdmon.c |    2 +-
 msg.c   |   15 ++++++++++++---
 msg.h   |    2 +-
 util.c  |    2 +-
 4 files changed, 15 insertions(+), 6 deletions(-)

mdmon.c +1 -1

index 3410e84..3627a80 100644
@@ -447,7 +447,7 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 			victim = mdmon_pid(container->devnum, pid_dir);
 		}
 		if (victim >= 0)
-			victim_sock = connect_monitor(container->devname);
+			victim_sock = connect_monitor(container->devname, pid_dir);
 	}
 
 	ignore = chdir("/");

msg.c +12 -3

index d2d8445..78a7964 100644
@@ -138,7 +138,7 @@ int wait_reply(int fd, int tmo)
 	return receive_message(fd, &msg, tmo);
 }
 
-int connect_monitor(char *devname)
+int connect_monitor(char *devname, const char *pid_dir)
 {
 	char path[100];
 	int sfd;

@@ -147,6 +147,15 @@ int connect_monitor(char *devname)
 	int pos;
 	char *c;
 
+	/* if pid_dir is null try to detect it */
+	if (!pid_dir) {
+		sfd = connect_monitor(devname, VAR_RUN);
+		if (sfd >= 0)
+			return sfd;
+		else
+			return connect_monitor(devname, ALT_RUN);
+	}
+
 	pos = sprintf(path, "%s/", pid_dir);
 	if (is_subarray(devname)) {
 		devname++;

@@ -199,7 +208,7 @@ int fping_monitor(int sfd)
 /* give the monitor a chance to update the metadata */
 int ping_monitor(char *devname)
 {
-	int sfd = connect_monitor(devname);
+	int sfd = connect_monitor(devname, NULL);
 	int err = fping_monitor(sfd);
 
 	close(sfd);

@@ -213,7 +222,7 @@ int ping_monitor(char *devname)
  */
 int ping_manager(char *devname)
 {
-	int sfd = connect_monitor(devname);
+	int sfd = connect_monitor(devname, NULL);
 	struct metadata_update msg = { .len = -1 };
 	int err = 0;

msg.h +1 -1

index f8e89fd..46299db 100644
@@ -25,7 +25,7 @@ extern int receive_message(int fd, struct metadata_update *msg, int tmo);
 extern int send_message(int fd, struct metadata_update *msg, int tmo);
 extern int ack(int fd, int tmo);
 extern int wait_reply(int fd, int tmo);
-extern int connect_monitor(char *devname);
+extern int connect_monitor(char *devname, const char *pid_dir);
 extern int ping_monitor(char *devname);
 extern int fping_monitor(int sock);
 extern int ping_manager(char *devname);

util.c +1 -1

index 50e04bf..c22886b 100644
@@ -1585,7 +1585,7 @@ int flush_metadata_updates(struct supertype *st)
 		return -1;
 	}
 
-	sfd = connect_monitor(devnum2devname(st->container_dev));
+	sfd = connect_monitor(devnum2devname(st->container_dev), NULL);
 	if (sfd < 0)
 		return -1;

Signature · 10 lines

1.7.0


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

Neil Brown

2010-03-03 01:51 UTC #19 lore raw

Re: [mdadm PATCH 6/9] mdmon: connect_monitor should use socket from either dir

Quoted 2 lines · On Sun, 28 Feb 2010 15:44:24 +0100 Luca Berra <[email protected]> wrote:

connect_monitor is called by ping_monitor in various places the socket should be checked in either VAR_RUN or ALT_RUN

I agree with this even less, for much the same reasons.

If you still see a need for something like this in my latest git tree (not yet published - wait an hour or two at least), please explain.

Thanks,
NeilBrown

Quoted 95 lines

Signed-off-by Luca Berra <[email protected]>

---
  mdmon.c |    2 +-
  msg.c   |   15 ++++++++++++---
  msg.h   |    2 +-
  util.c  |    2 +-
  4 files changed, 15 insertions(+), 6 deletions(-)

mdmon.c +1 -1

index 3410e84..3627a80 100644
@@ -447,7 +447,7 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  			victim = mdmon_pid(container->devnum, pid_dir);
  		}
  		if (victim >= 0)
-			victim_sock = connect_monitor(container->devname);
+			victim_sock = connect_monitor(container->devname, pid_dir);
  	}
 
  	ignore = chdir("/");

msg.c +12 -3

index d2d8445..78a7964 100644
@@ -138,7 +138,7 @@ int wait_reply(int fd, int tmo)
  	return receive_message(fd, &msg, tmo);
  }
 
-int connect_monitor(char *devname)
+int connect_monitor(char *devname, const char *pid_dir)
  {
  	char path[100];
  	int sfd;

@@ -147,6 +147,15 @@ int connect_monitor(char *devname)
  	int pos;
  	char *c;
 
+	/* if pid_dir is null try to detect it */
+	if (!pid_dir) {
+		sfd = connect_monitor(devname, VAR_RUN);
+		if (sfd >= 0)
+			return sfd;
+		else
+			return connect_monitor(devname, ALT_RUN);
+	}
+
  	pos = sprintf(path, "%s/", pid_dir);
  	if (is_subarray(devname)) {
  		devname++;

@@ -199,7 +208,7 @@ int fping_monitor(int sfd)
  /* give the monitor a chance to update the metadata */
  int ping_monitor(char *devname)
  {
-	int sfd = connect_monitor(devname);
+	int sfd = connect_monitor(devname, NULL);
  	int err = fping_monitor(sfd);
 
  	close(sfd);

@@ -213,7 +222,7 @@ int ping_monitor(char *devname)
   */
  int ping_manager(char *devname)
  {
-	int sfd = connect_monitor(devname);
+	int sfd = connect_monitor(devname, NULL);
  	struct metadata_update msg = { .len = -1 };
  	int err = 0;

msg.h +1 -1

index f8e89fd..46299db 100644
@@ -25,7 +25,7 @@ extern int receive_message(int fd, struct metadata_update *msg, int tmo);
  extern int send_message(int fd, struct metadata_update *msg, int tmo);
  extern int ack(int fd, int tmo);
  extern int wait_reply(int fd, int tmo);
-extern int connect_monitor(char *devname);
+extern int connect_monitor(char *devname, const char *pid_dir);
  extern int ping_monitor(char *devname);
  extern int fping_monitor(int sock);
  extern int ping_manager(char *devname);

util.c +1 -1

index 50e04bf..c22886b 100644
@@ -1585,7 +1585,7 @@ int flush_metadata_updates(struct supertype *st)
  		return -1;
  	}
 
-	sfd = connect_monitor(devnum2devname(st->container_dev));
+	sfd = connect_monitor(devnum2devname(st->container_dev), NULL);
  	if (sfd < 0)
  		return -1;

Luca Berra

author

2010-02-28 14:45 UTC #20 lore raw

PATCH 7/9 mdmon: move pid_dir to mdmon.c

In reply to Luca Berra

since it is not used elsewere anymore

Signed-off-by Luca Berra <[email protected]>

---
 mdadm.h |    1 -
 mdmon.c |    2 ++
 util.c  |    1 -
 3 files changed, 2 insertions(+), 2 deletions(-)

mdadm.h +0 -1

index 7efa8bf..825e3c2 100644
@@ -895,7 +895,6 @@ extern int create_mddev(char *dev, char *name, int autof, int trustworthy,
 extern int open_mddev(char *dev, int report_errors);
 extern int open_container(int fd);
 
-extern char *pid_dir;
 extern int mdmon_running(int devnum);
 extern int mdmon_pid(int devnum, const char *pid_dir);
 extern int check_env(char *name);

mdmon.c +2 -0

index 3627a80..eef4bfa 100644
@@ -71,6 +71,8 @@ int mon_tid, mgr_tid;
 
 int sigterm;
 
+static char *pid_dir = VAR_RUN;
+
 int run_child(void *v)
 {
 	struct supertype *c = v;

util.c +0 -1

index c22886b..800d4b2 100644
@@ -1463,7 +1463,6 @@ int fd2devnum(int fd)
 	return NoMdDev;
 }
 
-char *pid_dir = VAR_RUN;
 
 int mdmon_pid(int devnum, const char *pid_dir)
 {

Signature · 10 lines

1.7.0


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

Luca Berra

author

2010-02-28 14:45 UTC #21 lore raw

PATCH 8/9 mdmon: rework startup and takeover logic

In reply to Luca Berra

rework startup and takeover logic in order to make it more robust

Signed-off-by Luca Berra <[email protected]>

---
 mdmon.c |   45 +++++++++++++++++++++------------------------
 1 files changed, 21 insertions(+), 24 deletions(-)

mdmon.c +21 ~~-24~~

index eef4bfa..b823a8c 100644
@@ -71,7 +71,7 @@ int mon_tid, mgr_tid;
 
 int sigterm;
 
-static char *pid_dir = VAR_RUN;
+static char *pid_dir = NULL;
 
 int run_child(void *v)
 {

@@ -189,9 +189,6 @@ void remove_pidfile(char *devname)
 	unlink(buf);
 	sprintf(buf, "%s/%s.sock", pid_dir, devname);
 	unlink(buf);
-	if (strcmp(pid_dir, ALT_RUN) == 0)
-		/* try to clean up when we are finished with this dir */
-		rmdir(pid_dir);
 }
 
 static int make_control_sock(char *devname)

@@ -443,26 +440,20 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 	sigaction(SIGPIPE, &act, NULL);
 
 	if (takeover) {
-		victim = mdmon_pid(container->devnum, pid_dir);
-		if (victim < 0) {
-			pid_dir = ALT_RUN;
-			victim = mdmon_pid(container->devnum, pid_dir);
+		/* Check the parent process by socket, not pid, which could be
+		 * stale, then get the pid from the same pid_dir */
+		char * victim_pid_dir = VAR_RUN;
+		victim_sock = connect_monitor(container->devname, victim_pid_dir);
+		if (victim_sock < 0) {
+			victim_pid_dir = ALT_RUN;
+			victim_sock = connect_monitor(container->devname, victim_pid_dir);
 		}
-		if (victim >= 0)
-			victim_sock = connect_monitor(container->devname, pid_dir);
+		if (victim_sock >= 0)
+ 			victim = mdmon_pid(container->devnum, victim_pid_dir);
 	}
 
 	ignore = chdir("/");
 	if (victim < 0) {
-		pid_dir = ALT_RUN;
-		if (ping_monitor(container->devname) == 0) {
-			fprintf(stderr, "mdmon: %s already managed\n",
-				container->devname);
-			if (!takeover)
-				fprintf(stderr, "\trun mdmon --takeover instead\n");
-			exit(3);
-		}
-		pid_dir = VAR_RUN;
 		if (ping_monitor(container->devname) == 0) {
 			fprintf(stderr, "mdmon: %s already managed\n",
 				container->devname);

@@ -479,14 +470,20 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 	close(mdfd);
 
 	/* Ok, this is close enough.  We can say goodbye to our parent now.
+	 * take care to remove all possible pid files and sockets.
 	 */
-	if (victim > 0)
+	if (mkdir(ALT_RUN, 0600) >= 0 || (errno == EEXIST && access(ALT_RUN, W_OK) >= 0)) {
+		pid_dir = ALT_RUN;
 		remove_pidfile(devname);
-	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0))
+	}
+	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0)) {
+		/* try to clean up when we are finished with ALT_RUN dir */
+		if (pid_dir != NULL)
+			rmdir(pid_dir);
 		pid_dir = VAR_RUN;
-	else if (mkdir(ALT_RUN, 0600) >= 0 || (errno == EEXIST && access(ALT_RUN, W_OK) >= 0))
-		pid_dir = ALT_RUN;
-	else {
+		remove_pidfile(devname);
+	}
+	if (pid_dir == NULL) {
 		fprintf(stderr, "mdmon: Neither %s nor %s are writable\n"
 			"       cannot create .pid or .sock files.  Aborting\n",
 			VAR_RUN, ALT_RUN);

Signature · 10 lines

1.7.0


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

Neil Brown

2010-03-03 01:52 UTC #22 lore raw

Re: [mdadm PATCH 8/9] mdmon: rework startup and takeover logic

Quoted 1 line · On Sun, 28 Feb 2010 15:45:45 +0100 Luca Berra <[email protected]> wrote:

rework startup and takeover logic in order to make it more robust

With the other things I dropped, this no longer applied, and the I didn't feel up to picking it apart to see what you were really trying to do. Again, if you feel something here is still needed, please explain.

Thanks,
NeilBrown

Quoted 91 lines

Signed-off-by Luca Berra <[email protected]>

---
  mdmon.c |   45 +++++++++++++++++++++------------------------
  1 files changed, 21 insertions(+), 24 deletions(-)

mdmon.c +21 ~~-24~~

index eef4bfa..b823a8c 100644
@@ -71,7 +71,7 @@ int mon_tid, mgr_tid;
 
  int sigterm;
 
-static char *pid_dir = VAR_RUN;
+static char *pid_dir = NULL;
 
  int run_child(void *v)
  {

@@ -189,9 +189,6 @@ void remove_pidfile(char *devname)
  	unlink(buf);
  	sprintf(buf, "%s/%s.sock", pid_dir, devname);
  	unlink(buf);
-	if (strcmp(pid_dir, ALT_RUN) == 0)
-		/* try to clean up when we are finished with this dir */
-		rmdir(pid_dir);
  }
 
  static int make_control_sock(char *devname)

@@ -443,26 +440,20 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  	sigaction(SIGPIPE, &act, NULL);
 
  	if (takeover) {
-		victim = mdmon_pid(container->devnum, pid_dir);
-		if (victim < 0) {
-			pid_dir = ALT_RUN;
-			victim = mdmon_pid(container->devnum, pid_dir);
+		/* Check the parent process by socket, not pid, which could be
+		 * stale, then get the pid from the same pid_dir */
+		char * victim_pid_dir = VAR_RUN;
+		victim_sock = connect_monitor(container->devname, victim_pid_dir);
+		if (victim_sock < 0) {
+			victim_pid_dir = ALT_RUN;
+			victim_sock = connect_monitor(container->devname, victim_pid_dir);
  		}
-		if (victim >= 0)
-			victim_sock = connect_monitor(container->devname, pid_dir);
+		if (victim_sock >= 0)
+ 			victim = mdmon_pid(container->devnum, victim_pid_dir);
  	}
 
  	ignore = chdir("/");
  	if (victim < 0) {
-		pid_dir = ALT_RUN;
-		if (ping_monitor(container->devname) == 0) {
-			fprintf(stderr, "mdmon: %s already managed\n",
-				container->devname);
-			if (!takeover)
-				fprintf(stderr, "\trun mdmon --takeover instead\n");
-			exit(3);
-		}
-		pid_dir = VAR_RUN;
  		if (ping_monitor(container->devname) == 0) {
  			fprintf(stderr, "mdmon: %s already managed\n",
  				container->devname);

@@ -479,14 +470,20 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  	close(mdfd);
 
  	/* Ok, this is close enough.  We can say goodbye to our parent now.
+	 * take care to remove all possible pid files and sockets.
  	 */
-	if (victim > 0)
+	if (mkdir(ALT_RUN, 0600) >= 0 || (errno == EEXIST && access(ALT_RUN, W_OK) >= 0)) {
+		pid_dir = ALT_RUN;
  		remove_pidfile(devname);
-	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0))
+	}
+	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0)) {
+		/* try to clean up when we are finished with ALT_RUN dir */
+		if (pid_dir != NULL)
+			rmdir(pid_dir);
  		pid_dir = VAR_RUN;
-	else if (mkdir(ALT_RUN, 0600) >= 0 || (errno == EEXIST && access(ALT_RUN, W_OK) >= 0))
-		pid_dir = ALT_RUN;
-	else {
+		remove_pidfile(devname);
+	}
+	if (pid_dir == NULL) {
  		fprintf(stderr, "mdmon: Neither %s nor %s are writable\n"
  			"       cannot create .pid or .sock files.  Aborting\n",
  			VAR_RUN, ALT_RUN);

Luca Berra

author

2010-03-03 06:48 UTC #23 lore raw

Re: [mdadm PATCH 8/9] mdmon: rework startup and takeover logic

Quoted 8 lines · Neil Brown

Quoted 1 line · On Sun, 28 Feb 2010 15:45:45 +0100 Luca Berra <[email protected]> wrote:
rework startup and takeover logic in order to make it more robust
With the other things I dropped, this no longer applied, and the I didn't feel up to picking it apart to see what you were really trying to do. Again, if you feel something here is still needed, please explain.

Since this depends on all the other things you dropped there isn't much point in analyzing this.
I will look git when you update it and base my comments on that code.

Regards,
L.

Signature · 6 lines

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

Luca Berra

author

2010-02-28 14:46 UTC #24 lore raw

PATCH 9/9 allow redefinition of VAR_RUN

In reply to Luca Berra

having mdmon socket under var is painful at shutdown time

Signed-off-by Luca Berra <[email protected]>

---
 Makefile |    8 ++++++--
 mdadm.h  |    2 ++
 mdmon.c  |    3 ---
 3 files changed, 8 insertions(+), 5 deletions(-)

Makefile +6 -2

index 1836b4b..1fffb26 100644
@@ -62,9 +62,13 @@ CONFFILEFLAGS = -DCONFFILE=\"$(CONFFILE)\" -DCONFFILE2=\"$(CONFFILE2)\"
 # from early boot to late boot.
 # 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
+ifdef ALT_RUN
 ALTFLAGS = -DALT_RUN=\"$(ALT_RUN)\"
-CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(ALTFLAGS)
+endif
+ifdef VAR_RUN
+VARFLAGS = -DVAR_RUN=\"$(VAR_RUN)\"
+endif
+CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(ALTFLAGS) $(VARFLAGS)
 
 # If you want a static binary, you might uncomment these
 # LDFLAGS = -static

mdadm.h +2 -0

index 825e3c2..2ad4a0e 100644
@@ -68,7 +68,9 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
 #define DEFAULT_BITMAP_DELAY 5
 #define DEFAULT_MAX_WRITE_BEHIND 256
 
+#ifndef VAR_RUN
 #define VAR_RUN "/var/run/mdadm"
+#endif /* VAR_RUN */
 /* ALT_RUN should be somewhere that persists across the pivotroot
  * from early boot to late boot.
  * If you don't have /lib/init/rw you might want to use /dev/.something

mdmon.c +0 -3

index b823a8c..6570637 100644
@@ -477,9 +477,6 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
 		remove_pidfile(devname);
 	}
 	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0)) {
-		/* try to clean up when we are finished with ALT_RUN dir */
-		if (pid_dir != NULL)
-			rmdir(pid_dir);
 		pid_dir = VAR_RUN;
 		remove_pidfile(devname);
 	}

Signature · 10 lines

1.7.0


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

Neil Brown

2010-03-03 01:53 UTC #25 lore raw

Re: [mdadm PATCH 9/9] allow redefinition of VAR_RUN

Quoted 1 line · On Sun, 28 Feb 2010 15:46:16 +0100 Luca Berra <[email protected]> wrote:

having mdmon socket under var is painful at shutdown time

Thanks. I have applied this after some minor modifications, just as removing the "ifdef" stuff from Makefile and adding some commentary.

Thanks,
NeilBrown

Quoted 56 lines

Signed-off-by Luca Berra <[email protected]>

---
  Makefile |    8 ++++++--
  mdadm.h  |    2 ++
  mdmon.c  |    3 ---
  3 files changed, 8 insertions(+), 5 deletions(-)

Makefile +6 -2

index 1836b4b..1fffb26 100644
@@ -62,9 +62,13 @@ CONFFILEFLAGS = -DCONFFILE=\"$(CONFFILE)\" -DCONFFILE2=\"$(CONFFILE2)\"
  # from early boot to late boot.
  # 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
+ifdef ALT_RUN
  ALTFLAGS = -DALT_RUN=\"$(ALT_RUN)\"
-CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(ALTFLAGS)
+endif
+ifdef VAR_RUN
+VARFLAGS = -DVAR_RUN=\"$(VAR_RUN)\"
+endif
+CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(ALTFLAGS) $(VARFLAGS)
 
  # If you want a static binary, you might uncomment these
  # LDFLAGS = -static

mdadm.h +2 -0

index 825e3c2..2ad4a0e 100644
@@ -68,7 +68,9 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
  #define DEFAULT_BITMAP_DELAY 5
  #define DEFAULT_MAX_WRITE_BEHIND 256
 
+#ifndef VAR_RUN
  #define VAR_RUN "/var/run/mdadm"
+#endif /* VAR_RUN */
  /* ALT_RUN should be somewhere that persists across the pivotroot
   * from early boot to late boot.
   * If you don't have /lib/init/rw you might want to use /dev/.something

mdmon.c +0 -3

index b823a8c..6570637 100644
@@ -477,9 +477,6 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
  		remove_pidfile(devname);
  	}
  	if (mkdir(VAR_RUN, 0600) >= 0 || (errno == EEXIST && access(VAR_RUN, W_OK) >= 0)) {
-		/* try to clean up when we are finished with ALT_RUN dir */
-		if (pid_dir != NULL)
-			rmdir(pid_dir);
  		pid_dir = VAR_RUN;
  		remove_pidfile(devname);
  	}