[PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[Jes.Sorensen]

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

Hi,

Here is v2 of the systemd/dracut fixes for preboot handling in
mdadm/mdmon. I have changed the argument over from --initrd to
--preboot as Neil suggested, as well as fixed up the minor code nits
and added documentation to the man pages.

It shows up that I didn't need to add a separate enum for the command
line arguments in mdmon since mdadm.h gets pulled in so we can reuse
those. If we find that to be an issue, we can always create a separate
one later.

Cheers,
Jes

Jes Sorensen (4):
  mdmon: Use getopt_long() to parse command line options
  Add --preboot argument to mdadm
  Add --preboot argument to mdmon
  Spawn mdmon with --preboot if mdadm was launched with --preboot

 ReadMe.c   |    5 +++++
 mdadm.8.in |   12 ++++++++++++
 mdadm.c    |   10 ++++++++++
 mdadm.h    |    3 +++
 mdmon.8    |   12 +++++++++++-
 mdmon.c    |   60 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
 util.c     |   17 +++++++++++++----
 7 files changed, 103 insertions(+), 16 deletions(-)

-- 
1.7.8.3

[PATCH 1/4] mdmon: Use getopt_long() to parse command line options

[Jes.Sorensen]

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

This changes mdmon over to use getopt_long() for option parsing,
making it easier to add new options. In addition this patch introduces
a short version -t for --takeover and adds -h/--help.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 mdmon.c |   51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index b6ae0e6..a65c4a4 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -265,7 +265,14 @@ static int do_fork(void)
 
 void usage(void)
 {
-	fprintf(stderr, "Usage: mdmon [--all] [--takeover] CONTAINER\n");
+	fprintf(stderr,
+"Usage: mdmon [options] CONTAINER\n"
+"\n"
+"Options are:\n"
+"  --help        -h   : This message\n"
+"  --all              : All devices\n"
+"  --takeover    -t   : Takeover container\n"
+);
 	exit(2);
 }
 
@@ -277,25 +284,47 @@ int main(int argc, char *argv[])
 	int devnum;
 	char *devname;
 	int status = 0;
-	int arg;
+	int opt;
 	int all = 0;
 	int takeover = 0;
-
-	for (arg = 1; arg < argc; arg++) {
-		if (strncmp(argv[arg], "--all",5) == 0 ||
-		    strcmp(argv[arg], "/proc/mdstat") == 0) {
-			container_name = argv[arg];
+	static struct option options[] = {
+		{"all", 0, NULL, 'a'},
+		{"takeover", 0, NULL, 't'},
+		{"help", 0, NULL, 'h'},
+		{NULL, 0, NULL, 0}
+	};
+
+	while ((opt = getopt_long(argc, argv, "th", options, NULL)) != -1) {
+		switch (opt) {
+		case 'a':
+			container_name = argv[optind-1];
 			all = 1;
-		} else if (strcmp(argv[arg], "--takeover") == 0)
+			break;
+		case 't':
+			container_name = optarg;
 			takeover = 1;
-		else if (container_name == NULL)
-			container_name = argv[arg];
-		else
+			break;
+		case 'h':
+		default:
 			usage();
+			break;
+		}
 	}
+
+	if (all == 0 && container_name == NULL) {
+		if (argv[optind])
+			container_name = argv[optind];
+	}
+
 	if (container_name == NULL)
 		usage();
 
+	if (argc - optind > 1)
+		usage();
+
+	if (strcmp(container_name, "/proc/mdstat") == 0)
+		all = 1;
+
 	if (all) {
 		struct mdstat_ent *mdstat, *e;
 		int container_len = strlen(container_name);
-- 
1.7.8.3

[PATCH 2/4] Add --preboot argument to mdadm

[Jes.Sorensen]

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

When --preboot is parsed, mdadm will change the first character of
argv[0] to '@'. This is used to signal to systemd that mdadm was
launched from initramfs and should not be shut down before returning
to the initramfs.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 ReadMe.c   |    5 +++++
 mdadm.8.in |   12 ++++++++++++
 mdadm.c    |    9 +++++++++
 mdadm.h    |    1 +
 4 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/ReadMe.c b/ReadMe.c
index 9aa798b..10d6d44 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -111,6 +111,7 @@ struct option long_options[] = {
     {"kill-subarray", 1, 0, KillSubarray},
     {"update-subarray", 1, 0, UpdateSubarray},
     {"udev-rules", 2, 0, UdevRules},
+    {"preboot", 0, 0, PreBootOpt},
 
     /* synonyms */
     {"monitor",   0, 0, 'F'},
@@ -268,6 +269,10 @@ char OptionHelp[] =
 "  --query       -Q   : Display general information about how a\n"
 "                       device relates to the md driver\n"
 "  --auto-detect      : Start arrays auto-detected by the kernel\n"
+"  --preboot          : Set first character of argv[0] to @ to indicate the\n"
+"                       application was launched from initrd/initramfs and\n"
+"                       should not be shutdown by systemd as part of the\n"
+"                       regular shutdown process.\n"
 ;
 /*
 "\n"
diff --git a/mdadm.8.in b/mdadm.8.in
index 27be110..cb92ed6 100644
--- a/mdadm.8.in
+++ b/mdadm.8.in
@@ -255,6 +255,18 @@ Avoid printing purely informative messages.  With this,
 will be silent unless there is something really important to report.
 
 .TP
+.BR \-\-preboot
+Set first character of argv[0] to @ to indicate mdadm was launched
+from initrd/initramfs and should not be shutdown by systemd as part of
+the regular shutdown process. This option is normally only used by
+the system's initscripts. Please see here for more details on how
+systemd handled argv[0]:
+.IP
+.B http://www.freedesktop.org/wiki/Software/systemd/RootStorageDaemons
+.PP
+
+
+.TP
 .BR \-f ", " \-\-force
 Be more forceful about certain operations.  See the various modes for
 the exact meaning of this option in different contexts.
diff --git a/mdadm.c b/mdadm.c
index f07fac2..6c38064 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -174,6 +174,15 @@ int main(int argc, char *argv[])
 				homehost = optarg;
 			continue;
 
+		/*
+		 * --preboot sets first char of argv[0] to @. This is used
+		 * by systemd to signal that the tast was launched from
+		 * initrd/initramfs and should be preserved during shutdown
+		 */
+		case PreBootOpt:
+			argv[0][0] = '@';
+			continue;
+
 		case ':':
 		case '?':
 			fputs(Usage, stderr);
diff --git a/mdadm.h b/mdadm.h
index 381ef86..10b420f 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -321,6 +321,7 @@ enum special_options {
 	UdevRules,
 	FreezeReshape,
 	Continue,
+	PreBootOpt,
 };
 
 /* structures read from config file */
-- 
1.7.8.3

[PATCH 3/4] Add --preboot argument to mdmon

[Jes.Sorensen]

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

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 mdmon.8 |   12 +++++++++++-
 mdmon.c |    9 +++++++++
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/mdmon.8 b/mdmon.8
index 8c1ce5f..cceeaad 100644
--- a/mdmon.8
+++ b/mdmon.8
@@ -5,7 +5,7 @@ mdmon \- monitor MD external metadata arrays
 
 .SH SYNOPSIS
 
-.BI mdmon " [--all] [--takeover] CONTAINER"
+.BI mdmon " [--all] [--takeover] [--preboot] CONTAINER"
 
 .SH OVERVIEW
 The 2.6.27 kernel brings the ability to support external metadata arrays.
@@ -165,6 +165,16 @@ argument is over-written with the name of the container.  To allow for
 containers with names longer than 5 characters, this argument can be
 arbitrarily extended, e.g. to
 .BR \-\-all-active-arrays .
+.TP
+.BR \-\-preboot
+Set first character of argv[0] to @ to indicate mdmon was launched
+from initrd/initramfs and should not be shutdown by systemd as part of
+the regular shutdown process. This option is normally only used by
+the system's initscripts. Please see here for more details on how
+systemd handled argv[0]:
+.IP
+.B http://www.freedesktop.org/wiki/Software/systemd/RootStorageDaemons
+.PP
 
 .PP
 Note that
diff --git a/mdmon.c b/mdmon.c
index a65c4a4..fa653df 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -272,6 +272,10 @@ void usage(void)
 "  --help        -h   : This message\n"
 "  --all              : All devices\n"
 "  --takeover    -t   : Takeover container\n"
+"  --preboot          : Set first character of argv[0] to @ to indicate the\n"
+"                       application was launched from initrd/initramfs and\n"
+"                       should not be shutdown by systemd as part of the\n"
+"                       regular shutdown process.\n"
 );
 	exit(2);
 }
@@ -291,6 +295,7 @@ int main(int argc, char *argv[])
 		{"all", 0, NULL, 'a'},
 		{"takeover", 0, NULL, 't'},
 		{"help", 0, NULL, 'h'},
+		{"preboot", 0, NULL, PreBootOpt},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -304,6 +309,10 @@ int main(int argc, char *argv[])
 			container_name = optarg;
 			takeover = 1;
 			break;
+		case PreBootOpt:
+			argv[0][0] = '@';
+			continue;
+			
 		case 'h':
 		default:
 			usage();
-- 
1.7.8.3

[PATCH 4/4] Spawn mdmon with --preboot if mdadm was launched with --preboot

[Jes.Sorensen]

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

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 mdadm.c |    1 +
 mdadm.h |    2 ++
 util.c  |   17 +++++++++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/mdadm.c b/mdadm.c
index 6c38064..e401bb3 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -181,6 +181,7 @@ int main(int argc, char *argv[])
 		 */
 		case PreBootOpt:
 			argv[0][0] = '@';
+			__preboot = 1;
 			continue;
 
 		case ':':
diff --git a/mdadm.h b/mdadm.h
index 10b420f..0eb820c 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1383,3 +1383,5 @@ static inline int xasprintf(char **strp, const char *fmt, ...) {
 
 #define PROCESS_DELAYED -2
 #define PROCESS_PENDING -3
+
+extern int __preboot;
diff --git a/util.c b/util.c
index 6985a70..e31930e 100644
--- a/util.c
+++ b/util.c
@@ -32,6 +32,8 @@
 #include	<dirent.h>
 #include	<signal.h>
 
+int __preboot;
+
 /*
  * following taken from linux/blkpg.h because they aren't
  * anywhere else and it isn't safe to #include linux/ * stuff.
@@ -1622,10 +1624,17 @@ int start_mdmon(int devnum)
 				skipped = 0;
 
 		for (i=0; paths[i]; i++)
-			if (paths[i][0])
-				execl(paths[i], "mdmon",
-				      devnum2devname(devnum),
-				      NULL);
+			if (paths[i][0]) {
+				if (__preboot) {
+					execl(paths[i], "mdmon", "--preboot",
+					      devnum2devname(devnum),
+					      NULL);
+				} else {
+					execl(paths[i], "mdmon",
+					      devnum2devname(devnum),
+					      NULL);
+				}
+			}
 		exit(1);
 	case -1: fprintf(stderr, Name ": cannot run mdmon. "
 			 "Array remains readonly\n");
-- 
1.7.8.3

Re: [PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[Doug Ledford]

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

On 01/23/2012 06:18 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> Here is v2 of the systemd/dracut fixes for preboot handling in
> mdadm/mdmon. I have changed the argument over from --initrd to
> --preboot as Neil suggested, as well as fixed up the minor code nits
> and added documentation to the man pages.
> 
> It shows up that I didn't need to add a separate enum for the command
> line arguments in mdmon since mdadm.h gets pulled in so we can reuse
> those. If we find that to be an issue, we can always create a separate
> one later.

I'm not sure I like --preboot.  I understand the difficulty in finding
just the right option name, but to me, preboot sounds like an option you
would use if you are doing pre-processing of something for a later
invokation to use.  And that isn't the case here, so the gut level
understanding won't match up with the man page/help output understanding
and I try to avoid that as much as possible.  Since the distinct element
is that we want to keep running both before and after the normal root
filesystem is mounted and we want all files we open to be off of the
root filesystem namespace, maybe --offroot (which is odd enough by
itself to forestall any gut reactions and prompt a trip to the help/man
pages as opposed to guessing what the option means)?

> Cheers,
> Jes
> 
> Jes Sorensen (4):
>   mdmon: Use getopt_long() to parse command line options
>   Add --preboot argument to mdadm
>   Add --preboot argument to mdmon
>   Spawn mdmon with --preboot if mdadm was launched with --preboot
> 
>  ReadMe.c   |    5 +++++
>  mdadm.8.in |   12 ++++++++++++
>  mdadm.c    |   10 ++++++++++
>  mdadm.h    |    3 +++
>  mdmon.8    |   12 +++++++++++-
>  mdmon.c    |   60 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  util.c     |   17 +++++++++++++----
>  7 files changed, 103 insertions(+), 16 deletions(-)
> 


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[Jes Sorensen]

On 01/23/12 17:24, Doug Ledford wrote:
> On 01/23/2012 06:18 AM, Jes.Sorensen@redhat.com wrote:
>> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> > 
>> > Hi,
>> > 
>> > Here is v2 of the systemd/dracut fixes for preboot handling in
>> > mdadm/mdmon. I have changed the argument over from --initrd to
>> > --preboot as Neil suggested, as well as fixed up the minor code nits
>> > and added documentation to the man pages.
>> > 
>> > It shows up that I didn't need to add a separate enum for the command
>> > line arguments in mdmon since mdadm.h gets pulled in so we can reuse
>> > those. If we find that to be an issue, we can always create a separate
>> > one later.
> I'm not sure I like --preboot.  I understand the difficulty in finding
> just the right option name, but to me, preboot sounds like an option you
> would use if you are doing pre-processing of something for a later
> invokation to use.  And that isn't the case here, so the gut level
> understanding won't match up with the man page/help output understanding
> and I try to avoid that as much as possible.  Since the distinct element
> is that we want to keep running both before and after the normal root
> filesystem is mounted and we want all files we open to be off of the
> root filesystem namespace, maybe --offroot (which is odd enough by
> itself to forestall any gut reactions and prompt a trip to the help/man
> pages as opposed to guessing what the option means)?
> 

Kay suggested --preroot on irc - would that do the job? I think that is
a bit better than --offroot.

Cheers,
Jes

Re: [PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[Doug Ledford]

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

On 01/23/2012 11:39 AM, Jes Sorensen wrote:
> On 01/23/12 17:24, Doug Ledford wrote:
>> On 01/23/2012 06:18 AM, Jes.Sorensen@redhat.com wrote:
>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>
>>>> Hi,
>>>>
>>>> Here is v2 of the systemd/dracut fixes for preboot handling in
>>>> mdadm/mdmon. I have changed the argument over from --initrd to
>>>> --preboot as Neil suggested, as well as fixed up the minor code nits
>>>> and added documentation to the man pages.
>>>>
>>>> It shows up that I didn't need to add a separate enum for the command
>>>> line arguments in mdmon since mdadm.h gets pulled in so we can reuse
>>>> those. If we find that to be an issue, we can always create a separate
>>>> one later.
>> I'm not sure I like --preboot.  I understand the difficulty in finding
>> just the right option name, but to me, preboot sounds like an option you
>> would use if you are doing pre-processing of something for a later
>> invokation to use.  And that isn't the case here, so the gut level
>> understanding won't match up with the man page/help output understanding
>> and I try to avoid that as much as possible.  Since the distinct element
>> is that we want to keep running both before and after the normal root
>> filesystem is mounted and we want all files we open to be off of the
>> root filesystem namespace, maybe --offroot (which is odd enough by
>> itself to forestall any gut reactions and prompt a trip to the help/man
>> pages as opposed to guessing what the option means)?
>>
> 
> Kay suggested --preroot on irc - would that do the job? I think that is
> a bit better than --offroot.

I'd say preroot is better than preboot for sure.  I think I still like
offroot more than preroot though.  We *are* running off root (from the
initramfs, and we are modifying our argv[0] to signal to systemd that we
are doing exactly this).  We *are* keeping our files off of root (in
/run/mdadm versus /var/run/mdadm).  And we are intending to live both
before root (preroot) and also after root (postroot).  So, I find
preroot to be better than preboot, but still not as accurate as offroot.
 Offroot fairly accurately captures what we are doing (although it
doesn't speak to the expected life span of the action, nor does it speak
to the fact that we stay off root only if we are invoked from what will
become the non-root fs, so some sort of check to make sure we are
actually in an initrd/initramfs of sorts might be nice, but I'm not sure
how that would play with things like USB live images where you might
actually want to use the option on a non-initrd environment), where as
both preboot and preroot speak as to when it is used, but not what it
does nor the expected life span of the action.


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[John Robinson]

On 23/01/2012 16:52, Doug Ledford wrote:
> On 01/23/2012 11:39 AM, Jes Sorensen wrote:
[...]
>> Kay suggested --preroot on irc - would that do the job? I think that is
>> a bit better than --offroot.
>
> I'd say preroot is better than preboot for sure.  I think I still like
> offroot more than preroot though.

For what it's worth, as a moderately experienced user rather than an 
expert like you guys, I'd vote for this - I nearly suggested something 
like this earlier in the thread, but didn't because I thought I must 
have misunderstood, but Doug's explanation has just confirmed that I had 
it right in the first place - the mdmon invoked with --this-new-option 
is started from the initrd, in advance of the real root filesystem being 
mounted, but hangs around until after the real root filesystem has gone 
again, so that for example --postroot would make just as much sense as 
--preroot, and neither is as good as --offroot.

Cheers,

John.

Re: [PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[Jes Sorensen]

On 01/23/12 17:52, Doug Ledford wrote:
>> Kay suggested --preroot on irc - would that do the job? I think that is
>> > a bit better than --offroot.
> I'd say preroot is better than preboot for sure.  I think I still like
> offroot more than preroot though.  We *are* running off root (from the
> initramfs, and we are modifying our argv[0] to signal to systemd that we
> are doing exactly this).  We *are* keeping our files off of root (in
> /run/mdadm versus /var/run/mdadm).  And we are intending to live both
> before root (preroot) and also after root (postroot).  So, I find
> preroot to be better than preboot, but still not as accurate as offroot.
>  Offroot fairly accurately captures what we are doing (although it
> doesn't speak to the expected life span of the action, nor does it speak
> to the fact that we stay off root only if we are invoked from what will
> become the non-root fs, so some sort of check to make sure we are
> actually in an initrd/initramfs of sorts might be nice, but I'm not sure
> how that would play with things like USB live images where you might
> actually want to use the option on a non-initrd environment), where as
> both preboot and preroot speak as to when it is used, but not what it
> does nor the expected life span of the action.

Ok, I guess it depends a bit on how one thinks of it, ie. whether
thinking in the context of how it was launched or the system state. I am
not really biased one way or another, I mostly care about the
functionality, so I am going to wait until tomorrow and see what other
input we get and what Neil has to say.

Once we have a consensus, I'll cook up a v3 of the patches to match that.

Cheers,
Jes

Re: [PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[Krzysztof Adamski]

How about --prepivot

K

On Mon, 2012-01-23 at 11:24 -0500, Doug Ledford wrote:
> On 01/23/2012 06:18 AM, Jes.Sorensen@redhat.com wrote:
> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
> > 
> > Hi,
> > 
> > Here is v2 of the systemd/dracut fixes for preboot handling in
> > mdadm/mdmon. I have changed the argument over from --initrd to
> > --preboot as Neil suggested, as well as fixed up the minor code nits
> > and added documentation to the man pages.
> > 
> > It shows up that I didn't need to add a separate enum for the command
> > line arguments in mdmon since mdadm.h gets pulled in so we can reuse
> > those. If we find that to be an issue, we can always create a separate
> > one later.
> 
> I'm not sure I like --preboot.  I understand the difficulty in finding
> just the right option name, but to me, preboot sounds like an option you
> would use if you are doing pre-processing of something for a later
> invokation to use.  And that isn't the case here, so the gut level
> understanding won't match up with the man page/help output understanding
> and I try to avoid that as much as possible.  Since the distinct element
> is that we want to keep running both before and after the normal root
> filesystem is mounted and we want all files we open to be off of the
> root filesystem namespace, maybe --offroot (which is odd enough by
> itself to forestall any gut reactions and prompt a trip to the help/man
> pages as opposed to guessing what the option means)?
> 
> > Cheers,
> > Jes
> > 
> > Jes Sorensen (4):
> >   mdmon: Use getopt_long() to parse command line options
> >   Add --preboot argument to mdadm
> >   Add --preboot argument to mdmon
> >   Spawn mdmon with --preboot if mdadm was launched with --preboot
> > 
> >  ReadMe.c   |    5 +++++
> >  mdadm.8.in |   12 ++++++++++++
> >  mdadm.c    |   10 ++++++++++
> >  mdadm.h    |    3 +++
> >  mdmon.8    |   12 +++++++++++-
> >  mdmon.c    |   60 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
> >  util.c     |   17 +++++++++++++----
> >  7 files changed, 103 insertions(+), 16 deletions(-)
> > 
> 
> 

Re: [PATCH v2 0/4] add --preboot argument to mdadm/mdmon

[Lennart Poettering]

Heya,

On 23.01.2012 18:04, Krzysztof Adamski wrote:
> How about --prepivot

Note that pivot_root() is not involved in your boot process anymore, so 
this might be considered misleading.

Lennart