Re: [PATCH -next v2 3/7] md: delay choosing sync direction to md_start_sync()

[Song Liu]

On Tue, Aug 15, 2023 at 6:07 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> 在 2023/08/15 23:54, Song Liu 写道:
> > On Tue, Aug 15, 2023 at 2:00 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
> > [...]
> >>> +
> >>> +not_running:
> >>> +     clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> >>> +     clear_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> >>> +     clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
> >>> +     clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> >>> +     clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> >>> +     mddev_unlock(mddev);
> >>> +
> >>> +     wake_up(&resync_wait);
> >>> +     if (test_and_clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery) &&
> >>> +         mddev->sysfs_action)
> >>> +             sysfs_notify_dirent_safe(mddev->sysfs_action);
> >>>    }
> >>>
> >>>    /*
> >>> @@ -9379,7 +9402,6 @@ void md_check_recovery(struct mddev *mddev)
> >>>                return;
> >>>
> >>>        if (mddev_trylock(mddev)) {
> >>> -             int spares = 0;
> >>>                bool try_set_sync = mddev->safemode != 0;
> >>>
> >>>                if (!mddev->external && mddev->safemode == 1)
> >>> @@ -9467,29 +9489,11 @@ void md_check_recovery(struct mddev *mddev)
> >>>                clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
> >>>
> >>>                if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> >>> -                 test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
> >>> -                     goto not_running;
> >>> -             if (!md_choose_sync_direction(mddev, &spares))
> >>> -                     goto not_running;
> >>> -             if (mddev->pers->sync_request) {
> >>> -                     if (spares) {
> >>> -                             /* We are adding a device or devices to an array
> >>> -                              * which has the bitmap stored on all devices.
> >>> -                              * So make sure all bitmap pages get written
> >>> -                              */
> >>> -                             md_bitmap_write_all(mddev->bitmap);
> >>> -                     }
> >>> +                 test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
> >>
> >> Sorry that I made a mistake here while rebasing v2, here should be
> >>
> >> !test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)
> >>
> >> With this fixed, there are no new regression for mdadm tests using loop
> >> devicein my VM.
> >
> >                  if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> >                      !test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
> >                          queue_work(md_misc_wq, &mddev->sync_work);
> >                  } else {
> >
> > This doesn't look right. Should we do
> >
> >                  if (test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) &&
> >                      !test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
> >                          queue_work(md_misc_wq, &mddev->sync_work);
> >                  } else {
> >
> > instead?
> >
>
> Yes you're right, this is exactly what I did in v1, sorry that I keep
> making mistake while rebasing.

Please fix this, address comments from other reviews, and resend the
patches. Also, there are some typos in the commit logs, please also fix them.

Unfortunately, we won't ship this (and the two other big sets) in 6.6.

Thanks,
Song