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

Hi,

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

quoted

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

quoted

Hi,

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

quoted

On 5/29/23 21:34, Yu Kuai wrote:

quoted

From: Yu Kuai <redacted>

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

quoted

quoted

quoted

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

quoted

quoted

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

Yes, I missed that.

Thanks,
Guoqing