| 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
| ||
|
Message-ID: <bb11d6ca-978a-8e1d-e721-d9d84c9dc5e3@huaweicloud.com>
Date: Tue, 15 Aug 2023 14:00:28 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Yu Kuai <yukuai1@...weicloud.com>, xni@...hat.com, song@...nel.org
Cc: 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()
Hi,
在 2023/08/15 11:09, Yu Kuai 写道:
> From: Yu Kuai <yukuai3@...wei.com>
>
> Before this patch, for read-write array:
>
> 1) md_check_recover() found that something need to be done, and it'll
> try to grab 'reconfig_mutex'. The case that md_check_recover() need
> to do something:
> - array is not suspend;
> - super_block need to be updated;
> - 'MD_RECOVERY_NEEDED' or ''MD_RECOVERY_DONE' is set;
> - unusual case related to safemode;
>
> 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set,
> md_check_recover() will try to choose a sync direction, and then
> queue a work md_start_sync().
>
> 3) md_start_sync() register sync_thread;
>
> After this patch,
>
> 1) is the same;
> 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set,
> queue a work md_start_sync() directly;
> 3) md_start_sync() will try to choose a sync direction, and then
> register sync_thread();
>
> Because 'MD_RECOVERY_RUNNING' is cleared when sync_thread is done, 2)
> and 3) is always ran in serial and they can never concurrent, this
> change should not introduce any behavior change for now.
>
> Also fix a problem that md_start_sync() can clear 'MD_RECOVERY_RUNNING'
> without protection in error path, which might affect the logical in
> md_check_recovery().
>
> The advantage to change this is that array reconfiguration is
> independent from daemon now, and it'll be much easier to synchronize it
> with io, consider that io may rely on daemon thread to be done.
>
> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
> ---
> drivers/md/md.c | 70 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 37 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 4846ff6d25b0..03615b0e9fe1 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9291,6 +9291,22 @@ static bool md_choose_sync_direction(struct mddev *mddev, int *spares)
> static void md_start_sync(struct work_struct *ws)
> {
> struct mddev *mddev = container_of(ws, struct mddev, sync_work);
> + int spares = 0;
> +
> + mddev_lock_nointr(mddev);
> +
> + if (!md_choose_sync_direction(mddev, &spares))
> + goto not_running;
> +
> + if (!mddev->pers->sync_request)
> + goto not_running;
> +
> + /*
> + * 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.
> + */
> + if (spares)
> + md_bitmap_write_all(mddev->bitmap);
>
> rcu_assign_pointer(mddev->sync_thread,
> md_register_thread(md_do_sync, mddev, "resync"));
> @@ -9298,20 +9314,27 @@ static void md_start_sync(struct work_struct *ws)
> pr_warn("%s: could not start resync thread...\n",
> mdname(mddev));
> /* leave the spares where they are, it shouldn't hurt */
> - 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);
> - wake_up(&resync_wait);
> - if (test_and_clear_bit(MD_RECOVERY_RECOVER,
> - &mddev->recovery))
> - if (mddev->sysfs_action)
> - sysfs_notify_dirent_safe(mddev->sysfs_action);
> - } else
> - md_wakeup_thread(mddev->sync_thread);
> + goto not_running;
> + }
> +
> + mddev_unlock(mddev);
> + md_wakeup_thread(mddev->sync_thread);
> sysfs_notify_dirent_safe(mddev->sysfs_action);
> md_new_event();
> + return;
> +
> +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.
Thanks,
Kuai
> queue_work(md_misc_wq, &mddev->sync_work);
> - goto unlock;
> - }
> - not_running:
> - if (!mddev->sync_thread) {
> + } else {
> clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> wake_up(&resync_wait);
> - if (test_and_clear_bit(MD_RECOVERY_RECOVER,
> - &mddev->recovery))
> - if (mddev->sysfs_action)
> - sysfs_notify_dirent_safe(mddev->sysfs_action);
> }
> unlock:
> wake_up(&mddev->sb_wait);
>
Powered by blists - more mailing lists