All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread
@ 2023-08-20  9:09 Yu Kuai
  2023-08-20  9:09 ` [PATCH -next v3 1/7] md: use separate work_struct for md_start_sync() Yu Kuai
                   ` (6 more replies)
  0 siblings, 7 replies; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

From: Yu Kuai <yukuai3@huawei.com>

Changes in v3:
 - rename md_choose_sync_direction() to md_choose_sync_action() in patch 2;
 - fix an error in patch 3;
 - add flush_work(&mddev->sync_work) while change read-only array to
 read-write;

Changes in v2:
 - remove patch 1 from v1 and some related patches, those patches will
 be sent later when rcu protection for rdev is removed.
 - add patch 2.

This is the third patchset to do some preparatory work to synchronize
io with array reconfiguration.

1) The first patchset refactor 'active_io', make sure that mddev_suspend()
will wait for io to be done. [1]

2) The second patchset remove 'quiesce' callback from mddev_suspend(), so
that mddev_suspend() doesn't rely on 'quiesce' callback is registered,
and can be used for all personalites; [2]

3) This patchset make array reconfiguration independent from daemon thread,
and synchronize it with io will be much easier because io may rely on
daemon thread to be done.

More patchset on the way!

[1] https://lore.kernel.org/all/20230621165110.1498313-1-yukuai1@huaweicloud.com/
[2] https://lore.kernel.org/all/20230628012931.88911-2-yukuai1@huaweicloud.com/

Yu Kuai (7):
  md: use separate work_struct for md_start_sync()
  md: factor out a helper to choose sync action from md_check_recovery()
  md: delay choosing sync action to md_start_sync()
  md: factor out a helper rdev_removeable() from remove_and_add_spares()
  md: factor out a helper rdev_is_spare() from remove_and_add_spares()
  md: factor out a helper rdev_addable() from remove_and_add_spares()
  md: delay remove_and_add_spares() for read only array to
    md_start_sync()

 drivers/md/md.c | 268 ++++++++++++++++++++++++++++++------------------
 drivers/md/md.h |   5 +-
 2 files changed, 170 insertions(+), 103 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH -next v3 1/7] md: use separate work_struct for md_start_sync()
  2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
@ 2023-08-20  9:09 ` Yu Kuai
  2023-08-22 10:04   ` Xiao Ni
  2023-08-20  9:09 ` [PATCH -next v3 2/7] md: factor out a helper to choose sync action from md_check_recovery() Yu Kuai
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

From: Yu Kuai <yukuai3@huawei.com>

It's a little weird to borrow 'del_work' for md_start_sync(), declare
a new work_struct 'sync_work' for md_start_sync().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 10 ++++++----
 drivers/md/md.h |  5 ++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 5c3c19b8d509..90815be1e80f 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -631,13 +631,13 @@ void mddev_put(struct mddev *mddev)
 		 * flush_workqueue() after mddev_find will succeed in waiting
 		 * for the work to be done.
 		 */
-		INIT_WORK(&mddev->del_work, mddev_delayed_delete);
 		queue_work(md_misc_wq, &mddev->del_work);
 	}
 	spin_unlock(&all_mddevs_lock);
 }
 
 static void md_safemode_timeout(struct timer_list *t);
+static void md_start_sync(struct work_struct *ws);
 
 void mddev_init(struct mddev *mddev)
 {
@@ -662,6 +662,9 @@ void mddev_init(struct mddev *mddev)
 	mddev->resync_min = 0;
 	mddev->resync_max = MaxSector;
 	mddev->level = LEVEL_NONE;
+
+	INIT_WORK(&mddev->sync_work, md_start_sync);
+	INIT_WORK(&mddev->del_work, mddev_delayed_delete);
 }
 EXPORT_SYMBOL_GPL(mddev_init);
 
@@ -9245,7 +9248,7 @@ static int remove_and_add_spares(struct mddev *mddev,
 
 static void md_start_sync(struct work_struct *ws)
 {
-	struct mddev *mddev = container_of(ws, struct mddev, del_work);
+	struct mddev *mddev = container_of(ws, struct mddev, sync_work);
 
 	rcu_assign_pointer(mddev->sync_thread,
 			   md_register_thread(md_do_sync, mddev, "resync"));
@@ -9458,8 +9461,7 @@ void md_check_recovery(struct mddev *mddev)
 				 */
 				md_bitmap_write_all(mddev->bitmap);
 			}
-			INIT_WORK(&mddev->del_work, md_start_sync);
-			queue_work(md_misc_wq, &mddev->del_work);
+			queue_work(md_misc_wq, &mddev->sync_work);
 			goto unlock;
 		}
 	not_running:
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 9bcb77bca963..64d05cb65287 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -450,7 +450,10 @@ struct mddev {
 	struct kernfs_node		*sysfs_degraded;	/*handle for 'degraded' */
 	struct kernfs_node		*sysfs_level;		/*handle for 'level' */
 
-	struct work_struct del_work;	/* used for delayed sysfs removal */
+	/* used for delayed sysfs removal */
+	struct work_struct del_work;
+	/* used for register new sync thread */
+	struct work_struct sync_work;
 
 	/* "lock" protects:
 	 *   flush_bio transition from NULL to !NULL
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH -next v3 2/7] md: factor out a helper to choose sync action from md_check_recovery()
  2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
  2023-08-20  9:09 ` [PATCH -next v3 1/7] md: use separate work_struct for md_start_sync() Yu Kuai
@ 2023-08-20  9:09 ` Yu Kuai
  2023-08-22 10:04   ` Xiao Ni
  2023-08-20  9:09 ` [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync() Yu Kuai
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

From: Yu Kuai <yukuai3@huawei.com>

There are no functional changes, on the one hand make the code cleaner,
on the other hand prevent following checkpatch error in the next patch to
delay choosing sync action to md_start_sync().

ERROR: do not use assignment in if condition
+       } else if ((spares = remove_and_add_spares(mddev, NULL))) {

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 70 +++++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 25 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 90815be1e80f..0cb9fa703a0c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9246,6 +9246,50 @@ static int remove_and_add_spares(struct mddev *mddev,
 	return spares;
 }
 
+static bool md_choose_sync_action(struct mddev *mddev, int *spares)
+{
+	/* Check if reshape is in progress first. */
+	if (mddev->reshape_position != MaxSector) {
+		if (mddev->pers->check_reshape == NULL ||
+		    mddev->pers->check_reshape(mddev) != 0)
+			return false;
+
+		set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
+		clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
+		return true;
+	}
+
+	/*
+	 * Remove any failed drives, then add spares if possible. Spares are
+	 * also removed and re-added, to allow the personality to fail the
+	 * re-add.
+	 */
+	*spares = remove_and_add_spares(mddev, NULL);
+	if (*spares) {
+		clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
+		clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
+		clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
+
+		/* Start new recovery. */
+		set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
+		return true;
+	}
+
+	/* Check if recovery is in progress. */
+	if (mddev->recovery_cp < MaxSector) {
+		set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
+		clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
+		return true;
+	}
+
+	/* Delay to choose resync/check/repair in md_do_sync(). */
+	if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
+		return true;
+
+	/* Nothing to be done */
+	return false;
+}
+
 static void md_start_sync(struct work_struct *ws)
 {
 	struct mddev *mddev = container_of(ws, struct mddev, sync_work);
@@ -9427,32 +9471,8 @@ void md_check_recovery(struct mddev *mddev)
 		if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
 		    test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
 			goto not_running;
-		/* no recovery is running.
-		 * remove any failed drives, then
-		 * add spares if possible.
-		 * Spares are also removed and re-added, to allow
-		 * the personality to fail the re-add.
-		 */
-
-		if (mddev->reshape_position != MaxSector) {
-			if (mddev->pers->check_reshape == NULL ||
-			    mddev->pers->check_reshape(mddev) != 0)
-				/* Cannot proceed */
-				goto not_running;
-			set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
-			clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
-		} else if ((spares = remove_and_add_spares(mddev, NULL))) {
-			clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
-			clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
-			clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
-			set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
-		} else if (mddev->recovery_cp < MaxSector) {
-			set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
-			clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
-		} else if (!test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
-			/* nothing to be done ... */
+		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
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync()
  2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
  2023-08-20  9:09 ` [PATCH -next v3 1/7] md: use separate work_struct for md_start_sync() Yu Kuai
  2023-08-20  9:09 ` [PATCH -next v3 2/7] md: factor out a helper to choose sync action from md_check_recovery() Yu Kuai
@ 2023-08-20  9:09 ` Yu Kuai
  2023-08-22 10:06   ` Xiao Ni
  2023-08-20  9:09 ` [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares() Yu Kuai
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

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


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares()
  2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
                   ` (2 preceding siblings ...)
  2023-08-20  9:09 ` [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync() Yu Kuai
@ 2023-08-20  9:09 ` Yu Kuai
  2023-08-22 10:19   ` Xiao Ni
  2023-08-20  9:09 ` [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() " Yu Kuai
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

From: Yu Kuai <yukuai3@huawei.com>

There are no functional changes, just to make the code simpler and
prepare to delay remove_and_add_spares() to md_start_sync().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 561cac13ff96..ceace5ffadd6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9153,6 +9153,22 @@ void md_do_sync(struct md_thread *thread)
 }
 EXPORT_SYMBOL_GPL(md_do_sync);
 
+static bool rdev_removeable(struct md_rdev *rdev)
+{
+	if (rdev->raid_disk < 0 || test_bit(Blocked, &rdev->flags) ||
+	    atomic_read(&rdev->nr_pending))
+		return false;
+
+	if (test_bit(RemoveSynchronized, &rdev->flags))
+		return true;
+
+	if (test_bit(In_sync, &rdev->flags) ||
+	    test_bit(Journal, &rdev->flags))
+		return false;
+
+	return true;
+}
+
 static int remove_and_add_spares(struct mddev *mddev,
 				 struct md_rdev *this)
 {
@@ -9166,11 +9182,7 @@ static int remove_and_add_spares(struct mddev *mddev,
 		return 0;
 
 	rdev_for_each(rdev, mddev) {
-		if ((this == NULL || rdev == this) &&
-		    rdev->raid_disk >= 0 &&
-		    !test_bit(Blocked, &rdev->flags) &&
-		    test_bit(Faulty, &rdev->flags) &&
-		    atomic_read(&rdev->nr_pending)==0) {
+		if ((this == NULL || rdev == this) && rdev_removeable(rdev)) {
 			/* Faulty non-Blocked devices with nr_pending == 0
 			 * never get nr_pending incremented,
 			 * never get Faulty cleared, and never get Blocked set.
@@ -9185,19 +9197,12 @@ static int remove_and_add_spares(struct mddev *mddev,
 		synchronize_rcu();
 	rdev_for_each(rdev, mddev) {
 		if ((this == NULL || rdev == this) &&
-		    rdev->raid_disk >= 0 &&
-		    !test_bit(Blocked, &rdev->flags) &&
-		    ((test_bit(RemoveSynchronized, &rdev->flags) ||
-		     (!test_bit(In_sync, &rdev->flags) &&
-		      !test_bit(Journal, &rdev->flags))) &&
-		    atomic_read(&rdev->nr_pending)==0)) {
-			if (mddev->pers->hot_remove_disk(
-				    mddev, rdev) == 0) {
+		    rdev_removeable(rdev) &&
+		    mddev->pers->hot_remove_disk(mddev, rdev) == 0) {
 				sysfs_unlink_rdev(mddev, rdev);
 				rdev->saved_raid_disk = rdev->raid_disk;
 				rdev->raid_disk = -1;
 				removed++;
-			}
 		}
 		if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
 			clear_bit(RemoveSynchronized, &rdev->flags);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() from remove_and_add_spares()
  2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
                   ` (3 preceding siblings ...)
  2023-08-20  9:09 ` [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares() Yu Kuai
@ 2023-08-20  9:09 ` Yu Kuai
  2023-08-23  2:20   ` Xiao Ni
  2023-08-20  9:09 ` [PATCH -next v3 6/7] md: factor out a helper rdev_addable() " Yu Kuai
  2023-08-20  9:09 ` [PATCH -next v3 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync() Yu Kuai
  6 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

From: Yu Kuai <yukuai3@huawei.com>

There are no functional changes, just to make the code simpler and
prepare to delay remove_and_add_spares() to md_start_sync().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index ceace5ffadd6..11d27c934fdd 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9169,6 +9169,14 @@ static bool rdev_removeable(struct md_rdev *rdev)
 	return true;
 }
 
+static bool rdev_is_spare(struct md_rdev *rdev)
+{
+	return !test_bit(Candidate, &rdev->flags) && rdev->raid_disk >= 0 &&
+	       !test_bit(In_sync, &rdev->flags) &&
+	       !test_bit(Journal, &rdev->flags) &&
+	       !test_bit(Faulty, &rdev->flags);
+}
+
 static int remove_and_add_spares(struct mddev *mddev,
 				 struct md_rdev *this)
 {
@@ -9217,13 +9225,10 @@ static int remove_and_add_spares(struct mddev *mddev,
 	rdev_for_each(rdev, mddev) {
 		if (this && this != rdev)
 			continue;
+		if (rdev_is_spare(rdev))
+			spares++;
 		if (test_bit(Candidate, &rdev->flags))
 			continue;
-		if (rdev->raid_disk >= 0 &&
-		    !test_bit(In_sync, &rdev->flags) &&
-		    !test_bit(Journal, &rdev->flags) &&
-		    !test_bit(Faulty, &rdev->flags))
-			spares++;
 		if (rdev->raid_disk >= 0)
 			continue;
 		if (test_bit(Faulty, &rdev->flags))
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
                   ` (4 preceding siblings ...)
  2023-08-20  9:09 ` [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() " Yu Kuai
@ 2023-08-20  9:09 ` Yu Kuai
  2023-08-21 23:22   ` Song Liu
  2023-08-20  9:09 ` [PATCH -next v3 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync() Yu Kuai
  6 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

From: Yu Kuai <yukuai3@huawei.com>

There are no functional changes, just to make the code simpler and
prepare to delay remove_and_add_spares() to md_start_sync().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 11d27c934fdd..cdc361c521d4 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9177,6 +9177,20 @@ static bool rdev_is_spare(struct md_rdev *rdev)
 	       !test_bit(Faulty, &rdev->flags);
 }
 
+static bool rdev_addable(struct md_rdev *rdev)
+{
+	if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
+	    test_bit(Faulty, &rdev->flags))
+		return false;
+
+	if (!test_bit(Journal, &rdev->flags) && !md_is_rdwr(rdev->mddev) &&
+	    !(rdev->saved_raid_disk >= 0 &&
+	      !test_bit(Bitmap_sync, &rdev->flags)))
+		return false;
+
+	return true;
+}
+
 static int remove_and_add_spares(struct mddev *mddev,
 				 struct md_rdev *this)
 {
@@ -9227,20 +9241,10 @@ static int remove_and_add_spares(struct mddev *mddev,
 			continue;
 		if (rdev_is_spare(rdev))
 			spares++;
-		if (test_bit(Candidate, &rdev->flags))
+		if (!rdev_addable(rdev))
 			continue;
-		if (rdev->raid_disk >= 0)
-			continue;
-		if (test_bit(Faulty, &rdev->flags))
-			continue;
-		if (!test_bit(Journal, &rdev->flags)) {
-			if (!md_is_rdwr(mddev) &&
-			    !(rdev->saved_raid_disk >= 0 &&
-			      !test_bit(Bitmap_sync, &rdev->flags)))
-				continue;
-
+		if (!test_bit(Journal, &rdev->flags))
 			rdev->recovery_offset = 0;
-		}
 		if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
 			/* failure here is OK */
 			sysfs_link_rdev(mddev, rdev);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH -next v3 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync()
  2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
                   ` (5 preceding siblings ...)
  2023-08-20  9:09 ` [PATCH -next v3 6/7] md: factor out a helper rdev_addable() " Yu Kuai
@ 2023-08-20  9:09 ` Yu Kuai
  2023-08-23  3:24   ` Xiao Ni
  6 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-20  9:09 UTC (permalink / raw)
  To: song, xni, mariusz.tkaczyk
  Cc: linux-raid, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun

From: Yu Kuai <yukuai3@huawei.com>

Before this patch, for read-only array:

md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, then it will
call remove_and_add_spares() directly to try to remove and add rdevs
from array.

After this patch:

1) md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, and the
   worker 'sync_work' is not pending, and there are rdevs can be added
   or removed, then it will queue new work md_start_sync();
2) md_start_sync() will call remove_and_add_spares() and exist;

This change make sure that array reconfiguration is independent from
daemon, and it'll be much easier to synchronize it with io, consier
that io may rely on daemon thread to be done.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index cdc361c521d4..f08b683f724f 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4866,6 +4866,7 @@ action_store(struct mddev *mddev, const char *page, size_t len)
 		/* A write to sync_action is enough to justify
 		 * canceling read-auto mode
 		 */
+		flush_work(&mddev->sync_work);
 		mddev->ro = MD_RDWR;
 		md_wakeup_thread(mddev->sync_thread);
 	}
@@ -7638,6 +7639,10 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
 		mutex_unlock(&mddev->open_mutex);
 		sync_blockdev(bdev);
 	}
+
+	if (!md_is_rdwr(mddev))
+		flush_work(&mddev->sync_work);
+
 	err = mddev_lock(mddev);
 	if (err) {
 		pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n",
@@ -8562,6 +8567,7 @@ bool md_write_start(struct mddev *mddev, struct bio *bi)
 	BUG_ON(mddev->ro == MD_RDONLY);
 	if (mddev->ro == MD_AUTO_READ) {
 		/* need to switch to read/write */
+		flush_work(&mddev->sync_work);
 		mddev->ro = MD_RDWR;
 		set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 		md_wakeup_thread(mddev->thread);
@@ -9191,6 +9197,16 @@ static bool rdev_addable(struct md_rdev *rdev)
 	return true;
 }
 
+static bool md_spares_need_change(struct mddev *mddev)
+{
+	struct md_rdev *rdev;
+
+	rdev_for_each(rdev, mddev)
+		if (rdev_removeable(rdev) || rdev_addable(rdev))
+			return true;
+	return false;
+}
+
 static int remove_and_add_spares(struct mddev *mddev,
 				 struct md_rdev *this)
 {
@@ -9311,6 +9327,12 @@ static void md_start_sync(struct work_struct *ws)
 
 	mddev_lock_nointr(mddev);
 
+	if (!md_is_rdwr(mddev)) {
+		remove_and_add_spares(mddev, NULL);
+		mddev_unlock(mddev);
+		return;
+	}
+
 	if (!md_choose_sync_action(mddev, &spares))
 		goto not_running;
 
@@ -9405,7 +9427,8 @@ void md_check_recovery(struct mddev *mddev)
 	}
 
 	if (!md_is_rdwr(mddev) &&
-	    !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery))
+	    (!test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
+	     work_pending(&mddev->sync_work)))
 		return;
 	if ( ! (
 		(mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
@@ -9433,15 +9456,8 @@ void md_check_recovery(struct mddev *mddev)
 				 */
 				rdev_for_each(rdev, mddev)
 					clear_bit(Blocked, &rdev->flags);
-			/* On a read-only array we can:
-			 * - remove failed devices
-			 * - add already-in_sync devices if the array itself
-			 *   is in-sync.
-			 * As we only add devices that are already in-sync,
-			 * we can activate the spares immediately.
-			 */
-			remove_and_add_spares(mddev, NULL);
-			/* There is no thread, but we need to call
+			/*
+			 * There is no thread, but we need to call
 			 * ->spare_active and clear saved_raid_disk
 			 */
 			set_bit(MD_RECOVERY_INTR, &mddev->recovery);
@@ -9449,6 +9465,13 @@ void md_check_recovery(struct mddev *mddev)
 			clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
 			clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
 			clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);
+
+			/*
+			 * Let md_start_sync() to remove and add rdevs to the
+			 * array.
+			 */
+			if (md_spares_need_change(mddev))
+				queue_work(md_misc_wq, &mddev->sync_work);
 			goto unlock;
 		}
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-20  9:09 ` [PATCH -next v3 6/7] md: factor out a helper rdev_addable() " Yu Kuai
@ 2023-08-21 23:22   ` Song Liu
  2023-08-22  2:17     ` Yu Kuai
  0 siblings, 1 reply; 25+ messages in thread
From: Song Liu @ 2023-08-21 23:22 UTC (permalink / raw)
  To: Yu Kuai
  Cc: xni, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

On Sun, Aug 20, 2023 at 2:13 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> There are no functional changes, just to make the code simpler and
> prepare to delay remove_and_add_spares() to md_start_sync().
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 11d27c934fdd..cdc361c521d4 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9177,6 +9177,20 @@ static bool rdev_is_spare(struct md_rdev *rdev)
>                !test_bit(Faulty, &rdev->flags);
>  }
>
> +static bool rdev_addable(struct md_rdev *rdev)
> +{
> +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
> +           test_bit(Faulty, &rdev->flags))
> +               return false;
> +
> +       if (!test_bit(Journal, &rdev->flags) && !md_is_rdwr(rdev->mddev) &&

Instead of straightforward refactoring, I hope we can make these rdev_*
helpers more meaningful, and hopefullly reusable. For example, let's define
the meaning of "addable", and write the function to match that meaning. In
this case, I think we shouldn't check md_is_rdwr() inside rdev_addable().

Does this make sense?

Thanks,
Song


> +           !(rdev->saved_raid_disk >= 0 &&
> +             !test_bit(Bitmap_sync, &rdev->flags)))
> +               return false;
> +
> +       return true;
> +}
> +
>  static int remove_and_add_spares(struct mddev *mddev,
>                                  struct md_rdev *this)
>  {
> @@ -9227,20 +9241,10 @@ static int remove_and_add_spares(struct mddev *mddev,
>                         continue;
>                 if (rdev_is_spare(rdev))
>                         spares++;
> -               if (test_bit(Candidate, &rdev->flags))
> +               if (!rdev_addable(rdev))
>                         continue;
> -               if (rdev->raid_disk >= 0)
> -                       continue;
> -               if (test_bit(Faulty, &rdev->flags))
> -                       continue;
> -               if (!test_bit(Journal, &rdev->flags)) {
> -                       if (!md_is_rdwr(mddev) &&
> -                           !(rdev->saved_raid_disk >= 0 &&
> -                             !test_bit(Bitmap_sync, &rdev->flags)))
> -                               continue;
> -
> +               if (!test_bit(Journal, &rdev->flags))
>                         rdev->recovery_offset = 0;
> -               }
>                 if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
>                         /* failure here is OK */
>                         sysfs_link_rdev(mddev, rdev);
> --
> 2.39.2
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-21 23:22   ` Song Liu
@ 2023-08-22  2:17     ` Yu Kuai
  2023-08-23  3:04       ` Yu Kuai
  0 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-22  2:17 UTC (permalink / raw)
  To: Song Liu, Yu Kuai
  Cc: xni, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

Hi,

在 2023/08/22 7:22, Song Liu 写道:
> On Sun, Aug 20, 2023 at 2:13 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> There are no functional changes, just to make the code simpler and
>> prepare to delay remove_and_add_spares() to md_start_sync().
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/md.c | 28 ++++++++++++++++------------
>>   1 file changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 11d27c934fdd..cdc361c521d4 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9177,6 +9177,20 @@ static bool rdev_is_spare(struct md_rdev *rdev)
>>                 !test_bit(Faulty, &rdev->flags);
>>   }
>>
>> +static bool rdev_addable(struct md_rdev *rdev)
>> +{
>> +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
>> +           test_bit(Faulty, &rdev->flags))
>> +               return false;
>> +
>> +       if (!test_bit(Journal, &rdev->flags) && !md_is_rdwr(rdev->mddev) &&
> 
> Instead of straightforward refactoring, I hope we can make these rdev_*
> helpers more meaningful, and hopefullly reusable. For example, let's define
> the meaning of "addable", and write the function to match that meaning. In
> this case, I think we shouldn't check md_is_rdwr() inside rdev_addable().
> 
> Does this make sense?

Yes, this make sense, rdev can be added to read-only array.

There are total three callers of pers->hot_add_sisk, I'll try to find if
they have common conditions.

Thanks,
Kuai

> 
> Thanks,
> Song
> 
> 
>> +           !(rdev->saved_raid_disk >= 0 &&
>> +             !test_bit(Bitmap_sync, &rdev->flags)))
>> +               return false;
>> +
>> +       return true;
>> +}
>> +
>>   static int remove_and_add_spares(struct mddev *mddev,
>>                                   struct md_rdev *this)
>>   {
>> @@ -9227,20 +9241,10 @@ static int remove_and_add_spares(struct mddev *mddev,
>>                          continue;
>>                  if (rdev_is_spare(rdev))
>>                          spares++;
>> -               if (test_bit(Candidate, &rdev->flags))
>> +               if (!rdev_addable(rdev))
>>                          continue;
>> -               if (rdev->raid_disk >= 0)
>> -                       continue;
>> -               if (test_bit(Faulty, &rdev->flags))
>> -                       continue;
>> -               if (!test_bit(Journal, &rdev->flags)) {
>> -                       if (!md_is_rdwr(mddev) &&
>> -                           !(rdev->saved_raid_disk >= 0 &&
>> -                             !test_bit(Bitmap_sync, &rdev->flags)))
>> -                               continue;
>> -
>> +               if (!test_bit(Journal, &rdev->flags))
>>                          rdev->recovery_offset = 0;
>> -               }
>>                  if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
>>                          /* failure here is OK */
>>                          sysfs_link_rdev(mddev, rdev);
>> --
>> 2.39.2
>>
> .
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 1/7] md: use separate work_struct for md_start_sync()
  2023-08-20  9:09 ` [PATCH -next v3 1/7] md: use separate work_struct for md_start_sync() Yu Kuai
@ 2023-08-22 10:04   ` Xiao Ni
  0 siblings, 0 replies; 25+ messages in thread
From: Xiao Ni @ 2023-08-22 10:04 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> It's a little weird to borrow 'del_work' for md_start_sync(), declare
> a new work_struct 'sync_work' for md_start_sync().
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 10 ++++++----
>  drivers/md/md.h |  5 ++++-
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 5c3c19b8d509..90815be1e80f 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -631,13 +631,13 @@ void mddev_put(struct mddev *mddev)
>                  * flush_workqueue() after mddev_find will succeed in waiting
>                  * for the work to be done.
>                  */
> -               INIT_WORK(&mddev->del_work, mddev_delayed_delete);
>                 queue_work(md_misc_wq, &mddev->del_work);
>         }
>         spin_unlock(&all_mddevs_lock);
>  }
>
>  static void md_safemode_timeout(struct timer_list *t);
> +static void md_start_sync(struct work_struct *ws);
>
>  void mddev_init(struct mddev *mddev)
>  {
> @@ -662,6 +662,9 @@ void mddev_init(struct mddev *mddev)
>         mddev->resync_min = 0;
>         mddev->resync_max = MaxSector;
>         mddev->level = LEVEL_NONE;
> +
> +       INIT_WORK(&mddev->sync_work, md_start_sync);
> +       INIT_WORK(&mddev->del_work, mddev_delayed_delete);
>  }
>  EXPORT_SYMBOL_GPL(mddev_init);
>
> @@ -9245,7 +9248,7 @@ static int remove_and_add_spares(struct mddev *mddev,
>
>  static void md_start_sync(struct work_struct *ws)
>  {
> -       struct mddev *mddev = container_of(ws, struct mddev, del_work);
> +       struct mddev *mddev = container_of(ws, struct mddev, sync_work);
>
>         rcu_assign_pointer(mddev->sync_thread,
>                            md_register_thread(md_do_sync, mddev, "resync"));
> @@ -9458,8 +9461,7 @@ void md_check_recovery(struct mddev *mddev)
>                                  */
>                                 md_bitmap_write_all(mddev->bitmap);
>                         }
> -                       INIT_WORK(&mddev->del_work, md_start_sync);
> -                       queue_work(md_misc_wq, &mddev->del_work);
> +                       queue_work(md_misc_wq, &mddev->sync_work);
>                         goto unlock;
>                 }
>         not_running:
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 9bcb77bca963..64d05cb65287 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -450,7 +450,10 @@ struct mddev {
>         struct kernfs_node              *sysfs_degraded;        /*handle for 'degraded' */
>         struct kernfs_node              *sysfs_level;           /*handle for 'level' */
>
> -       struct work_struct del_work;    /* used for delayed sysfs removal */
> +       /* used for delayed sysfs removal */
> +       struct work_struct del_work;
> +       /* used for register new sync thread */
> +       struct work_struct sync_work;
>
>         /* "lock" protects:
>          *   flush_bio transition from NULL to !NULL
> --
> 2.39.2
>

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 2/7] md: factor out a helper to choose sync action from md_check_recovery()
  2023-08-20  9:09 ` [PATCH -next v3 2/7] md: factor out a helper to choose sync action from md_check_recovery() Yu Kuai
@ 2023-08-22 10:04   ` Xiao Ni
  0 siblings, 0 replies; 25+ messages in thread
From: Xiao Ni @ 2023-08-22 10:04 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> There are no functional changes, on the one hand make the code cleaner,
> on the other hand prevent following checkpatch error in the next patch to
> delay choosing sync action to md_start_sync().
>
> ERROR: do not use assignment in if condition
> +       } else if ((spares = remove_and_add_spares(mddev, NULL))) {
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 70 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 90815be1e80f..0cb9fa703a0c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9246,6 +9246,50 @@ static int remove_and_add_spares(struct mddev *mddev,
>         return spares;
>  }
>
> +static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> +{
> +       /* Check if reshape is in progress first. */
> +       if (mddev->reshape_position != MaxSector) {
> +               if (mddev->pers->check_reshape == NULL ||
> +                   mddev->pers->check_reshape(mddev) != 0)
> +                       return false;
> +
> +               set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> +               return true;
> +       }
> +
> +       /*
> +        * Remove any failed drives, then add spares if possible. Spares are
> +        * also removed and re-added, to allow the personality to fail the
> +        * re-add.
> +        */
> +       *spares = remove_and_add_spares(mddev, NULL);
> +       if (*spares) {
> +               clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
> +
> +               /* Start new recovery. */
> +               set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> +               return true;
> +       }
> +
> +       /* Check if recovery is in progress. */
> +       if (mddev->recovery_cp < MaxSector) {
> +               set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> +               return true;
> +       }
> +
> +       /* Delay to choose resync/check/repair in md_do_sync(). */
> +       if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
> +               return true;
> +
> +       /* Nothing to be done */
> +       return false;
> +}
> +
>  static void md_start_sync(struct work_struct *ws)
>  {
>         struct mddev *mddev = container_of(ws, struct mddev, sync_work);
> @@ -9427,32 +9471,8 @@ void md_check_recovery(struct mddev *mddev)
>                 if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>                     test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
>                         goto not_running;
> -               /* no recovery is running.
> -                * remove any failed drives, then
> -                * add spares if possible.
> -                * Spares are also removed and re-added, to allow
> -                * the personality to fail the re-add.
> -                */
> -
> -               if (mddev->reshape_position != MaxSector) {
> -                       if (mddev->pers->check_reshape == NULL ||
> -                           mddev->pers->check_reshape(mddev) != 0)
> -                               /* Cannot proceed */
> -                               goto not_running;
> -                       set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> -               } else if ((spares = remove_and_add_spares(mddev, NULL))) {
> -                       clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
> -                       set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> -               } else if (mddev->recovery_cp < MaxSector) {
> -                       set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> -               } else if (!test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
> -                       /* nothing to be done ... */
> +               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
> --
> 2.39.2
>

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync()
  2023-08-20  9:09 ` [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync() Yu Kuai
@ 2023-08-22 10:06   ` Xiao Ni
  0 siblings, 0 replies; 25+ messages in thread
From: Xiao Ni @ 2023-08-22 10:06 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

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>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares()
  2023-08-20  9:09 ` [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares() Yu Kuai
@ 2023-08-22 10:19   ` Xiao Ni
  2023-08-23  2:45     ` Yu Kuai
  0 siblings, 1 reply; 25+ messages in thread
From: Xiao Ni @ 2023-08-22 10:19 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> There are no functional changes, just to make the code simpler and
> prepare to delay remove_and_add_spares() to md_start_sync().
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 561cac13ff96..ceace5ffadd6 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9153,6 +9153,22 @@ void md_do_sync(struct md_thread *thread)
>  }
>  EXPORT_SYMBOL_GPL(md_do_sync);
>
> +static bool rdev_removeable(struct md_rdev *rdev)
> +{
> +       if (rdev->raid_disk < 0 || test_bit(Blocked, &rdev->flags) ||
> +           atomic_read(&rdev->nr_pending))
> +               return false;
> +
> +       if (test_bit(RemoveSynchronized, &rdev->flags))
> +               return true;
> +
> +       if (test_bit(In_sync, &rdev->flags) ||
> +           test_bit(Journal, &rdev->flags))
> +               return false;
> +
> +       return true;
> +}
> +
>  static int remove_and_add_spares(struct mddev *mddev,
>                                  struct md_rdev *this)
>  {
> @@ -9166,11 +9182,7 @@ static int remove_and_add_spares(struct mddev *mddev,
>                 return 0;
>
>         rdev_for_each(rdev, mddev) {
> -               if ((this == NULL || rdev == this) &&
> -                   rdev->raid_disk >= 0 &&
> -                   !test_bit(Blocked, &rdev->flags) &&
> -                   test_bit(Faulty, &rdev->flags) &&
> -                   atomic_read(&rdev->nr_pending)==0) {
> +               if ((this == NULL || rdev == this) && rdev_removeable(rdev)) {

There is a small change with the original method. Before this patch,
it checks the Faulty flag when setting RemoveSynchronized and it
checks RemoveSynchronized and "!In_sync && !Journal". I'm not sure if
it's right or not.

>                         /* Faulty non-Blocked devices with nr_pending == 0
>                          * never get nr_pending incremented,
>                          * never get Faulty cleared, and never get Blocked set.
> @@ -9185,19 +9197,12 @@ static int remove_and_add_spares(struct mddev *mddev,
>                 synchronize_rcu();
>         rdev_for_each(rdev, mddev) {
>                 if ((this == NULL || rdev == this) &&
> -                   rdev->raid_disk >= 0 &&
> -                   !test_bit(Blocked, &rdev->flags) &&
> -                   ((test_bit(RemoveSynchronized, &rdev->flags) ||
> -                    (!test_bit(In_sync, &rdev->flags) &&
> -                     !test_bit(Journal, &rdev->flags))) &&
> -                   atomic_read(&rdev->nr_pending)==0)) {
> -                       if (mddev->pers->hot_remove_disk(
> -                                   mddev, rdev) == 0) {
> +                   rdev_removeable(rdev) &&
> +                   mddev->pers->hot_remove_disk(mddev, rdev) == 0) {
>                                 sysfs_unlink_rdev(mddev, rdev);
>                                 rdev->saved_raid_disk = rdev->raid_disk;
>                                 rdev->raid_disk = -1;
>                                 removed++;
> -                       }
>                 }
>                 if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
>                         clear_bit(RemoveSynchronized, &rdev->flags);
> --
> 2.39.2
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() from remove_and_add_spares()
  2023-08-20  9:09 ` [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() " Yu Kuai
@ 2023-08-23  2:20   ` Xiao Ni
  2023-08-23  2:28     ` Xiao Ni
  0 siblings, 1 reply; 25+ messages in thread
From: Xiao Ni @ 2023-08-23  2:20 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> There are no functional changes, just to make the code simpler and
> prepare to delay remove_and_add_spares() to md_start_sync().
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index ceace5ffadd6..11d27c934fdd 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9169,6 +9169,14 @@ static bool rdev_removeable(struct md_rdev *rdev)
>         return true;
>  }
>
> +static bool rdev_is_spare(struct md_rdev *rdev)
> +{
> +       return !test_bit(Candidate, &rdev->flags) && rdev->raid_disk >= 0 &&
> +              !test_bit(In_sync, &rdev->flags) &&
> +              !test_bit(Journal, &rdev->flags) &&
> +              !test_bit(Faulty, &rdev->flags);
> +}
> +
>  static int remove_and_add_spares(struct mddev *mddev,
>                                  struct md_rdev *this)
>  {
> @@ -9217,13 +9225,10 @@ static int remove_and_add_spares(struct mddev *mddev,
>         rdev_for_each(rdev, mddev) {
>                 if (this && this != rdev)
>                         continue;
> +               if (rdev_is_spare(rdev))
> +                       spares++;
>                 if (test_bit(Candidate, &rdev->flags))
>                         continue;

Hi Kuai

Why not put rdev_is_spare after testing Candidate?

Best Regards
Xiao

> -               if (rdev->raid_disk >= 0 &&
> -                   !test_bit(In_sync, &rdev->flags) &&
> -                   !test_bit(Journal, &rdev->flags) &&
> -                   !test_bit(Faulty, &rdev->flags))
> -                       spares++;
>                 if (rdev->raid_disk >= 0)
>                         continue;
>                 if (test_bit(Faulty, &rdev->flags))
> --
> 2.39.2
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() from remove_and_add_spares()
  2023-08-23  2:20   ` Xiao Ni
@ 2023-08-23  2:28     ` Xiao Ni
  2023-08-23  2:47       ` Yu Kuai
  0 siblings, 1 reply; 25+ messages in thread
From: Xiao Ni @ 2023-08-23  2:28 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

On Wed, Aug 23, 2023 at 10:20 AM Xiao Ni <xni@redhat.com> wrote:
>
> On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >
> > From: Yu Kuai <yukuai3@huawei.com>
> >
> > There are no functional changes, just to make the code simpler and
> > prepare to delay remove_and_add_spares() to md_start_sync().
> >
> > Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> > ---
> >  drivers/md/md.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index ceace5ffadd6..11d27c934fdd 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -9169,6 +9169,14 @@ static bool rdev_removeable(struct md_rdev *rdev)
> >         return true;
> >  }
> >
> > +static bool rdev_is_spare(struct md_rdev *rdev)
> > +{
> > +       return !test_bit(Candidate, &rdev->flags) && rdev->raid_disk >= 0 &&
> > +              !test_bit(In_sync, &rdev->flags) &&
> > +              !test_bit(Journal, &rdev->flags) &&
> > +              !test_bit(Faulty, &rdev->flags);
> > +}
> > +
> >  static int remove_and_add_spares(struct mddev *mddev,
> >                                  struct md_rdev *this)
> >  {
> > @@ -9217,13 +9225,10 @@ static int remove_and_add_spares(struct mddev *mddev,
> >         rdev_for_each(rdev, mddev) {
> >                 if (this && this != rdev)
> >                         continue;
> > +               if (rdev_is_spare(rdev))
> > +                       spares++;
> >                 if (test_bit(Candidate, &rdev->flags))
> >                         continue;
>
> Hi Kuai
>
> Why not put rdev_is_spare after testing Candidate?
>
> Best Regards
> Xiao

I know the answer now, Because the next patch wants to put codes into
the function rdev_addable
>
> > -               if (rdev->raid_disk >= 0 &&
> > -                   !test_bit(In_sync, &rdev->flags) &&
> > -                   !test_bit(Journal, &rdev->flags) &&
> > -                   !test_bit(Faulty, &rdev->flags))
> > -                       spares++;
> >                 if (rdev->raid_disk >= 0)
> >                         continue;
> >                 if (test_bit(Faulty, &rdev->flags))
> > --
> > 2.39.2
> >

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares()
  2023-08-22 10:19   ` Xiao Ni
@ 2023-08-23  2:45     ` Yu Kuai
  2023-08-23  3:42       ` Xiao Ni
  0 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-23  2:45 UTC (permalink / raw)
  To: Xiao Ni, Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

Hi,

在 2023/08/22 18:19, Xiao Ni 写道:
> On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> There are no functional changes, just to make the code simpler and
>> prepare to delay remove_and_add_spares() to md_start_sync().
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/md.c | 33 +++++++++++++++++++--------------
>>   1 file changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 561cac13ff96..ceace5ffadd6 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9153,6 +9153,22 @@ void md_do_sync(struct md_thread *thread)
>>   }
>>   EXPORT_SYMBOL_GPL(md_do_sync);
>>
>> +static bool rdev_removeable(struct md_rdev *rdev)
>> +{
>> +       if (rdev->raid_disk < 0 || test_bit(Blocked, &rdev->flags) ||
>> +           atomic_read(&rdev->nr_pending))
>> +               return false;
>> +
>> +       if (test_bit(RemoveSynchronized, &rdev->flags))
>> +               return true;
>> +
>> +       if (test_bit(In_sync, &rdev->flags) ||
>> +           test_bit(Journal, &rdev->flags))
>> +               return false;
>> +
>> +       return true;
>> +}
>> +
>>   static int remove_and_add_spares(struct mddev *mddev,
>>                                   struct md_rdev *this)
>>   {
>> @@ -9166,11 +9182,7 @@ static int remove_and_add_spares(struct mddev *mddev,
>>                  return 0;
>>
>>          rdev_for_each(rdev, mddev) {
>> -               if ((this == NULL || rdev == this) &&
>> -                   rdev->raid_disk >= 0 &&
>> -                   !test_bit(Blocked, &rdev->flags) &&
>> -                   test_bit(Faulty, &rdev->flags) &&
>> -                   atomic_read(&rdev->nr_pending)==0) {
>> +               if ((this == NULL || rdev == this) && rdev_removeable(rdev)) {
> 
> There is a small change with the original method. Before this patch,
> it checks the Faulty flag when setting RemoveSynchronized and it
> checks RemoveSynchronized and "!In_sync && !Journal". I'm not sure if
> it's right or not.

Yes, there is a small change. After a second thought, I think it's OK
to leave the code to set RemoveSynchronized where it is for now, because
it'll be removed later. I don't need to bother factor out a common code
to set RemoveSynchronized and call hot_remove_disk().

By the way, once refactor of mddev_suspend() is done, then access to
rdev from fastpath will be replaced from:

rcu_read_lock()
...
rcu_read_unlock()

to:

md_array_enter()
// grab 'active_io', 'active_io' will probably be renamed
...
md_array_exit()

That's why I said RemoveSynchronized will be removed.

Thanks,
Kuai

> 
>>                          /* Faulty non-Blocked devices with nr_pending == 0
>>                           * never get nr_pending incremented,
>>                           * never get Faulty cleared, and never get Blocked set.
>> @@ -9185,19 +9197,12 @@ static int remove_and_add_spares(struct mddev *mddev,
>>                  synchronize_rcu();
>>          rdev_for_each(rdev, mddev) {
>>                  if ((this == NULL || rdev == this) &&
>> -                   rdev->raid_disk >= 0 &&
>> -                   !test_bit(Blocked, &rdev->flags) &&
>> -                   ((test_bit(RemoveSynchronized, &rdev->flags) ||
>> -                    (!test_bit(In_sync, &rdev->flags) &&
>> -                     !test_bit(Journal, &rdev->flags))) &&
>> -                   atomic_read(&rdev->nr_pending)==0)) {
>> -                       if (mddev->pers->hot_remove_disk(
>> -                                   mddev, rdev) == 0) {
>> +                   rdev_removeable(rdev) &&
>> +                   mddev->pers->hot_remove_disk(mddev, rdev) == 0) {
>>                                  sysfs_unlink_rdev(mddev, rdev);
>>                                  rdev->saved_raid_disk = rdev->raid_disk;
>>                                  rdev->raid_disk = -1;
>>                                  removed++;
>> -                       }
>>                  }
>>                  if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
>>                          clear_bit(RemoveSynchronized, &rdev->flags);
>> --
>> 2.39.2
>>
> 
> .
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() from remove_and_add_spares()
  2023-08-23  2:28     ` Xiao Ni
@ 2023-08-23  2:47       ` Yu Kuai
  0 siblings, 0 replies; 25+ messages in thread
From: Yu Kuai @ 2023-08-23  2:47 UTC (permalink / raw)
  To: Xiao Ni, Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

Hi,

在 2023/08/23 10:28, Xiao Ni 写道:
> On Wed, Aug 23, 2023 at 10:20 AM Xiao Ni <xni@redhat.com> wrote:
>>
>> On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> There are no functional changes, just to make the code simpler and
>>> prepare to delay remove_and_add_spares() to md_start_sync().
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>> ---
>>>   drivers/md/md.c | 15 ++++++++++-----
>>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index ceace5ffadd6..11d27c934fdd 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -9169,6 +9169,14 @@ static bool rdev_removeable(struct md_rdev *rdev)
>>>          return true;
>>>   }
>>>
>>> +static bool rdev_is_spare(struct md_rdev *rdev)
>>> +{
>>> +       return !test_bit(Candidate, &rdev->flags) && rdev->raid_disk >= 0 &&
>>> +              !test_bit(In_sync, &rdev->flags) &&
>>> +              !test_bit(Journal, &rdev->flags) &&
>>> +              !test_bit(Faulty, &rdev->flags);
>>> +}
>>> +
>>>   static int remove_and_add_spares(struct mddev *mddev,
>>>                                   struct md_rdev *this)
>>>   {
>>> @@ -9217,13 +9225,10 @@ static int remove_and_add_spares(struct mddev *mddev,
>>>          rdev_for_each(rdev, mddev) {
>>>                  if (this && this != rdev)
>>>                          continue;
>>> +               if (rdev_is_spare(rdev))
>>> +                       spares++;
>>>                  if (test_bit(Candidate, &rdev->flags))
>>>                          continue;
>>
>> Hi Kuai
>>
>> Why not put rdev_is_spare after testing Candidate?
>>
>> Best Regards
>> Xiao
> 
> I know the answer now, Because the next patch wants to put codes into
> the function rdev_addable

Yes, and thanks for the review!

Kuai
>>
>>> -               if (rdev->raid_disk >= 0 &&
>>> -                   !test_bit(In_sync, &rdev->flags) &&
>>> -                   !test_bit(Journal, &rdev->flags) &&
>>> -                   !test_bit(Faulty, &rdev->flags))
>>> -                       spares++;
>>>                  if (rdev->raid_disk >= 0)
>>>                          continue;
>>>                  if (test_bit(Faulty, &rdev->flags))
>>> --
>>> 2.39.2
>>>
> 
> Reviewed-by: Xiao Ni <xni@redhat.com>
> 
> .
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-22  2:17     ` Yu Kuai
@ 2023-08-23  3:04       ` Yu Kuai
  2023-08-23  5:26         ` Song Liu
  0 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-23  3:04 UTC (permalink / raw)
  To: Yu Kuai, Song Liu
  Cc: xni, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

Hi,

在 2023/08/22 10:17, Yu Kuai 写道:
> Hi,
> 
> 在 2023/08/22 7:22, Song Liu 写道:
>> On Sun, Aug 20, 2023 at 2:13 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> There are no functional changes, just to make the code simpler and
>>> prepare to delay remove_and_add_spares() to md_start_sync().
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>> ---
>>>   drivers/md/md.c | 28 ++++++++++++++++------------
>>>   1 file changed, 16 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index 11d27c934fdd..cdc361c521d4 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -9177,6 +9177,20 @@ static bool rdev_is_spare(struct md_rdev *rdev)
>>>                 !test_bit(Faulty, &rdev->flags);
>>>   }
>>>
>>> +static bool rdev_addable(struct md_rdev *rdev)
>>> +{
>>> +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
>>> +           test_bit(Faulty, &rdev->flags))
>>> +               return false;
>>> +
>>> +       if (!test_bit(Journal, &rdev->flags) && 
>>> !md_is_rdwr(rdev->mddev) &&
>>
>> Instead of straightforward refactoring, I hope we can make these rdev_*
>> helpers more meaningful, and hopefullly reusable. For example, let's 
>> define
>> the meaning of "addable", and write the function to match that 
>> meaning. In
>> this case, I think we shouldn't check md_is_rdwr() inside rdev_addable().
>>
>> Does this make sense?
> 
> Yes, this make sense, rdev can be added to read-only array.
> 
> There are total three callers of pers->hot_add_sisk, I'll try to find if
> they have common conditions.

Unfortunately, the conditions is quite different, and It's difficult to
factor out a common helper for them to use.

In this case, !md_is_rdwr() is one of the four conditions, which means
if the array is read-only, there is a special case that rdev can't be
added to the configuration. Perhaps it's okay to keep this?

Thanks,
Kuai
> 
> Thanks,
> Kuai
> 
>>
>> Thanks,
>> Song
>>
>>
>>> +           !(rdev->saved_raid_disk >= 0 &&
>>> +             !test_bit(Bitmap_sync, &rdev->flags)))
>>> +               return false;
>>> +
>>> +       return true;
>>> +}
>>> +
>>>   static int remove_and_add_spares(struct mddev *mddev,
>>>                                   struct md_rdev *this)
>>>   {
>>> @@ -9227,20 +9241,10 @@ static int remove_and_add_spares(struct mddev 
>>> *mddev,
>>>                          continue;
>>>                  if (rdev_is_spare(rdev))
>>>                          spares++;
>>> -               if (test_bit(Candidate, &rdev->flags))
>>> +               if (!rdev_addable(rdev))
>>>                          continue;
>>> -               if (rdev->raid_disk >= 0)
>>> -                       continue;
>>> -               if (test_bit(Faulty, &rdev->flags))
>>> -                       continue;
>>> -               if (!test_bit(Journal, &rdev->flags)) {
>>> -                       if (!md_is_rdwr(mddev) &&
>>> -                           !(rdev->saved_raid_disk >= 0 &&
>>> -                             !test_bit(Bitmap_sync, &rdev->flags)))
>>> -                               continue;
>>> -
>>> +               if (!test_bit(Journal, &rdev->flags))
>>>                          rdev->recovery_offset = 0;
>>> -               }
>>>                  if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
>>>                          /* failure here is OK */
>>>                          sysfs_link_rdev(mddev, rdev);
>>> -- 
>>> 2.39.2
>>>
>> .
>>
> 
> .
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync()
  2023-08-20  9:09 ` [PATCH -next v3 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync() Yu Kuai
@ 2023-08-23  3:24   ` Xiao Ni
  0 siblings, 0 replies; 25+ messages in thread
From: Xiao Ni @ 2023-08-23  3:24 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yukuai3,
	yi.zhang, yangerkun

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-only array:
>
> md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, then it will
> call remove_and_add_spares() directly to try to remove and add rdevs
> from array.
>
> After this patch:
>
> 1) md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, and the
>    worker 'sync_work' is not pending, and there are rdevs can be added
>    or removed, then it will queue new work md_start_sync();
> 2) md_start_sync() will call remove_and_add_spares() and exist;
>
> This change make sure that array reconfiguration is independent from
> daemon, and it'll be much easier to synchronize it with io, consier
> that io may rely on daemon thread to be done.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index cdc361c521d4..f08b683f724f 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4866,6 +4866,7 @@ action_store(struct mddev *mddev, const char *page, size_t len)
>                 /* A write to sync_action is enough to justify
>                  * canceling read-auto mode
>                  */
> +               flush_work(&mddev->sync_work);
>                 mddev->ro = MD_RDWR;
>                 md_wakeup_thread(mddev->sync_thread);
>         }
> @@ -7638,6 +7639,10 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
>                 mutex_unlock(&mddev->open_mutex);
>                 sync_blockdev(bdev);
>         }
> +
> +       if (!md_is_rdwr(mddev))
> +               flush_work(&mddev->sync_work);
> +
>         err = mddev_lock(mddev);
>         if (err) {
>                 pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n",
> @@ -8562,6 +8567,7 @@ bool md_write_start(struct mddev *mddev, struct bio *bi)
>         BUG_ON(mddev->ro == MD_RDONLY);
>         if (mddev->ro == MD_AUTO_READ) {
>                 /* need to switch to read/write */
> +               flush_work(&mddev->sync_work);
>                 mddev->ro = MD_RDWR;
>                 set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>                 md_wakeup_thread(mddev->thread);
> @@ -9191,6 +9197,16 @@ static bool rdev_addable(struct md_rdev *rdev)
>         return true;
>  }
>
> +static bool md_spares_need_change(struct mddev *mddev)
> +{
> +       struct md_rdev *rdev;
> +
> +       rdev_for_each(rdev, mddev)
> +               if (rdev_removeable(rdev) || rdev_addable(rdev))
> +                       return true;
> +       return false;
> +}
> +
>  static int remove_and_add_spares(struct mddev *mddev,
>                                  struct md_rdev *this)
>  {
> @@ -9311,6 +9327,12 @@ static void md_start_sync(struct work_struct *ws)
>
>         mddev_lock_nointr(mddev);
>
> +       if (!md_is_rdwr(mddev)) {
> +               remove_and_add_spares(mddev, NULL);
> +               mddev_unlock(mddev);
> +               return;
> +       }
> +
>         if (!md_choose_sync_action(mddev, &spares))
>                 goto not_running;
>
> @@ -9405,7 +9427,8 @@ void md_check_recovery(struct mddev *mddev)
>         }
>
>         if (!md_is_rdwr(mddev) &&
> -           !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery))
> +           (!test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> +            work_pending(&mddev->sync_work)))
>                 return;
>         if ( ! (
>                 (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
> @@ -9433,15 +9456,8 @@ void md_check_recovery(struct mddev *mddev)
>                                  */
>                                 rdev_for_each(rdev, mddev)
>                                         clear_bit(Blocked, &rdev->flags);
> -                       /* On a read-only array we can:
> -                        * - remove failed devices
> -                        * - add already-in_sync devices if the array itself
> -                        *   is in-sync.
> -                        * As we only add devices that are already in-sync,
> -                        * we can activate the spares immediately.
> -                        */

Can we keep those comments?

> -                       remove_and_add_spares(mddev, NULL);
> -                       /* There is no thread, but we need to call
> +                       /*
> +                        * There is no thread, but we need to call
>                          * ->spare_active and clear saved_raid_disk
>                          */
>                         set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> @@ -9449,6 +9465,13 @@ void md_check_recovery(struct mddev *mddev)
>                         clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>                         clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>                         clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);
> +
> +                       /*
> +                        * Let md_start_sync() to remove and add rdevs to the
> +                        * array.
> +                        */
> +                       if (md_spares_need_change(mddev))
> +                               queue_work(md_misc_wq, &mddev->sync_work);
>                         goto unlock;
>                 }
>
> --
> 2.39.2
>

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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares()
  2023-08-23  2:45     ` Yu Kuai
@ 2023-08-23  3:42       ` Xiao Ni
  0 siblings, 0 replies; 25+ messages in thread
From: Xiao Ni @ 2023-08-23  3:42 UTC (permalink / raw)
  To: Yu Kuai
  Cc: song, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

On Wed, Aug 23, 2023 at 10:45 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2023/08/22 18:19, Xiao Ni 写道:
> > On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >>
> >> From: Yu Kuai <yukuai3@huawei.com>
> >>
> >> There are no functional changes, just to make the code simpler and
> >> prepare to delay remove_and_add_spares() to md_start_sync().
> >>
> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> >> ---
> >>   drivers/md/md.c | 33 +++++++++++++++++++--------------
> >>   1 file changed, 19 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 561cac13ff96..ceace5ffadd6 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -9153,6 +9153,22 @@ void md_do_sync(struct md_thread *thread)
> >>   }
> >>   EXPORT_SYMBOL_GPL(md_do_sync);
> >>
> >> +static bool rdev_removeable(struct md_rdev *rdev)
> >> +{
> >> +       if (rdev->raid_disk < 0 || test_bit(Blocked, &rdev->flags) ||
> >> +           atomic_read(&rdev->nr_pending))
> >> +               return false;
> >> +
> >> +       if (test_bit(RemoveSynchronized, &rdev->flags))
> >> +               return true;
> >> +
> >> +       if (test_bit(In_sync, &rdev->flags) ||
> >> +           test_bit(Journal, &rdev->flags))
> >> +               return false;
> >> +
> >> +       return true;
> >> +}
> >> +
> >>   static int remove_and_add_spares(struct mddev *mddev,
> >>                                   struct md_rdev *this)
> >>   {
> >> @@ -9166,11 +9182,7 @@ static int remove_and_add_spares(struct mddev *mddev,
> >>                  return 0;
> >>
> >>          rdev_for_each(rdev, mddev) {
> >> -               if ((this == NULL || rdev == this) &&
> >> -                   rdev->raid_disk >= 0 &&
> >> -                   !test_bit(Blocked, &rdev->flags) &&
> >> -                   test_bit(Faulty, &rdev->flags) &&
> >> -                   atomic_read(&rdev->nr_pending)==0) {
> >> +               if ((this == NULL || rdev == this) && rdev_removeable(rdev)) {
> >
> > There is a small change with the original method. Before this patch,
> > it checks the Faulty flag when setting RemoveSynchronized and it
> > checks RemoveSynchronized and "!In_sync && !Journal". I'm not sure if
> > it's right or not.
>
> Yes, there is a small change. After a second thought, I think it's OK
> to leave the code to set RemoveSynchronized where it is for now, because
> it'll be removed later. I don't need to bother factor out a common code
> to set RemoveSynchronized and call hot_remove_disk().

This will be easier for review, thanks.

>
> By the way, once refactor of mddev_suspend() is done, then access to
> rdev from fastpath will be replaced from:
>
> rcu_read_lock()
> ...
> rcu_read_unlock()
>
> to:
>
> md_array_enter()
> // grab 'active_io', 'active_io' will probably be renamed
> ...
> md_array_exit()
>
> That's why I said RemoveSynchronized will be removed.

:) I'll try to understand it in your following patches.

Regards
Xiao
>
> Thanks,
> Kuai
>
> >
> >>                          /* Faulty non-Blocked devices with nr_pending == 0
> >>                           * never get nr_pending incremented,
> >>                           * never get Faulty cleared, and never get Blocked set.
> >> @@ -9185,19 +9197,12 @@ static int remove_and_add_spares(struct mddev *mddev,
> >>                  synchronize_rcu();
> >>          rdev_for_each(rdev, mddev) {
> >>                  if ((this == NULL || rdev == this) &&
> >> -                   rdev->raid_disk >= 0 &&
> >> -                   !test_bit(Blocked, &rdev->flags) &&
> >> -                   ((test_bit(RemoveSynchronized, &rdev->flags) ||
> >> -                    (!test_bit(In_sync, &rdev->flags) &&
> >> -                     !test_bit(Journal, &rdev->flags))) &&
> >> -                   atomic_read(&rdev->nr_pending)==0)) {
> >> -                       if (mddev->pers->hot_remove_disk(
> >> -                                   mddev, rdev) == 0) {
> >> +                   rdev_removeable(rdev) &&
> >> +                   mddev->pers->hot_remove_disk(mddev, rdev) == 0) {
> >>                                  sysfs_unlink_rdev(mddev, rdev);
> >>                                  rdev->saved_raid_disk = rdev->raid_disk;
> >>                                  rdev->raid_disk = -1;
> >>                                  removed++;
> >> -                       }
> >>                  }
> >>                  if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
> >>                          clear_bit(RemoveSynchronized, &rdev->flags);
> >> --
> >> 2.39.2
> >>
> >
> > .
> >
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-23  3:04       ` Yu Kuai
@ 2023-08-23  5:26         ` Song Liu
  2023-08-23  8:37           ` Yu Kuai
  0 siblings, 1 reply; 25+ messages in thread
From: Song Liu @ 2023-08-23  5:26 UTC (permalink / raw)
  To: Yu Kuai
  Cc: xni, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

On Tue, Aug 22, 2023 at 8:04 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2023/08/22 10:17, Yu Kuai 写道:
> > Hi,
> >
> > 在 2023/08/22 7:22, Song Liu 写道:
> >> On Sun, Aug 20, 2023 at 2:13 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >>>
> >>> From: Yu Kuai <yukuai3@huawei.com>
> >>>
> >>> There are no functional changes, just to make the code simpler and
> >>> prepare to delay remove_and_add_spares() to md_start_sync().
> >>>
> >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> >>> ---
> >>>   drivers/md/md.c | 28 ++++++++++++++++------------
> >>>   1 file changed, 16 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >>> index 11d27c934fdd..cdc361c521d4 100644
> >>> --- a/drivers/md/md.c
> >>> +++ b/drivers/md/md.c
> >>> @@ -9177,6 +9177,20 @@ static bool rdev_is_spare(struct md_rdev *rdev)
> >>>                 !test_bit(Faulty, &rdev->flags);
> >>>   }
> >>>
> >>> +static bool rdev_addable(struct md_rdev *rdev)
> >>> +{
> >>> +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
> >>> +           test_bit(Faulty, &rdev->flags))
> >>> +               return false;
> >>> +
> >>> +       if (!test_bit(Journal, &rdev->flags) &&
> >>> !md_is_rdwr(rdev->mddev) &&
> >>
> >> Instead of straightforward refactoring, I hope we can make these rdev_*
> >> helpers more meaningful, and hopefullly reusable. For example, let's
> >> define
> >> the meaning of "addable", and write the function to match that
> >> meaning. In
> >> this case, I think we shouldn't check md_is_rdwr() inside rdev_addable().
> >>
> >> Does this make sense?
> >
> > Yes, this make sense, rdev can be added to read-only array.
> >
> > There are total three callers of pers->hot_add_sisk, I'll try to find if
> > they have common conditions.
>
> Unfortunately, the conditions is quite different, and It's difficult to
> factor out a common helper for them to use.
>
> In this case, !md_is_rdwr() is one of the four conditions, which means
> if the array is read-only, there is a special case that rdev can't be
> added to the configuration. Perhaps it's okay to keep this?

My main concern is that rdev_addable() is not making the code easier to
understand. We have a few different cases at this point:

1. rdev is not suitable for add (Faulty, raid_disk>=0, Candidate);
2. rdev is Journal;
3. Re-add rdev to RO array;
4. Non-re-add rdev to RO array;
5. Other cases.

Current rdev_addable() handles more or less all of this, which is
confusing. Maybe we can do something along similar to the
following (not tested). Does this look more clear?

Thanks,
Song

diff --git i/drivers/md/md.c w/drivers/md/md.c
index 78be7811a89f..8cb855d03e0a 100644
--- i/drivers/md/md.c
+++ w/drivers/md/md.c
@@ -9117,6 +9117,20 @@ void md_do_sync(struct md_thread *thread)
 }
 EXPORT_SYMBOL_GPL(md_do_sync);

+static bool rdev_addable(struct md_rdev *rdev)
+{
+       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
+           test_bit(Faulty, &rdev->flags))
+               return false;
+       return true;
+}
+
+static bool rdev_is_readd(struct md_rdev *rdev)
+{
+       return rdev->saved_raid_disk >= 0 ||
+               !test_bit(Bitmap_sync, &rdev->flags);
+}
+
 static int remove_and_add_spares(struct mddev *mddev,
                                 struct md_rdev *this)
 {
@@ -9176,25 +9190,24 @@ static int remove_and_add_spares(struct mddev *mddev,
        rdev_for_each(rdev, mddev) {
                if (this && this != rdev)
                        continue;
-               if (test_bit(Candidate, &rdev->flags))
-                       continue;
                if (rdev->raid_disk >= 0 &&
                    !test_bit(In_sync, &rdev->flags) &&
                    !test_bit(Journal, &rdev->flags) &&
                    !test_bit(Faulty, &rdev->flags))
                        spares++;
-               if (rdev->raid_disk >= 0)
+
+               if (!rdev_addable(rdev))
                        continue;
-               if (test_bit(Faulty, &rdev->flags))
+
+               if (test_bit(Journal, &rdev->flags))
+                       goto hot_add_disk;
+
+               if (!md_is_rdwr(mddev) && !rdev_is_readd(rdev))
                        continue;
-               if (!test_bit(Journal, &rdev->flags)) {
-                       if (!md_is_rdwr(mddev) &&
-                           !(rdev->saved_raid_disk >= 0 &&
-                             !test_bit(Bitmap_sync, &rdev->flags)))
-                               continue;

-                       rdev->recovery_offset = 0;
-               }
+               rdev->recovery_offset = 0;
+
+       hot_add_disk:
                if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
                        /* failure here is OK */
                        sysfs_link_rdev(mddev, rdev);

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-23  5:26         ` Song Liu
@ 2023-08-23  8:37           ` Yu Kuai
  2023-08-23 11:25             ` Song Liu
  0 siblings, 1 reply; 25+ messages in thread
From: Yu Kuai @ 2023-08-23  8:37 UTC (permalink / raw)
  To: Song Liu, Yu Kuai
  Cc: xni, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

Hi,

在 2023/08/23 13:26, Song Liu 写道:
> On Tue, Aug 22, 2023 at 8:04 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> Hi,
>>
>> 在 2023/08/22 10:17, Yu Kuai 写道:
>>> Hi,
>>>
>>> 在 2023/08/22 7:22, Song Liu 写道:
>>>> On Sun, Aug 20, 2023 at 2:13 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>>>
>>>>> From: Yu Kuai <yukuai3@huawei.com>
>>>>>
>>>>> There are no functional changes, just to make the code simpler and
>>>>> prepare to delay remove_and_add_spares() to md_start_sync().
>>>>>
>>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>>>> ---
>>>>>    drivers/md/md.c | 28 ++++++++++++++++------------
>>>>>    1 file changed, 16 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>>>> index 11d27c934fdd..cdc361c521d4 100644
>>>>> --- a/drivers/md/md.c
>>>>> +++ b/drivers/md/md.c
>>>>> @@ -9177,6 +9177,20 @@ static bool rdev_is_spare(struct md_rdev *rdev)
>>>>>                  !test_bit(Faulty, &rdev->flags);
>>>>>    }
>>>>>
>>>>> +static bool rdev_addable(struct md_rdev *rdev)
>>>>> +{
>>>>> +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
>>>>> +           test_bit(Faulty, &rdev->flags))
>>>>> +               return false;
>>>>> +
>>>>> +       if (!test_bit(Journal, &rdev->flags) &&
>>>>> !md_is_rdwr(rdev->mddev) &&
>>>>
>>>> Instead of straightforward refactoring, I hope we can make these rdev_*
>>>> helpers more meaningful, and hopefullly reusable. For example, let's
>>>> define
>>>> the meaning of "addable", and write the function to match that
>>>> meaning. In
>>>> this case, I think we shouldn't check md_is_rdwr() inside rdev_addable().
>>>>
>>>> Does this make sense?
>>>
>>> Yes, this make sense, rdev can be added to read-only array.
>>>
>>> There are total three callers of pers->hot_add_sisk, I'll try to find if
>>> they have common conditions.
>>
>> Unfortunately, the conditions is quite different, and It's difficult to
>> factor out a common helper for them to use.
>>
>> In this case, !md_is_rdwr() is one of the four conditions, which means
>> if the array is read-only, there is a special case that rdev can't be
>> added to the configuration. Perhaps it's okay to keep this?
> 
> My main concern is that rdev_addable() is not making the code easier to
> understand. We have a few different cases at this point:
> 
> 1. rdev is not suitable for add (Faulty, raid_disk>=0, Candidate);
> 2. rdev is Journal;
> 3. Re-add rdev to RO array;
> 4. Non-re-add rdev to RO array;
> 5. Other cases.
> 
> Current rdev_addable() handles more or less all of this, which is
> confusing. Maybe we can do something along similar to the
> following (not tested). Does this look more clear?
> 
> Thanks,
> Song
> 
> diff --git i/drivers/md/md.c w/drivers/md/md.c
> index 78be7811a89f..8cb855d03e0a 100644
> --- i/drivers/md/md.c
> +++ w/drivers/md/md.c
> @@ -9117,6 +9117,20 @@ void md_do_sync(struct md_thread *thread)
>   }
>   EXPORT_SYMBOL_GPL(md_do_sync);
> 
> +static bool rdev_addable(struct md_rdev *rdev)
> +{
> +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
> +           test_bit(Faulty, &rdev->flags))
> +               return false;
> +       return true;
> +}
> +
> +static bool rdev_is_readd(struct md_rdev *rdev)
> +{
> +       return rdev->saved_raid_disk >= 0 ||
> +               !test_bit(Bitmap_sync, &rdev->flags);
This should use '&&' instead of '||' ?

> +}
> +
>   static int remove_and_add_spares(struct mddev *mddev,
>                                   struct md_rdev *this)
>   {
> @@ -9176,25 +9190,24 @@ static int remove_and_add_spares(struct mddev *mddev,
>          rdev_for_each(rdev, mddev) {
>                  if (this && this != rdev)
>                          continue;
> -               if (test_bit(Candidate, &rdev->flags))
> -                       continue;
>                  if (rdev->raid_disk >= 0 &&
>                      !test_bit(In_sync, &rdev->flags) &&
>                      !test_bit(Journal, &rdev->flags) &&
>                      !test_bit(Faulty, &rdev->flags))
>                          spares++;
> -               if (rdev->raid_disk >= 0)
> +
> +               if (!rdev_addable(rdev))
>                          continue;
> -               if (test_bit(Faulty, &rdev->flags))
> +
> +               if (test_bit(Journal, &rdev->flags))
> +                       goto hot_add_disk;
> +

I understand what you mean now, but I must use the exact same judgement
in the new helper md_spares_need_change() in patch 7, there will be 
redundant code this way.

How about this, rework rdev_addable():

   static bool rdev_addable(struct md_rdev *rdev)
   {
+         /* rdev is already used, don't add it again. */
           if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
               test_bit(Faulty, &rdev->flags))
                   return false;

~         /* Allow to add journal disk. */
~         if (test_bit(Journal, &rdev->flags))
~_                return true;

~         /* Allow to add if array is read-write. */
+         if (md_is_rdwr(rdev->mddev))
+                 return true;
+
+         /*
+          * For read-only array, only allow to readd a rdev. And if 
bitmap is
+          * used, don't allow to readd a rdev that is too old.
+          */
+         if (rdev->saved_raid_disk >=0 && !test_bit(Bitmap_sync, 
&rdev->flags))
+                 return true;
+
+         return false;
   }


Thanks,
Kuai

> +               if (!md_is_rdwr(mddev) && !rdev_is_readd(rdev))
>                          continue;
> -               if (!test_bit(Journal, &rdev->flags)) {
> -                       if (!md_is_rdwr(mddev) &&
> -                           !(rdev->saved_raid_disk >= 0 &&
> -                             !test_bit(Bitmap_sync, &rdev->flags)))
> -                               continue;
> 
> -                       rdev->recovery_offset = 0;
> -               }
> +               rdev->recovery_offset = 0;
> +
> +       hot_add_disk:
>                  if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
>                          /* failure here is OK */
>                          sysfs_link_rdev(mddev, rdev);
> .
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-23  8:37           ` Yu Kuai
@ 2023-08-23 11:25             ` Song Liu
  2023-08-24  1:16               ` Yu Kuai
  0 siblings, 1 reply; 25+ messages in thread
From: Song Liu @ 2023-08-23 11:25 UTC (permalink / raw)
  To: Yu Kuai
  Cc: xni, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

On Wed, Aug 23, 2023 at 1:37 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
[...]
> > diff --git i/drivers/md/md.c w/drivers/md/md.c
> > index 78be7811a89f..8cb855d03e0a 100644
> > --- i/drivers/md/md.c
> > +++ w/drivers/md/md.c
> > @@ -9117,6 +9117,20 @@ void md_do_sync(struct md_thread *thread)
> >   }
> >   EXPORT_SYMBOL_GPL(md_do_sync);
> >
> > +static bool rdev_addable(struct md_rdev *rdev)
> > +{
> > +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
> > +           test_bit(Faulty, &rdev->flags))
> > +               return false;
> > +       return true;
> > +}
> > +
> > +static bool rdev_is_readd(struct md_rdev *rdev)
> > +{
> > +       return rdev->saved_raid_disk >= 0 ||
> > +               !test_bit(Bitmap_sync, &rdev->flags);
> This should use '&&' instead of '||' ?
>
> > +}
> > +
> >   static int remove_and_add_spares(struct mddev *mddev,
> >                                   struct md_rdev *this)
> >   {
> > @@ -9176,25 +9190,24 @@ static int remove_and_add_spares(struct mddev *mddev,
> >          rdev_for_each(rdev, mddev) {
> >                  if (this && this != rdev)
> >                          continue;
> > -               if (test_bit(Candidate, &rdev->flags))
> > -                       continue;
> >                  if (rdev->raid_disk >= 0 &&
> >                      !test_bit(In_sync, &rdev->flags) &&
> >                      !test_bit(Journal, &rdev->flags) &&
> >                      !test_bit(Faulty, &rdev->flags))
> >                          spares++;
> > -               if (rdev->raid_disk >= 0)
> > +
> > +               if (!rdev_addable(rdev))
> >                          continue;
> > -               if (test_bit(Faulty, &rdev->flags))
> > +
> > +               if (test_bit(Journal, &rdev->flags))
> > +                       goto hot_add_disk;
> > +
>
> I understand what you mean now, but I must use the exact same judgement
> in the new helper md_spares_need_change() in patch 7, there will be
> redundant code this way.
>
> How about this, rework rdev_addable():

Yeah, this was another option that I was thinking about. Let's go with
this version.

Thanks,
Song

>
>    static bool rdev_addable(struct md_rdev *rdev)
>    {
> +         /* rdev is already used, don't add it again. */
>            if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
>                test_bit(Faulty, &rdev->flags))
>                    return false;
>
> ~         /* Allow to add journal disk. */
> ~         if (test_bit(Journal, &rdev->flags))
> ~_                return true;
>
> ~         /* Allow to add if array is read-write. */
> +         if (md_is_rdwr(rdev->mddev))
> +                 return true;
> +
> +         /*
> +          * For read-only array, only allow to readd a rdev. And if
> bitmap is
> +          * used, don't allow to readd a rdev that is too old.
> +          */
> +         if (rdev->saved_raid_disk >=0 && !test_bit(Bitmap_sync,
> &rdev->flags))
> +                 return true;
> +
> +         return false;
>    }

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH -next v3 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()
  2023-08-23 11:25             ` Song Liu
@ 2023-08-24  1:16               ` Yu Kuai
  0 siblings, 0 replies; 25+ messages in thread
From: Yu Kuai @ 2023-08-24  1:16 UTC (permalink / raw)
  To: Song Liu, Yu Kuai
  Cc: xni, mariusz.tkaczyk, linux-raid, linux-kernel, yi.zhang,
	yangerkun, yukuai (C)

Hi,

在 2023/08/23 19:25, Song Liu 写道:
> On Wed, Aug 23, 2023 at 1:37 AM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
> [...]
>>> diff --git i/drivers/md/md.c w/drivers/md/md.c
>>> index 78be7811a89f..8cb855d03e0a 100644
>>> --- i/drivers/md/md.c
>>> +++ w/drivers/md/md.c
>>> @@ -9117,6 +9117,20 @@ void md_do_sync(struct md_thread *thread)
>>>    }
>>>    EXPORT_SYMBOL_GPL(md_do_sync);
>>>
>>> +static bool rdev_addable(struct md_rdev *rdev)
>>> +{
>>> +       if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
>>> +           test_bit(Faulty, &rdev->flags))
>>> +               return false;
>>> +       return true;
>>> +}
>>> +
>>> +static bool rdev_is_readd(struct md_rdev *rdev)
>>> +{
>>> +       return rdev->saved_raid_disk >= 0 ||
>>> +               !test_bit(Bitmap_sync, &rdev->flags);
>> This should use '&&' instead of '||' ?
>>
>>> +}
>>> +
>>>    static int remove_and_add_spares(struct mddev *mddev,
>>>                                    struct md_rdev *this)
>>>    {
>>> @@ -9176,25 +9190,24 @@ static int remove_and_add_spares(struct mddev *mddev,
>>>           rdev_for_each(rdev, mddev) {
>>>                   if (this && this != rdev)
>>>                           continue;
>>> -               if (test_bit(Candidate, &rdev->flags))
>>> -                       continue;
>>>                   if (rdev->raid_disk >= 0 &&
>>>                       !test_bit(In_sync, &rdev->flags) &&
>>>                       !test_bit(Journal, &rdev->flags) &&
>>>                       !test_bit(Faulty, &rdev->flags))
>>>                           spares++;
>>> -               if (rdev->raid_disk >= 0)
>>> +
>>> +               if (!rdev_addable(rdev))
>>>                           continue;
>>> -               if (test_bit(Faulty, &rdev->flags))
>>> +
>>> +               if (test_bit(Journal, &rdev->flags))
>>> +                       goto hot_add_disk;
>>> +
>>
>> I understand what you mean now, but I must use the exact same judgement
>> in the new helper md_spares_need_change() in patch 7, there will be
>> redundant code this way.
>>
>> How about this, rework rdev_addable():
> 
> Yeah, this was another option that I was thinking about. Let's go with
> this version.
> 
Ok, and I'll do this for rdev_removeable() in patch 4 as well.

Thanks,
Kuai

> Thanks,
> Song
> 
>>
>>     static bool rdev_addable(struct md_rdev *rdev)
>>     {
>> +         /* rdev is already used, don't add it again. */
>>             if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
>>                 test_bit(Faulty, &rdev->flags))
>>                     return false;
>>
>> ~         /* Allow to add journal disk. */
>> ~         if (test_bit(Journal, &rdev->flags))
>> ~_                return true;
>>
>> ~         /* Allow to add if array is read-write. */
>> +         if (md_is_rdwr(rdev->mddev))
>> +                 return true;
>> +
>> +         /*
>> +          * For read-only array, only allow to readd a rdev. And if
>> bitmap is
>> +          * used, don't allow to readd a rdev that is too old.
>> +          */
>> +         if (rdev->saved_raid_disk >=0 && !test_bit(Bitmap_sync,
>> &rdev->flags))
>> +                 return true;
>> +
>> +         return false;
>>     }
> .
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2023-08-24  1:17 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-20  9:09 [PATCH -next v3 0/7] md: make rdev addition and removal independent from daemon thread Yu Kuai
2023-08-20  9:09 ` [PATCH -next v3 1/7] md: use separate work_struct for md_start_sync() Yu Kuai
2023-08-22 10:04   ` Xiao Ni
2023-08-20  9:09 ` [PATCH -next v3 2/7] md: factor out a helper to choose sync action from md_check_recovery() Yu Kuai
2023-08-22 10:04   ` Xiao Ni
2023-08-20  9:09 ` [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync() Yu Kuai
2023-08-22 10:06   ` Xiao Ni
2023-08-20  9:09 ` [PATCH -next v3 4/7] md: factor out a helper rdev_removeable() from remove_and_add_spares() Yu Kuai
2023-08-22 10:19   ` Xiao Ni
2023-08-23  2:45     ` Yu Kuai
2023-08-23  3:42       ` Xiao Ni
2023-08-20  9:09 ` [PATCH -next v3 5/7] md: factor out a helper rdev_is_spare() " Yu Kuai
2023-08-23  2:20   ` Xiao Ni
2023-08-23  2:28     ` Xiao Ni
2023-08-23  2:47       ` Yu Kuai
2023-08-20  9:09 ` [PATCH -next v3 6/7] md: factor out a helper rdev_addable() " Yu Kuai
2023-08-21 23:22   ` Song Liu
2023-08-22  2:17     ` Yu Kuai
2023-08-23  3:04       ` Yu Kuai
2023-08-23  5:26         ` Song Liu
2023-08-23  8:37           ` Yu Kuai
2023-08-23 11:25             ` Song Liu
2023-08-24  1:16               ` Yu Kuai
2023-08-20  9:09 ` [PATCH -next v3 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync() Yu Kuai
2023-08-23  3:24   ` Xiao Ni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.