[PATCH v2 0/3] support launching mdmon via systemctl

[PATCH v2 0/3] support launching mdmon via systemctl

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

This is v2 of the patch to support launching mdmon via systemctl/
systemd. It now provides a command line option --systemctl, as well as
the two systemd .service files.

Cheers,
Jes


Jes Sorensen (3):
  Add support for launching mdmon via systemctl instead of fork/exec
  systemd .service files for launching mdmon via systemctl
  execl() only returns in case of error

 ReadMe.c                       |  2 ++
 mdadm.c                        |  4 ++++
 mdadm.h                        |  2 ++
 mdmon.c                        |  4 +++-
 systemd/mdmon-offroot@.service | 19 +++++++++++++++++++
 systemd/mdmon@.service         | 13 +++++++++++++
 util.c                         | 42 ++++++++++++++++++++++++++++++------------
 7 files changed, 73 insertions(+), 13 deletions(-)
 create mode 100644 systemd/mdmon-offroot@.service
 create mode 100644 systemd/mdmon@.service

-- 
1.7.11.7

[PATCH 1/3] Add support for launching mdmon via systemctl instead of fork/exec

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

To launch mdmon via systemctl, a new command line argument is added to
mdadm '--systemctl'. Alternatively it is possible to set the
environment variable MDMON_SYSTEMCTL.

This allows for having mdmon launched via systemctl which avoids
problems with it getting killed by systemd due to it ending up in the
parent's cgroup (udev).

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 ReadMe.c |  2 ++
 mdadm.c  |  4 ++++
 mdadm.h  |  2 ++
 mdmon.c  |  4 +++-
 util.c   | 41 ++++++++++++++++++++++++++++++-----------
 5 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/ReadMe.c b/ReadMe.c
index 0aa8cbd..69b8597 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -95,6 +95,7 @@ struct option long_options[] = {
     {"update-subarray", 1, 0, UpdateSubarray},
     {"udev-rules", 2, 0, UdevRules},
     {"offroot", 0, 0, OffRootOpt},
+    {"systemctl", 0, 0, SystemctlOpt},
 
     /* synonyms */
     {"monitor",   0, 0, 'F'},
@@ -261,6 +262,7 @@ char OptionHelp[] =
 "                       application was launched from initrd/initramfs and\n"
 "                       should not be shutdown by systemd as part of the\n"
 "                       regular shutdown process.\n"
+"  --systemctl        : Use systemctl to launch mdmon rather than exec/fork\n"
 ;
 /*
 "\n"
diff --git a/mdadm.c b/mdadm.c
index 11016e7..4c8d382 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -176,6 +176,10 @@ int main(int argc, char *argv[])
 				c.prefer = NULL;
 			continue;
 
+		case SystemctlOpt:
+			__mdmon_systemctl = 1;
+			continue;
+
 		case ':':
 		case '?':
 			fputs(Usage, stderr);
diff --git a/mdadm.h b/mdadm.h
index f1352e3..54874cb 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -337,6 +337,7 @@ enum special_options {
 	Prefer,
 	KillOpt,
 	DataOffset,
+	SystemctlOpt,
 };
 
 enum prefix_standard {
@@ -1485,3 +1486,4 @@ char *xstrdup(const char *str);
 #define VARIABLE_OFFSET 3
 
 extern int __offroot;
+extern int __mdmon_systemctl;
diff --git a/mdmon.c b/mdmon.c
index 5d5ae94..7459ae2 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -188,7 +188,9 @@ static void try_kill_monitor(pid_t pid, char *devname, int sock)
 	 * might be "@dmon"
 	 */
 	if (n < 0 || !(strstr(buf, "mdmon") ||
-		       strstr(buf, "@dmon")))
+		       strstr(buf, "@dmon") ||
+		       strstr(buf, "/usr/sbin/mdmon") ||
+		       strstr(buf, "@usr/sbin/mdmon")))
 		return;
 
 	kill(pid, SIGTERM);
diff --git a/util.c b/util.c
index 6c10365..500b2bd 100644
--- a/util.c
+++ b/util.c
@@ -33,6 +33,7 @@
 #include	<signal.h>
 
 int __offroot;
+int __mdmon_systemctl;
 
 /*
  * following taken from linux/blkpg.h because they aren't
@@ -1651,6 +1652,9 @@ int start_mdmon(int devnum)
 	if (check_env("MDADM_NO_MDMON"))
 		return 0;
 
+	if (check_env("MDMON_SYSTEMCTL"))
+		__mdmon_systemctl = 1;
+
 	len = readlink("/proc/self/exe", pathbuf, sizeof(pathbuf)-1);
 	if (len > 0) {
 		char *sl;
@@ -1674,18 +1678,33 @@ int start_mdmon(int devnum)
 			else
 				skipped = 0;
 
-		for (i = 0; paths[i]; i++)
-			if (paths[i][0]) {
-				if (__offroot) {
-					execl(paths[i], "mdmon", "--offroot",
-					      devnum2devname(devnum),
-					      NULL);
-				} else {
-					execl(paths[i], "mdmon",
-					      devnum2devname(devnum),
-					      NULL);
-				}
+		if (__mdmon_systemctl) {
+			if (__offroot) {
+				snprintf(pathbuf, 40, "mdmon-offroot@%s.service",
+					 devnum2devname(devnum));
+				execl("/usr/bin/systemctl", "systemctl",
+				      "start", pathbuf, NULL);
+			} else {
+				snprintf(pathbuf, 30, "mdmon@%s.service",
+					 devnum2devname(devnum));
+				execl("/usr/bin/systemctl", "systemctl",
+				      "start", pathbuf, NULL);
 			}
+		} else {
+			for (i = 0; paths[i]; i++)
+				if (paths[i][0]) {
+					if (__offroot) {
+						execl(paths[i], "mdmon",
+						      "--offroot",
+						      devnum2devname(devnum),
+						      NULL);
+					} else {
+						execl(paths[i], "mdmon",
+						      devnum2devname(devnum),
+						      NULL);
+					}
+				}
+		}
 		exit(1);
 	case -1: pr_err("cannot run mdmon. "
 			 "Array remains readonly\n");
-- 
1.7.11.7

[PATCH 2/3] systemd .service files for launching mdmon via systemctl

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 systemd/mdmon-offroot@.service | 19 +++++++++++++++++++
 systemd/mdmon@.service         | 13 +++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 systemd/mdmon-offroot@.service
 create mode 100644 systemd/mdmon@.service

diff --git a/systemd/mdmon-offroot@.service b/systemd/mdmon-offroot@.service
new file mode 100644
index 0000000..77bb04b
--- /dev/null
+++ b/systemd/mdmon-offroot@.service
@@ -0,0 +1,19 @@
+#  This file is part of mdadm.
+#
+#  mdadm is free software; you can redistribute it and/or modify it
+#  under the terms of the GNU General Public License as published by
+#  the Free Software Foundation; either version 2 of the License, or
+#  (at your option) any later version.
+
+[Unit]
+Description=MD Metadata Monitor on /dev/%I
+DefaultDependencies=no
+Before=initrd-switch-root.target
+ConditionPathExists=/etc/initrd-release
+
+[Service]
+ExecStart=/usr/sbin/mdmon --offroot %I
+StandardInput=null
+StandardOutput=null
+StandardError=null
+KillMode=none
diff --git a/systemd/mdmon@.service b/systemd/mdmon@.service
new file mode 100644
index 0000000..49d04ec
--- /dev/null
+++ b/systemd/mdmon@.service
@@ -0,0 +1,13 @@
+#  This file is part of mdadm.
+#
+#  mdadm is free software; you can redistribute it and/or modify it
+#  under the terms of the GNU General Public License as published by
+#  the Free Software Foundation; either version 2 of the License, or
+#  (at your option) any later version.
+
+[Unit]
+Description=MD Metadata Monitor on /dev/%I
+DefaultDependencies=no
+
+[Service]
+ExecStart=/usr/sbin/mdmon %I
-- 
1.7.11.7

[PATCH 3/3] execl() only returns in case of error

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

In case exec fails, fall through and print warning rather than just
calling exit(1) silently.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 util.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util.c b/util.c
index 500b2bd..68fd27e 100644
--- a/util.c
+++ b/util.c
@@ -1705,7 +1705,6 @@ int start_mdmon(int devnum)
 					}
 				}
 		}
-		exit(1);
 	case -1: pr_err("cannot run mdmon. "
 			 "Array remains readonly\n");
 		return -1;
-- 
1.7.11.7

Re: [PATCH 1/3] Add support for launching mdmon via systemctl instead of fork/exec

[NeilBrown]

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

On Mon, 21 Jan 2013 14:22:56 +0100 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> To launch mdmon via systemctl, a new command line argument is added to
> mdadm '--systemctl'. Alternatively it is possible to set the
> environment variable MDMON_SYSTEMCTL.
> 
> This allows for having mdmon launched via systemctl which avoids
> problems with it getting killed by systemd due to it ending up in the
> parent's cgroup (udev).
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

Thanks Jes.  This is certainly something that we want in some form.
Some issues:

 - I have come to the conclusion that --offroot is a bad idea.  We should
   just make that the default.  No matter whether the array is providing the
   root filesystem or not, I never want systemd (or anything else) to kill
   mdmon.  I want it to remain in control.  So the systemctl handling should
   assume offroot. e.g. there should only be one .service file.

 - on my machine (openSUSE), systemctl is in /bin, not /usr/bin.
   Would I be correct in thinking that on your machine, /bin is a symlink
   to /usr/bin?  If so, maybe we can just use "/bin/systemctl"?  If not,
   we might need to try a few different options.
   Similarly I have mdmon in /sbin, not /usr/sbin.  We we cannot all
   use /sbin, we might need to 'sed' the .service file on installation to
   match the current system.
   (I note that you didn't get Makefile to install the .services files.  I
   think we want it to - at least optionally)

 - I want the default behaviour to "do the right thing" in most case, but it
   should be possible  to over-rid (by command line option or env var or
   both).
   So I think I would like it to try systemctl and if that fails for some
   reason (either because the binary doesn't exist, or because it finds that
   the .services file doesn't exist), then it should try to exec mdmon
   directly.
   This default could be over-ridden to always run mdmon directly.

Thanks!
NeilBrown



> +++ b/mdmon.c
> @@ -188,7 +188,9 @@ static void try_kill_monitor(pid_t pid, char *devname, int sock)
>  	 * might be "@dmon"
>  	 */
>  	if (n < 0 || !(strstr(buf, "mdmon") ||
> -		       strstr(buf, "@dmon")))
> +		       strstr(buf, "@dmon") ||
> +		       strstr(buf, "/usr/sbin/mdmon") ||
> +		       strstr(buf, "@usr/sbin/mdmon")))
>  		return;

As we are using "strstr", here, the second two cases will be matched by the
first case, so this change is unnecessary.


>  
>  	kill(pid, SIGTERM);
> diff --git a/util.c b/util.c
> index 6c10365..500b2bd 100644
> --- a/util.c
> +++ b/util.c
> @@ -33,6 +33,7 @@
>  #include	<signal.h>
>  
>  int __offroot;
> +int __mdmon_systemctl;
>  
>  /*
>   * following taken from linux/blkpg.h because they aren't
> @@ -1651,6 +1652,9 @@ int start_mdmon(int devnum)
>  	if (check_env("MDADM_NO_MDMON"))
>  		return 0;
>  
> +	if (check_env("MDMON_SYSTEMCTL"))
> +		__mdmon_systemctl = 1;
> +
>  	len = readlink("/proc/self/exe", pathbuf, sizeof(pathbuf)-1);
>  	if (len > 0) {
>  		char *sl;
> @@ -1674,18 +1678,33 @@ int start_mdmon(int devnum)
>  			else
>  				skipped = 0;
>  
> -		for (i = 0; paths[i]; i++)
> -			if (paths[i][0]) {
> -				if (__offroot) {
> -					execl(paths[i], "mdmon", "--offroot",
> -					      devnum2devname(devnum),
> -					      NULL);
> -				} else {
> -					execl(paths[i], "mdmon",
> -					      devnum2devname(devnum),
> -					      NULL);
> -				}
> +		if (__mdmon_systemctl) {
> +			if (__offroot) {
> +				snprintf(pathbuf, 40, "mdmon-offroot@%s.service",
> +					 devnum2devname(devnum));
> +				execl("/usr/bin/systemctl", "systemctl",
> +				      "start", pathbuf, NULL);
> +			} else {
> +				snprintf(pathbuf, 30, "mdmon@%s.service",
> +					 devnum2devname(devnum));
> +				execl("/usr/bin/systemctl", "systemctl",
> +				      "start", pathbuf, NULL);
>  			}
> +		} else {
> +			for (i = 0; paths[i]; i++)
> +				if (paths[i][0]) {
> +					if (__offroot) {
> +						execl(paths[i], "mdmon",
> +						      "--offroot",
> +						      devnum2devname(devnum),
> +						      NULL);
> +					} else {
> +						execl(paths[i], "mdmon",
> +						      devnum2devname(devnum),
> +						      NULL);
> +					}
> +				}
> +		}
>  		exit(1);
>  	case -1: pr_err("cannot run mdmon. "
>  			 "Array remains readonly\n");


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

Re: [PATCH 3/3] execl() only returns in case of error

[NeilBrown]

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

On Mon, 21 Jan 2013 14:22:58 +0100 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> In case exec fails, fall through and print warning rather than just
> calling exit(1) silently.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  util.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/util.c b/util.c
> index 500b2bd..68fd27e 100644
> --- a/util.c
> +++ b/util.c
> @@ -1705,7 +1705,6 @@ int start_mdmon(int devnum)
>  					}
>  				}
>  		}
> -		exit(1);
>  	case -1: pr_err("cannot run mdmon. "
>  			 "Array remains readonly\n");
>  		return -1;

???
Print the warning, and (still in the child) return to whoever called
start_mdmon()?  Not a good idea.

If we want an error when mdmon fails (might be a good idea) we should print
it after the 'wait' returns an error, not in the child.

Thanks,
NeilBrown

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

Re: [PATCH 1/3] Add support for launching mdmon via systemctl instead of fork/exec

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Mon, 21 Jan 2013 14:22:56 +0100 Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> 
>> To launch mdmon via systemctl, a new command line argument is added to
>> mdadm '--systemctl'. Alternatively it is possible to set the
>> environment variable MDMON_SYSTEMCTL.
>> 
>> This allows for having mdmon launched via systemctl which avoids
>> problems with it getting killed by systemd due to it ending up in the
>> parent's cgroup (udev).
>> 
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Thanks Jes.  This is certainly something that we want in some form.
> Some issues:
>
>  - I have come to the conclusion that --offroot is a bad idea.  We should
>    just make that the default.  No matter whether the array is providing the
>    root filesystem or not, I never want systemd (or anything else) to kill
>    mdmon.  I want it to remain in control.  So the systemctl handling should
>    assume offroot. e.g. there should only be one .service file.

Ok I guess the question is what happens if an array is shut down in
userland, does it take mdmon down manually once it is finished with it?
We need this to happen, because otherwise we end up with a dangling
mdmon process once we reboot, if the IMSM array wasn't assembled in the
initrd.

>  - on my machine (openSUSE), systemctl is in /bin, not /usr/bin.
>    Would I be correct in thinking that on your machine, /bin is a symlink
>    to /usr/bin?  If so, maybe we can just use "/bin/systemctl"?  If not,
>    we might need to try a few different options.
>    Similarly I have mdmon in /sbin, not /usr/sbin.  We we cannot all
>    use /sbin, we might need to 'sed' the .service file on installation to
>    match the current system.
>    (I note that you didn't get Makefile to install the .services files.  I
>    think we want it to - at least optionally)

This was done due to the big ->/usr move, I was assuming SuSE was going
the same way? This move isn't actually something which I think was a
great idea, but it's what has been forced upon us, so I tried to stick
to it.

I didn't want to force install the .service files but if you find it's a
good idea, I am not against that.

>  - I want the default behaviour to "do the right thing" in most case, but it
>    should be possible  to over-rid (by command line option or env var or
>    both).
>    So I think I would like it to try systemctl and if that fails for some
>    reason (either because the binary doesn't exist, or because it finds that
>    the .services file doesn't exist), then it should try to exec mdmon
>    directly.
>    This default could be over-ridden to always run mdmon directly.

This should be quite easy to do. I actually made it fail explicitly so
the user knows it fails when they try to run it via systemctl. However
if we take this approach, maybe we should also default to always trying
via systemctl before exec().

Cheers,
Jes

Re: [PATCH 1/3] Add support for launching mdmon via systemctl instead of fork/exec

[Jes Sorensen]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> NeilBrown <neilb@suse.de> writes:
>>  - I have come to the conclusion that --offroot is a bad idea.  We should
>>    just make that the default.  No matter whether the array is providing the
>>    root filesystem or not, I never want systemd (or anything else) to kill
>>    mdmon.  I want it to remain in control.  So the systemctl handling should
>>    assume offroot. e.g. there should only be one .service file.
>
> Ok I guess the question is what happens if an array is shut down in
> userland, does it take mdmon down manually once it is finished with it?
> We need this to happen, because otherwise we end up with a dangling
> mdmon process once we reboot, if the IMSM array wasn't assembled in the
> initrd.

I have been digging some more into this. Basically it looks like we can
nuke the --offroot argument and simply default to running that way.

On the bad side, we need two different .service files, one for on the
initrd and one for post system boot, due to different condition
requiremnts. The question is whether to name them the same, and just
have dracut know about the magic initrd version or if we want to try for
the two versions.

Having dracut install a special initrd version under the same name seems
the simplest from an mdadm code perspective though.

Thoughts?

Jes

Re: [PATCH 1/3] Add support for launching mdmon via systemctl instead of fork/exec

[Jes Sorensen]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>> NeilBrown <neilb@suse.de> writes:
>>>  - I have come to the conclusion that --offroot is a bad idea.  We should
>>>    just make that the default.  No matter whether the array is providing the
>>>    root filesystem or not, I never want systemd (or anything else) to kill
>>>    mdmon.  I want it to remain in control.  So the systemctl handling should
>>>    assume offroot. e.g. there should only be one .service file.
>>
>> Ok I guess the question is what happens if an array is shut down in
>> userland, does it take mdmon down manually once it is finished with it?
>> We need this to happen, because otherwise we end up with a dangling
>> mdmon process once we reboot, if the IMSM array wasn't assembled in the
>> initrd.
>
> I have been digging some more into this. Basically it looks like we can
> nuke the --offroot argument and simply default to running that way.
>
> On the bad side, we need two different .service files, one for on the
> initrd and one for post system boot, due to different condition
> requiremnts. The question is whether to name them the same, and just
> have dracut know about the magic initrd version or if we want to try for
> the two versions.

Very odd - more testing and it looks like this may not be a problem
after all, but instead because something went wrong during the creation
of the initrd in my testing. It does make me a little concerned if this
can happen to users who are installing an update, but with nothing more
than a single incident there isn't much I can do.

I will look at a set of patches.

Jes