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

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

[Jes.Sorensen]

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

Hi,

In order to resolve the problem with reboots hanging on systems with /
on an IMSM RAID, systemd needs to be able to see that a given mdadm
and mdmon process was launched from the initramfs, and allow it to
survive until it gets back to there, so it can unmount the root
filesystem safely.

These patches introduces the --initrd argument to mdadm which makes it
change the first character of argv[0] to '@' to match the convention
set by systemd.

In addition I also changed mdmon to use getopt_long to make it easier
to add more command line arguments to it. Note that I went through
great effort to preserve the behavior, even though the old code did
some somewhat dodgy stuff using changing the arvg buffer pointing to
the '--all' argument.

Comments?

Thanks,
Jes

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

 ReadMe.c |    3 ++
 mdadm.c  |   11 ++++++++++
 mdadm.h  |    3 ++
 mdmon.c  |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
 util.c   |   17 ++++++++++++---
 5 files changed, 83 insertions(+), 15 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 --initrd argument to mdadm

[Jes.Sorensen]

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

When --initrd 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 |    3 +++
 mdadm.c  |   10 ++++++++++
 mdadm.h  |    1 +
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/ReadMe.c b/ReadMe.c
index 9aa798b..49dc04f 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},
+    {"initrd", 0, 0, InitrdOpt},
 
     /* synonyms */
     {"monitor",   0, 0, 'F'},
@@ -268,6 +269,8 @@ 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"
+"  --initrd           : Set first character of argv[0] to @ to indicate the\n"
+"                       application was launched from initrd.\n"
 ;
 /*
 "\n"
diff --git a/mdadm.c b/mdadm.c
index f07fac2..c0ccf55 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -174,6 +174,16 @@ int main(int argc, char *argv[])
 				homehost = optarg;
 			continue;
 
+		/*
+		 * --initrd 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 InitrdOpt:
+			c = argv[0];
+			c[0] = '@';
+			continue;
+
 		case ':':
 		case '?':
 			fputs(Usage, stderr);
diff --git a/mdadm.h b/mdadm.h
index 381ef86..d1ac1e8 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -321,6 +321,7 @@ enum special_options {
 	UdevRules,
 	FreezeReshape,
 	Continue,
+	InitrdOpt,
 };
 
 /* structures read from config file */
-- 
1.7.8.3

RE: [PATCH 2/4] Add --initrd argument to mdadm

[Kwolek, Adam]

Hi,

I think that '--initrd'  option should have '--freeze-reshape' option functionality for mdadm also.
To do this e.g.  proper variable (freeze_reshape in mdadm.c) should be set, when '--initrd' option is passed in command line.

BR
Adam

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Jes.Sorensen@redhat.com
> Sent: Tuesday, January 17, 2012 11:49 AM
> To: neilb@suse.de
> Cc: linux-raid@vger.kernel.org; dledford@redhat.com; kay@redhat.com;
> harald@redhat.com; lpoetter@redhat.com; mschmidt@redhat.com
> Subject: [PATCH 2/4] Add --initrd argument to mdadm
> 
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> When --initrd 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 |    3 +++
>  mdadm.c  |   10 ++++++++++
>  mdadm.h  |    1 +
>  3 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/ReadMe.c b/ReadMe.c
> index 9aa798b..49dc04f 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},
> +    {"initrd", 0, 0, InitrdOpt},
> 
>      /* synonyms */
>      {"monitor",   0, 0, 'F'},
> @@ -268,6 +269,8 @@ 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"
> +"  --initrd           : Set first character of argv[0] to @ to indicate the\n"
> +"                       application was launched from initrd.\n"
>  ;
>  /*
>  "\n"
> diff --git a/mdadm.c b/mdadm.c
> index f07fac2..c0ccf55 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -174,6 +174,16 @@ int main(int argc, char *argv[])
>  				homehost = optarg;
>  			continue;
> 
> +		/*
> +		 * --initrd 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 InitrdOpt:
> +			c = argv[0];
> +			c[0] = '@';
> +			continue;
> +
>  		case ':':
>  		case '?':
>  			fputs(Usage, stderr);
> diff --git a/mdadm.h b/mdadm.h
> index 381ef86..d1ac1e8 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -321,6 +321,7 @@ enum special_options {
>  	UdevRules,
>  	FreezeReshape,
>  	Continue,
> +	InitrdOpt,
>  };
> 
>  /* structures read from config file */
> --
> 1.7.8.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Jes Sorensen]

On 01/18/12 09:13, Kwolek, Adam wrote:
> Hi,
> 
> I think that '--initrd'  option should have '--freeze-reshape' option functionality for mdadm also.
> To do this e.g.  proper variable (freeze_reshape in mdadm.c) should be set, when '--initrd' option is passed in command line.

Hi Adam,

I don't quite know what freeze_reshape does - from the name it sounds
like an option you want to set at runtime rather than when a raid is
assembled initially?

Are you proposing that we shouldn't allow reshape of an IMSM raid that
is used as the root device?

I am merely a bit confused here, so I am hoping you could clarify a bit.

Thanks,
Jes

RE: [PATCH 2/4] Add --initrd argument to mdadm

[Kwolek, Adam]

> -----Original Message-----
> From: Jes Sorensen [mailto:Jes.Sorensen@redhat.com]
> Sent: Wednesday, January 18, 2012 11:09 AM
> To: Kwolek, Adam
> Cc: neilb@suse.de; linux-raid@vger.kernel.org; dledford@redhat.com;
> kay@redhat.com; harald@redhat.com; lpoetter@redhat.com;
> mschmidt@redhat.com
> Subject: Re: [PATCH 2/4] Add --initrd argument to mdadm
> 
> On 01/18/12 09:13, Kwolek, Adam wrote:
> > Hi,
> >
> > I think that '--initrd'  option should have '--freeze-reshape' option
> functionality for mdadm also.
> > To do this e.g.  proper variable (freeze_reshape in mdadm.c) should be set,
> when '--initrd' option is passed in command line.
> 
> Hi Adam,
> 
> I don't quite know what freeze_reshape does - from the name it sounds like
> an option you want to set at runtime rather than when a raid is assembled
> initially?

Yes, it is intended to be specified for assembly during initrd phase. It "tells" to 'grow continue' to restore array critical section only.
It stops reshape at this moment to keep process safe during file system pivot when mdadm loosing fs context. 
Reshape should be continued later by invoking mdadm grow with '--continue' option.

> 
> Are you proposing that we shouldn't allow reshape of an IMSM raid that is
> used as the root device?

We should tell to mdadm for boot device assembly to be prepared for fs pivot when reshape is in progress. We can do this by specifying 2 options in command line,
but in real world "someone will forget about something", so I think '--iinitrd' can have '--freeze-reshape' functionality also /without freeze-reshape option removal for separate use when raid will not be a boot device/.
As I wrote before it needs to set one variable during command line parsing in mdadm.c only.


> 
> I am merely a bit confused here, so I am hoping you could clarify a bit.
> 
> Thanks,
> Jes

Let me know if you need more clarification, or let me know if in your opinion I'm wrong.

BR
Adam

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Jes Sorensen]

On 01/18/12 11:36, Kwolek, Adam wrote:
> 
> 
>> -----Original Message-----
>> From: Jes Sorensen [mailto:Jes.Sorensen@redhat.com]
>> Sent: Wednesday, January 18, 2012 11:09 AM
>> To: Kwolek, Adam
>> Cc: neilb@suse.de; linux-raid@vger.kernel.org; dledford@redhat.com;
>> kay@redhat.com; harald@redhat.com; lpoetter@redhat.com;
>> mschmidt@redhat.com
>> Subject: Re: [PATCH 2/4] Add --initrd argument to mdadm
>>
>> On 01/18/12 09:13, Kwolek, Adam wrote:
>>> Hi,
>>>
>>> I think that '--initrd'  option should have '--freeze-reshape' option
>> functionality for mdadm also.
>>> To do this e.g.  proper variable (freeze_reshape in mdadm.c) should be set,
>> when '--initrd' option is passed in command line.
>>
>> Hi Adam,
>>
>> I don't quite know what freeze_reshape does - from the name it sounds like
>> an option you want to set at runtime rather than when a raid is assembled
>> initially?
> 
> Yes, it is intended to be specified for assembly during initrd
> phase. It "tells" to 'grow continue' to restore array critical
> section only. It stops reshape at this moment to keep process
> safe during file system pivot when mdadm loosing fs context.
> Reshape should be continued later by invoking mdadm grow with
> '--continue' option.

Is there already a --freeze-reshape option implemented? I didn't see any
in the man page or the help output on my system here.

>> Are you proposing that we shouldn't allow reshape of an IMSM raid that is
>> used as the root device?
> 
> We should tell to mdadm for boot device assembly to be prepared for
> fs pivot when reshape is in progress. We can do this by specifying 2
> options in command line, but in real world "someone will forget about
> something", so I think '--iinitrd' can have '--freeze-reshape'
> functionality also /without freeze-reshape option removal for
> separate use when raid will not be a boot device/. As I wrote before
> it needs to set one variable during command line parsing in mdadm.c
> only.

If there is already a --freeze-reshape option, then I think it is better
to have the boot scripts set that explicitly. The initramfs stage isn't
really something we expect users or admins to mess with manually, so it
ought to work for that. However if there is no option for this, and I
just need to set the variable when detecting --initrd, then I can add
that to the patchset no problem.

Cheers,
Jes

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Jes Sorensen]

On 01/18/12 17:43, Jes Sorensen wrote:
>> We should tell to mdadm for boot device assembly to be prepared for
>> > fs pivot when reshape is in progress. We can do this by specifying 2
>> > options in command line, but in real world "someone will forget about
>> > something", so I think '--iinitrd' can have '--freeze-reshape'
>> > functionality also /without freeze-reshape option removal for
>> > separate use when raid will not be a boot device/. As I wrote before
>> > it needs to set one variable during command line parsing in mdadm.c
>> > only.
> If there is already a --freeze-reshape option, then I think it is better
> to have the boot scripts set that explicitly. The initramfs stage isn't
> really something we expect users or admins to mess with manually, so it
> ought to work for that. However if there is no option for this, and I
> just need to set the variable when detecting --initrd, then I can add
> that to the patchset no problem.

I guess I should add here, that I am not strongly opposed to setting
freeze_reshape for initrd. I am just wary of setting multiple flags
magically without it being obvious to the user/admin.

Cheers,
Jes

RE: [PATCH 2/4] Add --initrd argument to mdadm

[Kwolek, Adam]

> -----Original Message-----
> From: Jes Sorensen [mailto:Jes.Sorensen@redhat.com]
> Sent: Wednesday, January 18, 2012 5:47 PM
> To: Kwolek, Adam
> Cc: neilb@suse.de; linux-raid@vger.kernel.org; dledford@redhat.com;
> kay@redhat.com; harald@redhat.com; lpoetter@redhat.com;
> mschmidt@redhat.com
> Subject: Re: [PATCH 2/4] Add --initrd argument to mdadm
> 
> On 01/18/12 17:43, Jes Sorensen wrote:
> >> We should tell to mdadm for boot device assembly to be prepared for
> >> > fs pivot when reshape is in progress. We can do this by specifying
> >> > 2 options in command line, but in real world "someone will forget
> >> > about something", so I think '--iinitrd' can have '--freeze-reshape'
> >> > functionality also /without freeze-reshape option removal for
> >> > separate use when raid will not be a boot device/. As I wrote
> >> > before it needs to set one variable during command line parsing in
> >> > mdadm.c only.
> > If there is already a --freeze-reshape option, then I think it is
> > better to have the boot scripts set that explicitly. The initramfs
> > stage isn't really something we expect users or admins to mess with
> > manually, so it ought to work for that. However if there is no option
> > for this, and I just need to set the variable when detecting --initrd,
> > then I can add that to the patchset no problem.
> 
> I guess I should add here, that I am not strongly opposed to setting
> freeze_reshape for initrd. I am just wary of setting multiple flags magically
> without it being obvious to the user/admin.
> 
> Cheers,
> Jes

Up to you.

BR
Adam

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Jes Sorensen]

On 01/19/12 08:25, Kwolek, Adam wrote:
>>> If there is already a --freeze-reshape option, then I think it is
>>> > > better to have the boot scripts set that explicitly. The initramfs
>>> > > stage isn't really something we expect users or admins to mess with
>>> > > manually, so it ought to work for that. However if there is no option
>>> > > for this, and I just need to set the variable when detecting --initrd,
>>> > > then I can add that to the patchset no problem.
>> > 
>> > I guess I should add here, that I am not strongly opposed to setting
>> > freeze_reshape for initrd. I am just wary of setting multiple flags magically
>> > without it being obvious to the user/admin.
>> > 
>> > Cheers,
>> > Jes
> Up to you.

Ok, I will leave it up to Neil/Kay/Harald - what do you guys think?
Should we make --initrd set the freeze_reshape flag, or better to expect
the initramfs scripts to specify it directly?

Cheers,
Jes

Re: [PATCH 2/4] Add --initrd argument to mdadm

[NeilBrown]

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

On Fri, 20 Jan 2012 15:05:30 +0100 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:

> On 01/19/12 08:25, Kwolek, Adam wrote:
> >>> If there is already a --freeze-reshape option, then I think it is
> >>> > > better to have the boot scripts set that explicitly. The initramfs
> >>> > > stage isn't really something we expect users or admins to mess with
> >>> > > manually, so it ought to work for that. However if there is no option
> >>> > > for this, and I just need to set the variable when detecting --initrd,
> >>> > > then I can add that to the patchset no problem.
> >> > 
> >> > I guess I should add here, that I am not strongly opposed to setting
> >> > freeze_reshape for initrd. I am just wary of setting multiple flags magically
> >> > without it being obvious to the user/admin.
> >> > 
> >> > Cheers,
> >> > Jes
> > Up to you.
> 
> Ok, I will leave it up to Neil/Kay/Harald - what do you guys think?
> Should we make --initrd set the freeze_reshape flag, or better to expect
> the initramfs scripts to specify it directly?
>

I would like to keep these two options separate.
It might be worth noting in the man page that --freeze-reshape would normally
be used with --whatever-we-call-the-new-flag, but I don't think they should
be the same option.

thanks,
NeilBrown

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

Re: [PATCH 2/4] Add --initrd argument to mdadm

[NeilBrown]

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

On Tue, 17 Jan 2012 11:48:48 +0100 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> When --initrd 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 |    3 +++
>  mdadm.c  |   10 ++++++++++
>  mdadm.h  |    1 +
>  3 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/ReadMe.c b/ReadMe.c
> index 9aa798b..49dc04f 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},
> +    {"initrd", 0, 0, InitrdOpt},
>  
>      /* synonyms */
>      {"monitor",   0, 0, 'F'},
> @@ -268,6 +269,8 @@ 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"
> +"  --initrd           : Set first character of argv[0] to @ to indicate the\n"
> +"                       application was launched from initrd.\n"
>  ;
>  /*
>  "\n"
> diff --git a/mdadm.c b/mdadm.c
> index f07fac2..c0ccf55 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -174,6 +174,16 @@ int main(int argc, char *argv[])
>  				homehost = optarg;
>  			continue;
>  
> +		/*
> +		 * --initrd 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 InitrdOpt:
> +			c = argv[0];
> +			c[0] = '@';
> +			continue;

Why not:
                        argv[0][0] = '@';
??

Also I'm wondering about the choice of "initrd" as the option name.
After all, we mostly use initramfs these days.

 --preboot ??
 --systemd-root-storage-daemon ??

OK, the second is a joke, and the first is introducing terminology that isn't
widely used...

 --for-root
??

I'm open to suggestions, but I'm not seeing --initrd as the best choice just
yet.

Otherwise this and 1/4 are OK.

Thanks,
NeilBrown

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

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Jes Sorensen]

On 01/22/12 12:17, NeilBrown wrote:
> On Tue, 17 Jan 2012 11:48:48 +0100 Jes.Sorensen@redhat.com wrote:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>  
>> +		/*
>> +		 * --initrd 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 InitrdOpt:
>> +			c = argv[0];
>> +			c[0] = '@';
>> +			continue;
> 
> Why not:
>                         argv[0][0] = '@';
> ??

I always get confused by multi-indexed arrays, so I open-coded it. I
will change it.

> Also I'm wondering about the choice of "initrd" as the option name.
> After all, we mostly use initramfs these days.
> 
>  --preboot ??
>  --systemd-root-storage-daemon ??
> 
> OK, the second is a joke, and the first is introducing terminology that isn't
> widely used...

I was actually thinking --attacked-by-thunderbunnies-from-space but
users might find it a bit too long.....

>  --for-root
> ??
> 
> I'm open to suggestions, but I'm not seeing --initrd as the best choice just
> yet.

I was trying to come up with a better name, and I think --preboot is the
best I have seen so far. Other ideas I had were:
 --systemd-root
 --no-shutdown
however I like --preboot better, so I will go with that, unless someone
starts throwing things at me.

Cheers,
Jes

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Lennart Poettering]

Heya,

On 23.01.2012 11:25, Jes Sorensen wrote:

>> I'm open to suggestions, but I'm not seeing --initrd as the best choice just
>> yet.
>
> I was trying to come up with a better name, and I think --preboot is the
> best I have seen so far. Other ideas I had were:
>   --systemd-root
>   --no-shutdown
> however I like --preboot better, so I will go with that, unless someone
> starts throwing things at me.

I figure it's a bit of bike-shedding, but I'd like to ask you to stick 
to --initramfs or so if possible, simply because we're interested to 
reuse the same name for that switch for all storage daemons which need 
this, and the spec currently suggests --initramfs, and I'd prefer to 
avoid notifying all users of that change if we change it...

Lennart

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Kay Sievers]

On Mon, 2012-01-23 at 21:13 +0100, Lennart Poettering wrote:
> Heya,
> 
> On 23.01.2012 11:25, Jes Sorensen wrote:
> 
> >> I'm open to suggestions, but I'm not seeing --initrd as the best choice just
> >> yet.
> >
> > I was trying to come up with a better name, and I think --preboot is the
> > best I have seen so far. Other ideas I had were:
> >   --systemd-root
> >   --no-shutdown
> > however I like --preboot better, so I will go with that, unless someone
> > starts throwing things at me.
> 
> I figure it's a bit of bike-shedding, but I'd like to ask you to stick 
> to --initramfs or so if possible, simply because we're interested to 
> reuse the same name for that switch for all storage daemons which need 
> this, and the spec currently suggests --initramfs, and I'd prefer to 
> avoid notifying all users of that change if we change it...

Last time you and I decided to rename it to --initrd, so the document
does not say --initramfs.

Back to square one. :)

Kay

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Jes Sorensen]

On 01/23/12 21:50, Kay Sievers wrote:
> On Mon, 2012-01-23 at 21:13 +0100, Lennart Poettering wrote:
>> On 23.01.2012 11:25, Jes Sorensen wrote:
>>> I was trying to come up with a better name, and I think --preboot is the
>>> best I have seen so far. Other ideas I had were:
>>>   --systemd-root
>>>   --no-shutdown
>>> however I like --preboot better, so I will go with that, unless someone
>>> starts throwing things at me.
>>
>> I figure it's a bit of bike-shedding, but I'd like to ask you to stick 
>> to --initramfs or so if possible, simply because we're interested to 
>> reuse the same name for that switch for all storage daemons which need 
>> this, and the spec currently suggests --initramfs, and I'd prefer to 
>> avoid notifying all users of that change if we change it...
> 
> Last time you and I decided to rename it to --initrd, so the document
> does not say --initramfs.
> 
> Back to square one. :)

Neil,

In the light of the discussion so far, do you have any preference for
--preboot/--preroot/--initrd/--offroot

Cheers,
Jes

Re: [PATCH 2/4] Add --initrd argument to mdadm

[NeilBrown]

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

On Wed, 25 Jan 2012 11:53:53 +0100 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:

> On 01/23/12 21:50, Kay Sievers wrote:
> > On Mon, 2012-01-23 at 21:13 +0100, Lennart Poettering wrote:
> >> On 23.01.2012 11:25, Jes Sorensen wrote:
> >>> I was trying to come up with a better name, and I think --preboot is the
> >>> best I have seen so far. Other ideas I had were:
> >>>   --systemd-root
> >>>   --no-shutdown
> >>> however I like --preboot better, so I will go with that, unless someone
> >>> starts throwing things at me.
> >>
> >> I figure it's a bit of bike-shedding, but I'd like to ask you to stick 
> >> to --initramfs or so if possible, simply because we're interested to 
> >> reuse the same name for that switch for all storage daemons which need 
> >> this, and the spec currently suggests --initramfs, and I'd prefer to 
> >> avoid notifying all users of that change if we change it...
> > 
> > Last time you and I decided to rename it to --initrd, so the document
> > does not say --initramfs.
> > 
> > Back to square one. :)
> 
> Neil,
> 
> In the light of the discussion so far, do you have any preference for
> --preboot/--preroot/--initrd/--offroot
> 

How about
   -@

A single-character which reflects what it actually does :-)

I don't like --initrd because it is just wrong.

I can live with the others.

I think --offroot is closest to the right meaning (it is both pre and post).
I confess a little discomfort with the word "off" as it can mean both "not on"
and "was on".

"Did you run mdadm off root, or off /usr ??  Neither, I ran it from initramfs
which is off root."

But I agree with Doug that the term is sufficiently unusual that people are
unlikely to assume anything about it, so it should be safe.

I'm tempted to suggest something completely different that starts with --no-,
like
  --no-session-manage
but the word 'manage' already has a couple of meaning for mdadm.

So I'm leaning towards --offroot  though I'd go for --outside-root if it were
a bit shorter.

Maybe we need a name for this thing that exists before root is mounted and
remains until after root is unmounted.  If we had a name for that which
reflected its reality, that would work for me.
 --initial-root
isn't too bad but probably could be better.
 --outer-root
??


Yes, I know - you wanted a decision and I've just created more options !!

 --offroot

then.
(maybe)

NeilBrown

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

Re: [PATCH 2/4] Add --initrd argument to mdadm

[Jes Sorensen]

On 01/25/12 12:37, NeilBrown wrote:
>> In the light of the discussion so far, do you have any preference for
>> > --preboot/--preroot/--initrd/--offroot
>> > 
> How about
>    -@
> 
> A single-character which reflects what it actually does :-)
> 
> I don't like --initrd because it is just wrong.
> 
> I can live with the others.
> 
> I think --offroot is closest to the right meaning (it is both pre and post).
> I confess a little discomfort with the word "off" as it can mean both "not on"
> and "was on".
> 
> "Did you run mdadm off root, or off /usr ??  Neither, I ran it from initramfs
> which is off root."
> 
> But I agree with Doug that the term is sufficiently unusual that people are
> unlikely to assume anything about it, so it should be safe.
> 
> I'm tempted to suggest something completely different that starts with --no-,
> like
>   --no-session-manage
> but the word 'manage' already has a couple of meaning for mdadm.
> 
> So I'm leaning towards --offroot  though I'd go for --outside-root if it were
> a bit shorter.
> 
> Maybe we need a name for this thing that exists before root is mounted and
> remains until after root is unmounted.  If we had a name for that which
> reflected its reality, that would work for me.
>  --initial-root
> isn't too bad but probably could be better.
>  --outer-root
> ??
> 
> 
> Yes, I know - you wanted a decision and I've just created more options !!
> 
>  --offroot

Ok I am going to make an executive decision here and name it --offroot.
I just want to get this settled and move on, so whoever wants to argue
about the name, too late, go back and pound your sandbag instead :)

I was tempted to make -@ a short for --offroot, but I am not sure how
shell safe @ is, so some other time.

Patches coming up shortly.

Cheers,
Jes

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

[Jes.Sorensen]

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

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

diff --git a/mdmon.c b/mdmon.c
index a65c4a4..139bd85 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -276,6 +276,12 @@ void usage(void)
 	exit(2);
 }
 
+/*
+ * Option values that don't have a short version, to avoid clashing\
+ * with ascii values
+ */
+#define INITRD_OPT	0x100
+
 static int mdmon(char *devname, int devnum, int must_fork, int takeover);
 
 int main(int argc, char *argv[])
@@ -283,6 +289,7 @@ int main(int argc, char *argv[])
 	char *container_name = NULL;
 	int devnum;
 	char *devname;
+	char *c;
 	int status = 0;
 	int opt;
 	int all = 0;
@@ -291,6 +298,7 @@ int main(int argc, char *argv[])
 		{"all", 0, NULL, 'a'},
 		{"takeover", 0, NULL, 't'},
 		{"help", 0, NULL, 'h'},
+		{"initrd", 0, NULL, INITRD_OPT},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -304,6 +312,11 @@ int main(int argc, char *argv[])
 			container_name = optarg;
 			takeover = 1;
 			break;
+		case INITRD_OPT:
+			c = argv[0];
+			c[0] = '@';
+			continue;
+			
 		case 'h':
 		default:
 			usage();
-- 
1.7.8.3

Re: [PATCH 3/4] Add --initrd argument to mdmon

[NeilBrown]

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

On Tue, 17 Jan 2012 11:48:49 +0100 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  mdmon.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/mdmon.c b/mdmon.c
> index a65c4a4..139bd85 100644
> --- a/mdmon.c
> +++ b/mdmon.c
> @@ -276,6 +276,12 @@ void usage(void)
>  	exit(2);
>  }
>  
> +/*
> + * Option values that don't have a short version, to avoid clashing\
> + * with ascii values

The purpose of the trailing slosh (back slash) is ....?

> + */
> +#define INITRD_OPT	0x100

Can we make this an enum just like in mdadm??

> +
>  static int mdmon(char *devname, int devnum, int must_fork, int takeover);
>  
>  int main(int argc, char *argv[])
> @@ -283,6 +289,7 @@ int main(int argc, char *argv[])
>  	char *container_name = NULL;
>  	int devnum;
>  	char *devname;
> +	char *c;
>  	int status = 0;
>  	int opt;
>  	int all = 0;
> @@ -291,6 +298,7 @@ int main(int argc, char *argv[])
>  		{"all", 0, NULL, 'a'},
>  		{"takeover", 0, NULL, 't'},
>  		{"help", 0, NULL, 'h'},
> +		{"initrd", 0, NULL, INITRD_OPT},
>  		{NULL, 0, NULL, 0}
>  	};
>  
> @@ -304,6 +312,11 @@ int main(int argc, char *argv[])
>  			container_name = optarg;
>  			takeover = 1;
>  			break;
> +		case INITRD_OPT:
> +			c = argv[0];
> +			c[0] = '@';
> +			continue;

And can we use
                        argv[0][0] = '@';

here too?

Otherwise looks good.

Thanks,
NeilBrown

> +			
>  		case 'h':
>  		default:
>  			usage();


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

Re: [PATCH 3/4] Add --initrd argument to mdmon

[Jes Sorensen]

On 01/22/12 12:22, NeilBrown wrote:
> On Tue, 17 Jan 2012 11:48:49 +0100 Jes.Sorensen@redhat.com wrote:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  mdmon.c |   13 +++++++++++++
>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/mdmon.c b/mdmon.c
>> index a65c4a4..139bd85 100644
>> --- a/mdmon.c
>> +++ b/mdmon.c
>> @@ -276,6 +276,12 @@ void usage(void)
>>  	exit(2);
>>  }
>>  
>> +/*
>> + * Option values that don't have a short version, to avoid clashing\
>> + * with ascii values
> 
> The purpose of the trailing slosh (back slash) is ....?

It was there to see if you were awake :) .... or because someone didn't
see what he was doing.

>> + */
>> +#define INITRD_OPT	0x100
> 
> Can we make this an enum just like in mdadm??

Sure - I will keep the value though, to avoid clashing with ascii.

>> +
>>  static int mdmon(char *devname, int devnum, int must_fork, int takeover);
>>  
>>  int main(int argc, char *argv[])
>> @@ -283,6 +289,7 @@ int main(int argc, char *argv[])
>>  	char *container_name = NULL;
>>  	int devnum;
>>  	char *devname;
>> +	char *c;
>>  	int status = 0;
>>  	int opt;
>>  	int all = 0;
>> @@ -291,6 +298,7 @@ int main(int argc, char *argv[])
>>  		{"all", 0, NULL, 'a'},
>>  		{"takeover", 0, NULL, 't'},
>>  		{"help", 0, NULL, 'h'},
>> +		{"initrd", 0, NULL, INITRD_OPT},
>>  		{NULL, 0, NULL, 0}
>>  	};
>>  
>> @@ -304,6 +312,11 @@ int main(int argc, char *argv[])
>>  			container_name = optarg;
>>  			takeover = 1;
>>  			break;
>> +		case INITRD_OPT:
>> +			c = argv[0];
>> +			c[0] = '@';
>> +			continue;
> 
> And can we use
>                         argv[0][0] = '@';
> 
> here too?

Sure

> Otherwise looks good.

Thanks, I'll update and also do the help message + man-page stuff. v2
coming up shortly.

Cheers,
Jes

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

[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 c0ccf55..3edf2f1 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -182,6 +182,7 @@ int main(int argc, char *argv[])
 		case InitrdOpt:
 			c = argv[0];
 			c[0] = '@';
+			__initrd = 1;
 			continue;
 
 		case ':':
diff --git a/mdadm.h b/mdadm.h
index d1ac1e8..fddd98d 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 __initrd;
diff --git a/util.c b/util.c
index 6985a70..89299b8 100644
--- a/util.c
+++ b/util.c
@@ -32,6 +32,8 @@
 #include	<dirent.h>
 #include	<signal.h>
 
+int __initrd;
+
 /*
  * 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 (__initrd) {
+					execl(paths[i], "mdmon", "--initrd",
+					      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 0/4] add --initrd argument to mdadm/mdmon

[Doug Ledford]

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

On 01/17/2012 05:48 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> In order to resolve the problem with reboots hanging on systems with /
> on an IMSM RAID, systemd needs to be able to see that a given mdadm
> and mdmon process was launched from the initramfs, and allow it to
> survive until it gets back to there, so it can unmount the root
> filesystem safely.
> 
> These patches introduces the --initrd argument to mdadm which makes it
> change the first character of argv[0] to '@' to match the convention
> set by systemd.
> 
> In addition I also changed mdmon to use getopt_long to make it easier
> to add more command line arguments to it. Note that I went through
> great effort to preserve the behavior, even though the old code did
> some somewhat dodgy stuff using changing the arvg buffer pointing to
> the '--all' argument.
> 
> Comments?
> 
> Thanks,
> Jes
> 
> Jes Sorensen (4):
>   mdmon: Use getopt_long() to parse command line options
>   Add --initrd argument to mdadm
>   Add --initrd argument to mdmon
>   Spawn mdmon with --initrd if mdadm was launched with --initrd
> 
>  ReadMe.c |    3 ++
>  mdadm.c  |   11 ++++++++++
>  mdadm.h  |    3 ++
>  mdmon.c  |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  util.c   |   17 ++++++++++++---
>  5 files changed, 83 insertions(+), 15 deletions(-)
> 

Patch set looks reasonable to me, although you missed adding the new
option to the mdadm man page.

Do we have a specific systemd version for which we know that --initrd is
needed, or a version before which it is the wrong thing to do?

Ditto for dracut, do we know what the minimum version of dracut is
before it will use the --initrd parameter on its mdadm calls?


-- 
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 0/4] add --initrd argument to mdadm/mdmon

[Jes Sorensen]

On 01/17/12 20:57, Doug Ledford wrote:
> Patch set looks reasonable to me, although you missed adding the new
> option to the mdadm man page.
> 
> Do we have a specific systemd version for which we know that --initrd is
> needed, or a version before which it is the wrong thing to do?
> 
> Ditto for dracut, do we know what the minimum version of dracut is
> before it will use the --initrd parameter on its mdadm calls?

I actually did this on purpose - or rather I left it out of the help
message on purpose. The option is really only meant to be used by
dracut/systemd, it doesn't make much sense for normal users/admins to
play with it directly.

If you think it makes sense, I can add documentation for it.

Right now I believe dracut isn't ready to call mdadm with --initrd,
however systemd in rawhide is aware of the '@' hack. I am not sure of
the specific version, but we should document the final minimum versions
once it's all sorted so all distros know which versions to pick.

Cheers,
Jes

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

[NeilBrown]

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

On Tue, 17 Jan 2012 21:16:16 +0100 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:

> On 01/17/12 20:57, Doug Ledford wrote:
> > Patch set looks reasonable to me, although you missed adding the new
> > option to the mdadm man page.
> > 
> > Do we have a specific systemd version for which we know that --initrd is
> > needed, or a version before which it is the wrong thing to do?
> > 
> > Ditto for dracut, do we know what the minimum version of dracut is
> > before it will use the --initrd parameter on its mdadm calls?
> 
> I actually did this on purpose - or rather I left it out of the help
> message on purpose. The option is really only meant to be used by
> dracut/systemd, it doesn't make much sense for normal users/admins to
> play with it directly.

You left it out of the help message for mdmon, but included it in the help
message for mdadm!

> 
> If you think it makes sense, I can add documentation for it.

I think it should be in the help messages and definitely should be in the man
page.

If you fix that up and the other little things I've identified, then I'll
apply it .... providing we have a name that I can live with.

Thanks,
NeilBrown


> 
> Right now I believe dracut isn't ready to call mdadm with --initrd,
> however systemd in rawhide is aware of the '@' hack. I am not sure of
> the specific version, but we should document the final minimum versions
> once it's all sorted so all distros know which versions to pick.
> 
> Cheers,
> Jes


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

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

[Lennart Poettering]

On 17.01.2012 20:57, Doug Ledford wrote:

>
> Patch set looks reasonable to me, although you missed adding the new
> option to the mdadm man page.

If you decide to document this it might make sense to add a reference to 
http://www.freedesktop.org/wiki/Software/systemd/RootStorageDaemons 
which explains the semantics in a bit more detail.

> Do we have a specific systemd version for which we know that --initrd is
> needed, or a version before which it is the wrong thing to do?

Well, systemd honours the '@' starting with v38, and for older versions 
the '@' is practically a NOP, but not break anything that wasn't broken 
anyway ;-)

Lennart