On Mon, 21 Jan 2013 14:22:56 +0100 Jes.Sorensen@xxxxxxxxxx wrote:
> From: Jes Sorensen <Jes.Sorensen@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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:
signature.asc
Description: PGP signature
