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] [day] [month] [year] [list]
Date:   Fri, 24 Nov 2023 09:59:46 +0800
From:   Yu Kuai <yukuai1@...weicloud.com>
To:     Yu Kuai <yukuai1@...weicloud.com>, Xiao Ni <xni@...hat.com>
Cc:     song@...nel.org, neilb@...e.de, linux-kernel@...r.kernel.org,
        linux-raid@...r.kernel.org, yi.zhang@...wei.com,
        yangerkun@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next 6/8] md: factor out a helper to stop sync_thread

Hi,

在 2023/11/21 21:01, Yu Kuai 写道:
>>>
>>> It looks like the three roles (md_set_readonly, do_md_stop and
>>> stop_sync_thread) need to wait for different events. We can move these
>>> codes out this helper function and make this helper function to be
>>> more common.
>>
>> Or get lock again before returning this function and leave the wait here?

How about following patch?

  drivers/md/md.c | 89 
+++++++++++++++++++++++++++++++++++------------------------------------------------------
  1 file changed, 35 insertions(+), 54 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 78f32a2e434c..fe46b67f6e87 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4868,35 +4868,18 @@ action_show(struct mddev *mddev, char *page)
         return sprintf(page, "%s\n", action_names[get_sync_action(mddev)]);
  }

-static int stop_sync_thread(struct mddev *mddev)
+static void prepare_to_stop_sync_thread(struct mddev *mddev)
+       __releases(&mddev->reconfig_mutex)
  {
-       int err = mddev_lock(mddev);
-
-       if (err)
-               return err;
-
-       /*
-        * Check again in case MD_RECOVERY_RUNNING is cleared before lock is
-        * held.
-        */
-       if (!test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
-               mddev_unlock(mddev);
-               return 0;
-       }
-
-       if (work_pending(&mddev->sync_work))
-               flush_workqueue(md_misc_wq);
-
         set_bit(MD_RECOVERY_INTR, &mddev->recovery);
         /*
-        * Thread might be blocked waiting for metadata update which 
will now
-        * never happen
+        * Thread might be blocked waiting for metadata update which
+        * will now never happen.
          */
         md_wakeup_thread_directly(mddev->sync_thread);
-
         mddev_unlock(mddev);
-
-       return 0;
+       if (work_pending(&mddev->sync_work))
+               flush_work(&mddev->sync_work);
  }

  static int idle_sync_thread(struct mddev *mddev)
@@ -4905,13 +4888,17 @@ static int idle_sync_thread(struct mddev *mddev)
         int err;

         clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
-       err = stop_sync_thread(mddev);
+       err = mddev_lock(mddev);
         if (err)
                 return err;

-       err = wait_event_interruptible(resync_wait,
+       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
+               prepare_to_stop_sync_thread(mddev);
+               err = wait_event_interruptible(resync_wait,
                         sync_seq != atomic_read(&mddev->sync_seq) ||
                         !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery));
+       }
+
         return err;
  }

@@ -4920,12 +4907,15 @@ static int frozen_sync_thread(struct mddev *mddev)
         int err;

         set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
-       err = stop_sync_thread(mddev);
+       err = mddev_lock(mddev);
         if (err)
                 return err;

-       err = wait_event_interruptible(resync_wait,
+       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
+               prepare_to_stop_sync_thread(mddev);
+               err = wait_event_interruptible(resync_wait,
                         !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery));
+       }

         return err;
  }
@@ -6350,11 +6340,11 @@ static void md_clean(struct mddev *mddev)
  static void __md_stop_writes(struct mddev *mddev)
  {
         set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
-       if (work_pending(&mddev->sync_work))
-               flush_workqueue(md_misc_wq);
-       if (mddev->sync_thread) {
-               set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-               md_reap_sync_thread(mddev);
+       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
+               prepare_to_stop_sync_thread(mddev);
+               wait_event(resync_wait, !test_bit(MD_RECOVERY_RUNNING,
+                                                 &mddev->recovery));
+               mddev_lock_nointr(mddev);
         }

         del_timer_sync(&mddev->safemode_timer);
@@ -6447,18 +6437,15 @@ static int md_set_readonly(struct mddev *mddev, 
struct block_device *bdev)
                 did_freeze = 1;
                 set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
         }
-       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
-               set_bit(MD_RECOVERY_INTR, &mddev->recovery);

-       /*
-        * Thread might be blocked waiting for metadata update which 
will now
-        * never happen
-        */
-       md_wakeup_thread_directly(mddev->sync_thread);
+       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
+               prepare_to_stop_sync_thread(mddev);
+               wait_event(resync_wait, !test_bit(MD_RECOVERY_RUNNING,
+                                                 &mddev->recovery));
+       } else {
+               mddev_unlock(mddev);
+       }

-       mddev_unlock(mddev);
-       wait_event(resync_wait, !test_bit(MD_RECOVERY_RUNNING,
-                                         &mddev->recovery));
         wait_event(mddev->sb_wait,
                    !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags));
         mddev_lock_nointr(mddev);
@@ -6509,19 +6496,13 @@ static int do_md_stop(struct mddev *mddev, int mode,
                 did_freeze = 1;
                 set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
         }
-       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
-               set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-
-       /*
-        * Thread might be blocked waiting for metadata update which 
will now
-        * never happen
-        */
-       md_wakeup_thread_directly(mddev->sync_thread);

-       mddev_unlock(mddev);
-       wait_event(resync_wait, !test_bit(MD_RECOVERY_RUNNING,
-                                         &mddev->recovery));
-       mddev_lock_nointr(mddev);
+       if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
+               prepare_to_stop_sync_thread(mddev);
+               wait_event(resync_wait, !test_bit(MD_RECOVERY_RUNNING,
+                                                 &mddev->recovery));
+               mddev_lock_nointr(mddev);
+       }

         mutex_lock(&mddev->open_mutex);
         if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) ||

Powered by blists - more mailing lists