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

From: Guoqing Jiang
Date: Wed May 31 2023 - 03:34:12 EST

On 5/31/23 11:20, Yu Kuai wrote:

在 2023/05/31 9:49, Guoqing Jiang 写道:

On 5/31/23 09:22, Yu Kuai wrote:

在 2023/05/31 9:06, Guoqing Jiang 写道:

On 5/29/23 21:34, Yu Kuai wrote:
From: Yu Kuai <yukuai3@xxxxxxxxxx>

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

Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
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.

Then the subject, commit message and above comment are confusing given they can't happen
even without your change.

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

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

Yes, I missed that.