linux-kernel - Re: [PATCH -next v2 3/7] md: delay choosing sync direction to md_start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Aug 2023 14:53:34 -0700
From:   Song Liu <song@...nel.org>
To:     Yu Kuai <yukuai1@...weicloud.com>
Cc:     xni@...hat.com, linux-raid@...r.kernel.org,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        yangerkun@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next v2 3/7] md: delay choosing sync direction to md_start_sync()

On Tue, Aug 15, 2023 at 6:07 PM Yu Kuai <yukuai1@...weicloud.com> wrote:
>
> Hi,
>
> 在 2023/08/15 23:54, Song Liu 写道:
> > On Tue, Aug 15, 2023 at 2:00 PM Yu Kuai <yukuai1@...weicloud.com> 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