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

[Guoqing Jiang]

On 5/31/23 11:20, Yu Kuai wrote:
> 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@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
> >   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@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
> > > 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