[mdadm PATCH 0/2] *** mdmon fixes ***

[mdadm PATCH 0/2] *** mdmon fixes ***

[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

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

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

[mdadm PATCH 1/2] fix mdmon option parsing

[Luca Berra]

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

Signed-off-by: Luca Berra <bluca@comedia.it>
---
  mdmon.c |   13 +++++++++----
  1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index 70fbd0b..d593ebc 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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;
-- 
1.7.0

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Luca Berra]

On Sat, Feb 27, 2010 at 04:07:04PM +0100, Luca Berra wrote:
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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
>  	exit(2);
>  }
>  @@ -262,15 +262,20 @@ int main(int argc, char *argv[])

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Sat, 27 Feb 2010 16:07:04 +0100
Luca Berra <bluca@comedia.it> 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

> 
> Signed-off-by: Luca Berra <bluca@comedia.it>
> ---
>   mdmon.c |   13 +++++++++----
>   1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/mdmon.c b/mdmon.c
> index 70fbd0b..d593ebc 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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;

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

[Luca Berra]

On Tue, Mar 02, 2010 at 04:40:34PM +1100, Neil Brown wrote:
>On Sat, 27 Feb 2010 16:07:04 +0100
>Luca Berra <bluca@comedia.it> 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.

L.

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

[mdadm PATCH 2/2] fix mdmon takeover

[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 <bluca@comedia.it>
---
  mdmon.c |   10 +++++++++-
  msg.c   |    2 +-
  util.c  |    4 ++--
  3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index d593ebc..a2b7946 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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);
diff --git a/msg.c b/msg.c
index cc07b96..d2d8445 100644
--- a/msg.c
+++ b/msg.c
@@ -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, '/');
diff --git a/util.c b/util.c
index 1def2a0..2fe566d 100644
--- a/util.c
+++ b/util.c
@@ -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);
  }
  
-- 
1.7.0

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Sat, 27 Feb 2010 16:07:51 +0100
Luca Berra <bluca@comedia.it> 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 <bluca@comedia.it>
> ---
>   mdmon.c |   10 +++++++++-
>   msg.c   |    2 +-
>   util.c  |    4 ++--
>   3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/mdmon.c b/mdmon.c
> index d593ebc..a2b7946 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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?

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

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

> diff --git a/msg.c b/msg.c
> index cc07b96..d2d8445 100644
> --- a/msg.c
> +++ b/msg.c
> @@ -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.

> diff --git a/util.c b/util.c
> index 1def2a0..2fe566d 100644
> --- a/util.c
> +++ b/util.c
> @@ -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);
>   }
>   

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

Thanks.
NeilBrown

Re: [mdadm PATCH 0/2] *** mdmon fixes ***

[Luca Berra]

On Sat, Feb 27, 2010 at 04:05:40PM +0100, Luca Berra wrote:
> 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.

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

[mdadm PATCH 3/9] add missing newline to mdmon usage message

[Luca Berra]

Signed-off-by: Luca Berra <bluca@comedia.it>
---
  mdmon.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index a2b7946..11b4f32 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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);
  }
  
-- 
1.7.0


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Luca Berra]

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

Signed-off-by: Luca Berra <bluca@comedia.it>
---
  mdmon.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index 11b4f32..57fd492 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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"
-- 
1.7.0


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Sun, 28 Feb 2010 15:42:57 +0100
Luca Berra <bluca@comedia.it> 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


> 
> Signed-off-by: Luca Berra <bluca@comedia.it>
> ---
>   mdmon.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mdmon.c b/mdmon.c
> index 11b4f32..57fd492 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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"

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

[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 <bluca@comedia.it>
---
  mdadm.h |    2 +-
  mdmon.c |    6 +++---
  util.c  |   14 ++++++++++++--
  3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/mdadm.h b/mdadm.h
index df3a056..7efa8bf 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -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);
diff --git a/mdmon.c b/mdmon.c
index 57fd492..3410e84 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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",
diff --git a/util.c b/util.c
index 2fe566d..50e04bf 100644
--- a/util.c
+++ b/util.c
@@ -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)
-- 
1.7.0


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Sun, 28 Feb 2010 15:43:40 +0100
Luca Berra <bluca@comedia.it> 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



> 
> Signed-off-by: Luca Berra <bluca@comedia.it>
> ---
>   mdadm.h |    2 +-
>   mdmon.c |    6 +++---
>   util.c  |   14 ++++++++++++--
>   3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/mdadm.h b/mdadm.h
> index df3a056..7efa8bf 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -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);
> diff --git a/mdmon.c b/mdmon.c
> index 57fd492..3410e84 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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",
> diff --git a/util.c b/util.c
> index 2fe566d..50e04bf 100644
> --- a/util.c
> +++ b/util.c
> @@ -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)

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

[Luca Berra]

On Wed, Mar 03, 2010 at 12:50:31PM +1100, Neil Brown wrote:
>On Sun, 28 Feb 2010 15:43:40 +0100
>Luca Berra <bluca@comedia.it> 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??

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.

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Wed, 3 Mar 2010 07:44:03 +0100
Luca Berra <bluca@comedia.it> wrote:

> On Wed, Mar 03, 2010 at 12:50:31PM +1100, Neil Brown wrote:
> >On Sun, 28 Feb 2010 15:43:40 +0100
> >Luca Berra <bluca@comedia.it> 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??
> 
> 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.

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

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

[Luca Berra]

On Wed, Mar 03, 2010 at 05:55:54PM +1100, Neil Brown wrote:
>On Wed, 3 Mar 2010 07:44:03 +0100
>Luca Berra <bluca@comedia.it> wrote:
>
>> On Wed, Mar 03, 2010 at 12:50:31PM +1100, Neil Brown wrote:
>> >On Sun, 28 Feb 2010 15:43:40 +0100
>> >Luca Berra <bluca@comedia.it> 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??
>> 
>> 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.
>
>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)

L.

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[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 <bluca@comedia.it>
---
  mdmon.c |    2 +-
  msg.c   |   15 ++++++++++++---
  msg.h   |    2 +-
  util.c  |    2 +-
  4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index 3410e84..3627a80 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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("/");
diff --git a/msg.c b/msg.c
index d2d8445..78a7964 100644
--- a/msg.c
+++ b/msg.c
@@ -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;
  
diff --git a/msg.h b/msg.h
index f8e89fd..46299db 100644
--- a/msg.h
+++ b/msg.h
@@ -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);
diff --git a/util.c b/util.c
index 50e04bf..c22886b 100644
--- a/util.c
+++ b/util.c
@@ -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;
  
-- 
1.7.0


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Sun, 28 Feb 2010 15:44:24 +0100
Luca Berra <bluca@comedia.it> 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

> 
> Signed-off-by: Luca Berra <bluca@comedia.it>
> ---
>   mdmon.c |    2 +-
>   msg.c   |   15 ++++++++++++---
>   msg.h   |    2 +-
>   util.c  |    2 +-
>   4 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/mdmon.c b/mdmon.c
> index 3410e84..3627a80 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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("/");
> diff --git a/msg.c b/msg.c
> index d2d8445..78a7964 100644
> --- a/msg.c
> +++ b/msg.c
> @@ -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;
>   
> diff --git a/msg.h b/msg.h
> index f8e89fd..46299db 100644
> --- a/msg.h
> +++ b/msg.h
> @@ -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);
> diff --git a/util.c b/util.c
> index 50e04bf..c22886b 100644
> --- a/util.c
> +++ b/util.c
> @@ -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;
>   

[mdadm PATCH 7/9] mdmon: move pid_dir to mdmon.c

[Luca Berra]

since it is not used elsewere anymore

Signed-off-by: Luca Berra <bluca@comedia.it>
---
  mdadm.h |    1 -
  mdmon.c |    2 ++
  util.c  |    1 -
  3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mdadm.h b/mdadm.h
index 7efa8bf..825e3c2 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -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);
diff --git a/mdmon.c b/mdmon.c
index 3627a80..eef4bfa 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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;
diff --git a/util.c b/util.c
index c22886b..800d4b2 100644
--- a/util.c
+++ b/util.c
@@ -1463,7 +1463,6 @@ int fd2devnum(int fd)
  	return NoMdDev;
  }
  
-char *pid_dir = VAR_RUN;
  
  int mdmon_pid(int devnum, const char *pid_dir)
  {
-- 
1.7.0


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Luca Berra]

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

Signed-off-by: Luca Berra <bluca@comedia.it>
---
  mdmon.c |   45 +++++++++++++++++++++------------------------
  1 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index eef4bfa..b823a8c 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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);
-- 
1.7.0


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Sun, 28 Feb 2010 15:45:45 +0100
Luca Berra <bluca@comedia.it> 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


> 
> Signed-off-by: Luca Berra <bluca@comedia.it>
> ---
>   mdmon.c |   45 +++++++++++++++++++++------------------------
>   1 files changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/mdmon.c b/mdmon.c
> index eef4bfa..b823a8c 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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);

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

[Luca Berra]

On Wed, Mar 03, 2010 at 12:52:39PM +1100, Neil Brown wrote:
>On Sun, 28 Feb 2010 15:45:45 +0100
>Luca Berra <bluca@comedia.it> 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.

-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

[mdadm PATCH 9/9] allow redefinition of VAR_RUN

[Luca Berra]

having mdmon socket under var is painful at shutdown time

Signed-off-by: Luca Berra <bluca@comedia.it>
---
  Makefile |    8 ++++++--
  mdadm.h  |    2 ++
  mdmon.c  |    3 ---
  3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 1836b4b..1fffb26 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/mdadm.h b/mdadm.h
index 825e3c2..2ad4a0e 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -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
diff --git a/mdmon.c b/mdmon.c
index b823a8c..6570637 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -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);
  	}
-- 
1.7.0


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \

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

[Neil Brown]

On Sun, 28 Feb 2010 15:46:16 +0100
Luca Berra <bluca@comedia.it> 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


> 
> Signed-off-by: Luca Berra <bluca@comedia.it>
> ---
>   Makefile |    8 ++++++--
>   mdadm.h  |    2 ++
>   mdmon.c  |    3 ---
>   3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 1836b4b..1fffb26 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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
> diff --git a/mdadm.h b/mdadm.h
> index 825e3c2..2ad4a0e 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -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
> diff --git a/mdmon.c b/mdmon.c
> index b823a8c..6570637 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -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);
>   	}

Re: [mdadm PATCH 0/2] *** mdmon fixes ***

[Bill Davidsen]

Luca Berra wrote:
> On Sat, Feb 27, 2010 at 04:05:40PM +0100, Luca Berra wrote:
>> 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

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.

-- 
Bill Davidsen <davidsen@tmr.com>
  "We can't solve today's problems by using the same thinking we
   used in creating them." - Einstein

Re: [mdadm PATCH 0/2] *** mdmon fixes ***

[Luca Berra]

On Sun, Feb 28, 2010 at 10:41:35AM -0500, Bill Davidsen wrote:
> Luca Berra wrote:
>> On Sat, Feb 27, 2010 at 04:05:40PM +0100, Luca Berra wrote:
>>> 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
>
> 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)

> 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.

L.


-- 
Luca Berra -- bluca@comedia.it
         Communication Media & Services S.r.l.
  /"\
  \ /     ASCII RIBBON CAMPAIGN
   X        AGAINST HTML MAIL
  / \