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>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <e26af7db-a283-47ca-fc61-89af99f52c17@huaweicloud.com>
Date:   Wed, 31 May 2023 09:22:54 +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: <b9fd7105-eadc-29cb-fa2e-24109f4a99b7@linux.dev>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.
>>
>> 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.

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.

Thanks,
Kuai