Messages in this thread | | | Subject | Re: [PATCH -next v2 3/7] md: delay choosing sync direction to md_start_sync() | From | Yu Kuai <> | Date | Tue, 15 Aug 2023 14:00:28 +0800 |
| |
Hi,
在 2023/08/15 11:09, Yu Kuai 写道: > From: Yu Kuai <yukuai3@huawei.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@huawei.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); >
| |