Re: [PATCH v2] md/raid5: don't allow concurrent reshape with recovery

[Guoqing Jiang <guoqing.jiang@linux.dev>]

On 5/31/23 09:22, Yu Kuai wrote:
> Hi,
>
> 在 2023/05/31 9:06, Guoqing Jiang 写道:
>>
>>
>> On 5/29/23 21:34, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> Commit 0aecb06e2249 ("md/raid5: don't allow replacement while reshape
>>> is in progress") fixes that replacement can be set if reshape is
>>> interrupted, which will cause that array can't be assembled.

I just pulled md tree, but can't find the commit id either in md-next or 
md-fixes .
gjiang@pc:~/storage/md> git branch
  master
  md-fixes
* md-next
gjiang@pc:~/storage/md> git branch --contain 0aecb06e2249
error: malformed object name 0aecb06e2249

>>> There is a similar problem on the other side, if recovery is
>>> interrupted, then reshape can start, which will cause the same problem.
>>>
>>> Fix the problem by not starting to reshape while recovery is still in
>>> progress.
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>> ---
>>> Changes in v2:
>>>   - fix some typo in commit message.
>>>
>>>   drivers/md/raid5.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>>> index 8686d629e3f2..6615abf54d3f 100644
>>> --- a/drivers/md/raid5.c
>>> +++ b/drivers/md/raid5.c
>>> @@ -8525,6 +8525,7 @@ static int raid5_start_reshape(struct mddev 
>>> *mddev)
>>>       struct r5conf *conf = mddev->private;
>>>       struct md_rdev *rdev;
>>>       int spares = 0;
>>> +    int i;
>>>       unsigned long flags;
>>>       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
>>> @@ -8536,6 +8537,13 @@ static int raid5_start_reshape(struct mddev 
>>> *mddev)
>>>       if (has_failed(conf))
>>>           return -EINVAL;
>>> +    /* raid5 can't handle concurrent reshape and recovery */
>>> +    if (mddev->recovery_cp < MaxSector)
>>> +        return -EBUSY;
>>> +    for (i = 0; i < conf->raid_disks; i++)
>>> +        if (rdev_mdlock_deref(mddev, conf->disks[i].replacement))
>>> +            return -EBUSY;
>>> +
>>
>> Does it mean reshape and recovery  can happen in parallel without the 
>> change?
>> I really doubt about it given any kind of internal io (resync, 
>> reshape and recovery)
>> is handled by resync thread. And IIUC either md_do_sync or 
>> md_check_recovery
>> should avoid it, no need to do it in personality layer.
>>
>
> They can't, in this case recovery is interrupted, then recovery can't
> make progress, and md_check_recovery() will start reshape, and after
> reshape is done, recovery will continue, and data will be corrupted
> because raid456 reshape doesn't handle replacement.

So, do reshape first then recovery, right? I don't see concurrent 
reshape and recovery
happen based on your description, if concurrent reshape and recovery is 
possible
then I believe we really have big trouble.

> And by the way in raid456 is that if system reboot, this array can't be
> assembled, raid5_run() will fail if reshape and replacement are both
> set.

Assemble an array need to read data from sb, I don't know which place 
record replacement,
I probably misunderstand something.

Thanks,
Guoqing