Re: [PATCH v1] mdadm/grow: reshape would be stuck from raid1 to raid5

[NeilBrown <neilb@suse.com>]

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