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

[jes.sorensen@gmail.com]

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