Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v2] md/raid5: don't allow concurrent reshape with recovery
To:     Guoqing Jiang <guoqing.jiang@linux.dev>,
        Yu Kuai <yukuai1@huaweicloud.com>, song@kernel.org,
        pmenzel@molgen.mpg.de
Cc:     linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, yangerkun@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20230529133410.2125914-1-yukuai1@huaweicloud.com>
 <b9fd7105-eadc-29cb-fa2e-24109f4a99b7@linux.dev>
 <e26af7db-a283-47ca-fc61-89af99f52c17@huaweicloud.com>
 <e2c936ce-5832-9d69-919a-c98af673bb3a@linux.dev>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <98f285ff-5afb-1d12-f087-cf04a5208e21@huaweicloud.com>
Date:   Wed, 31 May 2023 11:20:18 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <e2c936ce-5832-9d69-919a-c98af673bb3a@linux.dev>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

在 2023/05/31 9:49, Guoqing Jiang 写道:
> 
> 
> 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.

Yes, reshape first, and yes they can't concurrent.
> 
>> 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.

It's in rdev->flags, if MD_FEATURE_REPLACEMENT is set in rdev
metadata(sb->feature_map), then this rdev will mark Replacement, and
later this rdev will set to mirros[]->replacement in setup_conf().

Thanks,
Kuai