diff mbox series

[-next,v3,3/7] md: delay choosing sync action to md_start_sync()

Message ID 20230820090949.2874537-4-yukuai1@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series md: make rdev addition and removal independent from daemon thread | expand

Commit Message

Yu Kuai Aug. 20, 2023, 9:09 a.m. UTC 
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(-)

Comments

Xiao Ni Aug. 22, 2023, 10:06 a.m. UTC | #1 
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>
diff mbox series

Patch

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);