All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mdadm/grow: reshape would be stuck from raid1 to raid5
@ 2017-03-20  5:20 Zhilong Liu
  2017-03-27 22:10 ` jes.sorensen
  0 siblings, 1 reply; 10+ messages in thread
From: Zhilong Liu @ 2017-03-20  5:20 UTC (permalink / raw)
  To: jes.sorensen; +Cc: linux-raid, Zhilong Liu

it would be stuck at the beginning of reshape progress
when grows array from raid1 to raid5, correct the name
of mdadm-grow-continue@.service in continue_via_systemd.

reproduce steps:
./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 Grow.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Grow.c b/Grow.c
index 455c5f9..10c02a1 100755
--- a/Grow.c
+++ b/Grow.c
@@ -2808,13 +2808,11 @@ static int continue_via_systemd(char *devnm)
 		 */
 		close(2);
 		open("/dev/null", O_WRONLY);
-		snprintf(pathbuf, sizeof(pathbuf), "mdadm-grow-continue@%s.service",
-			 devnm);
+		snprintf(pathbuf, sizeof(pathbuf), "mdadm-grow-continue@.service");
 		status = execl("/usr/bin/systemctl", "systemctl",
 			       "start",
 			       pathbuf, NULL);
-		status = execl("/bin/systemctl", "systemctl", "start",
-			       pathbuf, NULL);
+		pr_err("/usr/bin/systemctl %s got failure\n", pathbuf);
 		exit(1);
 	case -1: /* Just do it ourselves. */
 		break;
-- 
2.6.6


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-03-20  5:20 [PATCH] mdadm/grow: reshape would be stuck from raid1 to raid5 Zhilong Liu
@ 2017-03-27 22:10 ` jes.sorensen
  2017-03-28 14:03   ` Liu Zhilong
  2017-03-30  7:38   ` [PATCH v1] " Zhilong Liu
  0 siblings, 2 replies; 10+ messages in thread
From: jes.sorensen @ 2017-03-27 22:10 UTC (permalink / raw)
  To: Zhilong Liu; +Cc: linux-raid, Harald Hoyer

Zhilong Liu <zlliu@suse.com> writes:
> it would be stuck at the beginning of reshape progress
> when grows array from raid1 to raid5, correct the name
> of mdadm-grow-continue@.service in continue_via_systemd.
>
> reproduce steps:
> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>  Grow.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 455c5f9..10c02a1 100755
> --- a/Grow.c
> +++ b/Grow.c
> @@ -2808,13 +2808,11 @@ static int continue_via_systemd(char *devnm)
>  		 */
>  		close(2);
>  		open("/dev/null", O_WRONLY);
> -		snprintf(pathbuf, sizeof(pathbuf), "mdadm-grow-continue@%s.service",
> -			 devnm);
> +		snprintf(pathbuf, sizeof(pathbuf), "mdadm-grow-continue@.service");

My memory is rusty here, isn't systemctl interpreting the device name in
mdadm-grow-continue@<device>.service as an argument?

>  		status = execl("/usr/bin/systemctl", "systemctl",
>  			       "start",
>  			       pathbuf, NULL);
> -		status = execl("/bin/systemctl", "systemctl", "start",
> -			       pathbuf, NULL);
> +		pr_err("/usr/bin/systemctl %s got failure\n", pathbuf);
>  		exit(1);

This assumes systemctl is location in /usr/bin only - you removed the
fallback case for it being location in /bin.

In addition, instead of saying 'got failure' lets do something with the
errno value so the user gets a more descriptive error message.

Cheers,
Jes

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-03-27 22:10 ` jes.sorensen
@ 2017-03-28 14:03   ` Liu Zhilong
  2017-03-30  7:38   ` [PATCH v1] " Zhilong Liu
  1 sibling, 0 replies; 10+ messages in thread
From: Liu Zhilong @ 2017-03-28 14:03 UTC (permalink / raw)
  To: jes.sorensen; +Cc: linux-raid, Harald Hoyer, shli


On 03/28/2017 06:10 AM, jes.sorensen@gmail.com wrote:
> Zhilong Liu <zlliu@suse.com> writes:
>> it would be stuck at the beginning of reshape progress
>> when grows array from raid1 to raid5, correct the name
>> of mdadm-grow-continue@.service in continue_via_systemd.
>>
>> reproduce steps:
>> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
>> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>>
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>> ---
>>   Grow.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index 455c5f9..10c02a1 100755
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -2808,13 +2808,11 @@ static int continue_via_systemd(char *devnm)
>>   		 */
>>   		close(2);
>>   		open("/dev/null", O_WRONLY);
>> -		snprintf(pathbuf, sizeof(pathbuf), "mdadm-grow-continue@%s.service",
>> -			 devnm);
>> +		snprintf(pathbuf, sizeof(pathbuf), "mdadm-grow-continue@.service");
> My memory is rusty here, isn't systemctl interpreting the device name in
> mdadm-grow-continue@<device>.service as an argument?

actually, the service started failed. paste the journalctl log here when 
reshape from mirror to raid5.

command: ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2

Mar 28 21:43:47 linux-g0sr kernel:  --- level:5 rd:3 wd:3
Mar 28 21:43:47 linux-g0sr kernel:  disk 0, o:1, dev:loop0
Mar 28 21:43:47 linux-g0sr kernel:  disk 1, o:1, dev:loop1
Mar 28 21:43:47 linux-g0sr kernel:  disk 2, o:1, dev:loop2
Mar 28 21:43:47 linux-g0sr kernel: md: reshape of RAID array md0
Mar 28 21:43:47 linux-g0sr kernel: md: minimum _guaranteed_  speed: 1000 
KB/sec/disk.
Mar 28 21:43:47 linux-g0sr kernel: md: using maximum available idle IO 
bandwidth (but not more than 2000 KB/sec) for reshape.
Mar 28 21:43:47 linux-g0sr kernel: md: using 128k window, over a total 
of 19968k.
Mar 28 21:43:47 linux-g0sr systemd[1]: Started Manage MD Reshape on 
/dev/md0.
Mar 28 21:43:47 linux-g0sr systemd[1]: mdadm-grow-continue@md0.service: 
Main process exited, code=exited, status=2/INVALIDARGUMENT
Mar 28 21:43:47 linux-g0sr systemd[1]: mdadm-grow-continue@md0.service: 
Unit entered failed state.
Mar 28 21:43:47 linux-g0sr systemd[1]: mdadm-grow-continue@md0.service: 
Failed with result 'exit-code'.
Mar 28 21:44:03 linux-g0sr kernel: md: md0: reshape interrupted.

>
>>   		status = execl("/usr/bin/systemctl", "systemctl",
>>   			       "start",
>>   			       pathbuf, NULL);
>> -		status = execl("/bin/systemctl", "systemctl", "start",
>> -			       pathbuf, NULL);
>> +		pr_err("/usr/bin/systemctl %s got failure\n", pathbuf);
>>   		exit(1);
> This assumes systemctl is location in /usr/bin only - you removed the
> fallback case for it being location in /bin.
>
> In addition, instead of saying 'got failure' lets do something with the
> errno value so the user gets a more descriptive error message.

I would continue to look at this issue, thanks for your time.

Thanks,
-Zhilong
> Cheers,
> Jes
> --
> 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
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-03-27 22:10 ` jes.sorensen
  2017-03-28 14:03   ` Liu Zhilong
@ 2017-03-30  7:38   ` Zhilong Liu
  2017-03-30 15:50     ` Jes Sorensen
  2017-04-03  4:36     ` NeilBrown
  1 sibling, 2 replies; 10+ messages in thread
From: Zhilong Liu @ 2017-03-30  7:38 UTC (permalink / raw)
  To: Jes.Sorensen; +Cc: linux-raid, Zhilong Liu

systemctl doesn't interpret mdadm-grow-continue@.service
correctly due to the wrong argument provided in [service],
it should be corrected %I as %i. Otherwise, if the service
cannot start by systemctl and the reshap progress would be
stuck all time when grows array from raid1 to raid5.

reproduce steps:
./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2

Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
 systemd/mdadm-grow-continue@.service | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/systemd/mdadm-grow-continue@.service b/systemd/mdadm-grow-continue@.service
index 5c667d2..882bc0b 100644
--- a/systemd/mdadm-grow-continue@.service
+++ b/systemd/mdadm-grow-continue@.service
@@ -10,7 +10,7 @@ Description=Manage MD Reshape on /dev/%I
 DefaultDependencies=no
 
 [Service]
-ExecStart=BINDIR/mdadm --grow --continue /dev/%I
+ExecStart=BINDIR/mdadm --grow --continue /dev/%i
 StandardInput=null
 StandardOutput=null
 StandardError=null
-- 
2.10.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-03-30  7:38   ` [PATCH v1] " Zhilong Liu
@ 2017-03-30 15:50     ` Jes Sorensen
  2017-04-03  4:36     ` NeilBrown
  1 sibling, 0 replies; 10+ messages in thread
From: Jes Sorensen @ 2017-03-30 15:50 UTC (permalink / raw)
  To: Zhilong Liu; +Cc: linux-raid

On 03/30/2017 03:38 AM, Zhilong Liu wrote:
> systemctl doesn't interpret mdadm-grow-continue@.service
> correctly due to the wrong argument provided in [service],
> it should be corrected %I as %i. Otherwise, if the service
> cannot start by systemctl and the reshap progress would be
> stuck all time when grows array from raid1 to raid5.
>
> reproduce steps:
> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>  systemd/mdadm-grow-continue@.service | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This solution looks more correct to me :)

Applied!

Thanks,
Jes



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-03-30  7:38   ` [PATCH v1] " Zhilong Liu
  2017-03-30 15:50     ` Jes Sorensen
@ 2017-04-03  4:36     ` NeilBrown
  2017-04-04  5:07       ` Zhilong
  1 sibling, 1 reply; 10+ messages in thread
From: NeilBrown @ 2017-04-03  4:36 UTC (permalink / raw)
  To: Jes.Sorensen; +Cc: linux-raid, Zhilong Liu

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

On Thu, Mar 30 2017, Zhilong Liu wrote:

> systemctl doesn't interpret mdadm-grow-continue@.service
> correctly due to the wrong argument provided in [service],
> it should be corrected %I as %i. Otherwise, if the service
> cannot start by systemctl and the reshap progress would be
> stuck all time when grows array from raid1 to raid5.
>
> reproduce steps:
> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
>  systemd/mdadm-grow-continue@.service | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/systemd/mdadm-grow-continue@.service b/systemd/mdadm-grow-continue@.service
> index 5c667d2..882bc0b 100644
> --- a/systemd/mdadm-grow-continue@.service
> +++ b/systemd/mdadm-grow-continue@.service
> @@ -10,7 +10,7 @@ Description=Manage MD Reshape on /dev/%I
>  DefaultDependencies=no
>  
>  [Service]
> -ExecStart=BINDIR/mdadm --grow --continue /dev/%I
> +ExecStart=BINDIR/mdadm --grow --continue /dev/%i

Do you know why this makes a difference?  I don't think it should.
man systemd.unit says that "%i" is the "Instance name" while "%I" is the
"Unescaped instance name".

The Instance name here is something like "md0" so there is nothing to
escape.

I would rather not change it unless we know exactly why it is broken,
and I don't find your explanation to be convincing.

NeilBrown


>  StandardInput=null
>  StandardOutput=null
>  StandardError=null
> -- 
> 2.10.2
>
> --
> 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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-04-03  4:36     ` NeilBrown
@ 2017-04-04  5:07       ` Zhilong
  2017-04-04  5:40         ` Zhilong
  0 siblings, 1 reply; 10+ messages in thread
From: Zhilong @ 2017-04-04  5:07 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid



Send from iPhone

> 在 2017年4月3日,12:36,NeilBrown <neilb@suse.com> 写道:
> 
>> On Thu, Mar 30 2017, Zhilong Liu wrote:
>> 
>> systemctl doesn't interpret mdadm-grow-continue@.service
>> correctly due to the wrong argument provided in [service],
>> it should be corrected %I as %i. Otherwise, if the service
>> cannot start by systemctl and the reshap progress would be
>> stuck all time when grows array from raid1 to raid5.
>> 
>> reproduce steps:
>> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
>> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>> 
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>> ---
>> systemd/mdadm-grow-continue@.service | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/systemd/mdadm-grow-continue@.service b/systemd/mdadm-grow-continue@.service
>> index 5c667d2..882bc0b 100644
>> --- a/systemd/mdadm-grow-continue@.service
>> +++ b/systemd/mdadm-grow-continue@.service
>> @@ -10,7 +10,7 @@ Description=Manage MD Reshape on /dev/%I
>> DefaultDependencies=no
>> 
>> [Service]
>> -ExecStart=BINDIR/mdadm --grow --continue /dev/%I
>> +ExecStart=BINDIR/mdadm --grow --continue /dev/%i
> 
> Do you know why this makes a difference?  I don't think it should.
> man systemd.unit says that "%i" is the "Instance name" while "%I" is the
> "Unescaped instance name".
> 
> The Instance name here is something like "md0" so there is nothing to
> escape.
> 
> I would rather not change it unless we know exactly why it is broken,
> and I don't find your explanation to be convincing.
> 

Exactly, you're correct, in this case, %i and %I are the same. The root cause is the ExecStart part, all the path name should be verified by systemd-escape,such as:
/sbin/mdadm should be corrected as sbin-mdadm, and /dev/%I should be -dev-%I. Thus I'm sorry for this patch, I do agree with you not to change it. And say sorry to Jes.

Thanks 
-zhilong 

> NeilBrown
> 
> 
>> StandardInput=null
>> StandardOutput=null
>> StandardError=null
>> -- 
>> 2.10.2
>> 
>> --
>> 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-04-04  5:07       ` Zhilong
@ 2017-04-04  5:40         ` Zhilong
  2017-04-04  6:30           ` NeilBrown
  0 siblings, 1 reply; 10+ messages in thread
From: Zhilong @ 2017-04-04  5:40 UTC (permalink / raw)
  To: NeilBrown; +Cc: Jes.Sorensen, linux-raid



Send from iPhone

> 在 2017年4月4日,13:07,Zhilong <zlliu@suse.com> 写道:
> 
> 
> 
> Send from iPhone
> 
>>> 在 2017年4月3日,12:36,NeilBrown <neilb@suse.com> 写道:
>>> 
>>> On Thu, Mar 30 2017, Zhilong Liu wrote:
>>> 
>>> systemctl doesn't interpret mdadm-grow-continue@.service
>>> correctly due to the wrong argument provided in [service],
>>> it should be corrected %I as %i. Otherwise, if the service
>>> cannot start by systemctl and the reshap progress would be
>>> stuck all time when grows array from raid1 to raid5.
>>> 
>>> reproduce steps:
>>> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
>>> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>>> 
>>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>> ---
>>> systemd/mdadm-grow-continue@.service | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/systemd/mdadm-grow-continue@.service b/systemd/mdadm-grow-continue@.service
>>> index 5c667d2..882bc0b 100644
>>> --- a/systemd/mdadm-grow-continue@.service
>>> +++ b/systemd/mdadm-grow-continue@.service
>>> @@ -10,7 +10,7 @@ Description=Manage MD Reshape on /dev/%I
>>> DefaultDependencies=no
>>> 
>>> [Service]
>>> -ExecStart=BINDIR/mdadm --grow --continue /dev/%I
>>> +ExecStart=BINDIR/mdadm --grow --continue /dev/%i
>> 
>> Do you know why this makes a difference?  I don't think it should.
>> man systemd.unit says that "%i" is the "Instance name" while "%I" is the
>> "Unescaped instance name".
>> 
>> The Instance name here is something like "md0" so there is nothing to
>> escape.
>> 
>> I would rather not change it unless we know exactly why it is broken,
>> and I don't find your explanation to be convincing.
>> 
> 
> Exactly, you're correct, in this case, %i and %I are the same. The root cause is the ExecStart part, all the path name should be verified by systemd-escape,such as:
> /sbin/mdadm should be corrected as sbin-mdadm, and /dev/%I should be -dev-%I. Thus I'm sorry for this patch, I do agree with you not to change it. And say sorry to Jes.
> 

How about modifying this patch as:

ExecStart=-sbin-mdadm --grow --continue -dev-%I

Thanks,
Zhilong 

> Thanks 
> -zhilong 
> 
>> NeilBrown
>> 
>> 
>>> StandardInput=null
>>> StandardOutput=null
>>> StandardError=null
>>> -- 
>>> 2.10.2
>>> 
>>> --
>>> 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
> 
> --
> 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
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-04-04  5:40         ` Zhilong
@ 2017-04-04  6:30           ` NeilBrown
  2017-04-05 15:37             ` jes.sorensen
  0 siblings, 1 reply; 10+ messages in thread
From: NeilBrown @ 2017-04-04  6:30 UTC (permalink / raw)
  To: Zhilong; +Cc: Jes.Sorensen, linux-raid

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

On Tue, Apr 04 2017, Zhilong wrote:

> Send from iPhone
>
>> 在 2017年4月4日,13:07,Zhilong <zlliu@suse.com> 写道:
>> 
>> 
>> 
>> Send from iPhone
>> 
>>>> 在 2017年4月3日,12:36,NeilBrown <neilb@suse.com> 写道:
>>>> 
>>>> On Thu, Mar 30 2017, Zhilong Liu wrote:
>>>> 
>>>> systemctl doesn't interpret mdadm-grow-continue@.service
>>>> correctly due to the wrong argument provided in [service],
>>>> it should be corrected %I as %i. Otherwise, if the service
>>>> cannot start by systemctl and the reshap progress would be
>>>> stuck all time when grows array from raid1 to raid5.
>>>> 
>>>> reproduce steps:
>>>> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
>>>> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>>>> 
>>>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>>> ---
>>>> systemd/mdadm-grow-continue@.service | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/systemd/mdadm-grow-continue@.service b/systemd/mdadm-grow-continue@.service
>>>> index 5c667d2..882bc0b 100644
>>>> --- a/systemd/mdadm-grow-continue@.service
>>>> +++ b/systemd/mdadm-grow-continue@.service
>>>> @@ -10,7 +10,7 @@ Description=Manage MD Reshape on /dev/%I
>>>> DefaultDependencies=no
>>>> 
>>>> [Service]
>>>> -ExecStart=BINDIR/mdadm --grow --continue /dev/%I
>>>> +ExecStart=BINDIR/mdadm --grow --continue /dev/%i
>>> 
>>> Do you know why this makes a difference?  I don't think it should.
>>> man systemd.unit says that "%i" is the "Instance name" while "%I" is the
>>> "Unescaped instance name".
>>> 
>>> The Instance name here is something like "md0" so there is nothing to
>>> escape.
>>> 
>>> I would rather not change it unless we know exactly why it is broken,
>>> and I don't find your explanation to be convincing.
>>> 
>> 
>> Exactly, you're correct, in this case, %i and %I are the same. The root cause is the ExecStart part, all the path name should be verified by systemd-escape,such as:
>> /sbin/mdadm should be corrected as sbin-mdadm, and /dev/%I should be -dev-%I. Thus I'm sorry for this patch, I do agree with you not to change it. And say sorry to Jes.
>> 
>
> How about modifying this patch as:
>
> ExecStart=-sbin-mdadm --grow --continue -dev-%I
>

Why do you think anything needs changing here?

I have a tumbleweed install with the standard
mdadm-grow-continue@.server
file. i.e.

  ExecStart=/sbin/mdadm --grow --continue /dev/%I 

I run
  strace -f -s 1000 -p 1 -o /tmp/strace

in one window, then

  systemctl start mdadm-grow-continue@md0.service

in another.

Then

 grep execve /tmp/strace

shows:

18680 execve("/sbin/mdadm", ["/sbin/mdadm", "--grow", "--continue", "/dev/md0"], [/* 3 vars */] <unfinished ...>

which shows that mdadm is being correctly.

There is nothing to fix here that I can see.

NeilBrown

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5
  2017-04-04  6:30           ` NeilBrown
@ 2017-04-05 15:37             ` jes.sorensen
  0 siblings, 0 replies; 10+ messages in thread
From: jes.sorensen @ 2017-04-05 15:37 UTC (permalink / raw)
  To: NeilBrown; +Cc: Zhilong, linux-raid

NeilBrown <neilb@suse.com> writes:
> On Tue, Apr 04 2017, Zhilong wrote:
>
>> Send from iPhone
>>
>>> 在 2017年4月4日,13:07,Zhilong <zlliu@suse.com> 写道:
>>> 
>>> 
>>> 
>>> Send from iPhone
>>> 
>>>>> 在 2017年4月3日,12:36,NeilBrown <neilb@suse.com> 写道:
>>>>> 
>>>>> On Thu, Mar 30 2017, Zhilong Liu wrote:
>>>>> 
>>>>> systemctl doesn't interpret mdadm-grow-continue@.service
>>>>> correctly due to the wrong argument provided in [service],
>>>>> it should be corrected %I as %i. Otherwise, if the service
>>>>> cannot start by systemctl and the reshap progress would be
>>>>> stuck all time when grows array from raid1 to raid5.
>>>>> 
>>>>> reproduce steps:
>>>>> ./mdadm -CR /dev/md0 -l1 -b internal -n2 /dev/loop[0-1]
>>>>> ./mdadm --grow /dev/md0 -l5 -n3 -a /dev/loop2
>>>>> 
>>>>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>>>> ---
>>>>> systemd/mdadm-grow-continue@.service | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/systemd/mdadm-grow-continue@.service
>>>>> b/systemd/mdadm-grow-continue@.service
>>>>> index 5c667d2..882bc0b 100644
>>>>> --- a/systemd/mdadm-grow-continue@.service
>>>>> +++ b/systemd/mdadm-grow-continue@.service
>>>>> @@ -10,7 +10,7 @@ Description=Manage MD Reshape on /dev/%I
>>>>> DefaultDependencies=no
>>>>> 
>>>>> [Service]
>>>>> -ExecStart=BINDIR/mdadm --grow --continue /dev/%I
>>>>> +ExecStart=BINDIR/mdadm --grow --continue /dev/%i
>>>> 
>>>> Do you know why this makes a difference?  I don't think it should.
>>>> man systemd.unit says that "%i" is the "Instance name" while "%I" is the
>>>> "Unescaped instance name".
>>>> 
>>>> The Instance name here is something like "md0" so there is nothing to
>>>> escape.
>>>> 
>>>> I would rather not change it unless we know exactly why it is broken,
>>>> and I don't find your explanation to be convincing.
>>>> 
>>> 
>>> Exactly, you're correct, in this case, %i and %I are the same. The
>>> root cause is the ExecStart part, all the path name should be
>>> verified by systemd-escape,such as:
>>> /sbin/mdadm should be corrected as sbin-mdadm, and /dev/%I should
>>> be -dev-%I. Thus I'm sorry for this patch, I do agree with you not
>>> to change it. And say sorry to Jes.
>>> 
>>
>> How about modifying this patch as:
>>
>> ExecStart=-sbin-mdadm --grow --continue -dev-%I
>>
>
> Why do you think anything needs changing here?
>
> I have a tumbleweed install with the standard
> mdadm-grow-continue@.server
> file. i.e.
>
>   ExecStart=/sbin/mdadm --grow --continue /dev/%I 
>
> I run
>   strace -f -s 1000 -p 1 -o /tmp/strace
>
> in one window, then
>
>   systemctl start mdadm-grow-continue@md0.service
>
> in another.
>
> Then
>
>  grep execve /tmp/strace
>
> shows:
>
> 18680 execve("/sbin/mdadm", ["/sbin/mdadm", "--grow", "--continue", "/dev/md0"], [/* 3 vars */] <unfinished ...>
>
> which shows that mdadm is being correctly.
>
> There is nothing to fix here that I can see.

Unless I see some convincing arguments, I am going to revert this
patch.

Jes

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-04-05 15:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-20  5:20 [PATCH] mdadm/grow: reshape would be stuck from raid1 to raid5 Zhilong Liu
2017-03-27 22:10 ` jes.sorensen
2017-03-28 14:03   ` Liu Zhilong
2017-03-30  7:38   ` [PATCH v1] " Zhilong Liu
2017-03-30 15:50     ` Jes Sorensen
2017-04-03  4:36     ` NeilBrown
2017-04-04  5:07       ` Zhilong
2017-04-04  5:40         ` Zhilong
2017-04-04  6:30           ` NeilBrown
2017-04-05 15:37             ` jes.sorensen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.