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

[NeilBrown <neilb@suse.de>]

[-- 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 --]