lkml.org 
[lkml]   [2023]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync()
    On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
    >
    > 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 action, 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 action, and then register
    > sync_thread();
    >
    > Because 'MD_RECOVERY_RUNNING' is cleared when sync_thread is done, 2)
    > and 3) and md_do_sync() 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 | 73 ++++++++++++++++++++++++++-----------------------
    > 1 file changed, 39 insertions(+), 34 deletions(-)
    >
    > diff --git a/drivers/md/md.c b/drivers/md/md.c
    > index 0cb9fa703a0c..561cac13ff96 100644
    > --- a/drivers/md/md.c
    > +++ b/drivers/md/md.c
    > @@ -9293,6 +9293,22 @@ static bool md_choose_sync_action(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_action(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"));
    > @@ -9300,20 +9316,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);
    > }
    >
    > /*
    > @@ -9381,7 +9404,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)
    > @@ -9468,31 +9490,14 @@ void md_check_recovery(struct mddev *mddev)
    > clear_bit(MD_RECOVERY_INTR, &mddev->recovery);
    > 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_action(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);
    > - }
    > + 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);
    > - 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);
    > mddev_unlock(mddev);
    > --
    > 2.39.2
    >

    I like the idea. Thanks for the effort.

    Reviewed-by: Xiao Ni <xni@redhat.com>

    \
     
     \ /
      Last update: 2023-08-22 12:08    [W:4.258 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site