Re: [PATCH -next] md:ensure mddev->reconfig_mutex is hold when try to get mddev->sync_thread

Paul Menzel <pmenzel@xxxxxxxxxxxxx> · Fri, 28 Jul 2023 09:21:31 +0200

Dear Li,


Thank you for your patch.

I notice two minor things in the summary:

1.  Please add a space after the colon
2.  “is hold” should be “is held”.

Maybe even shorter:

md: Hold mddev->reconfig_mutex when trying to get mddev->sync_thread

Am 27.07.23 um 09:20 schrieb Li Lingfeng:
Commit ba9d9f1a707f ("Revert "md: unlock mddev before reap sync_thread in
action_store"") removed the scenario of calling md_unregister_thread()
without holding mddev->reconfig_mutex, so add a lock holding check before
acquiring mddev->sync_thread.

Maybe also mention, that this is done by passing `mdev` to 
`md_unregister_thread()`.

Plesae add a Fixes: tag.

Signed-off-by: Li Lingfeng <lilingfeng@xxxxxxxxxxxxxxx>
---
  drivers/md/md-cluster.c  | 8 ++++----
  drivers/md/md.c          | 9 +++++----
  drivers/md/md.h          | 2 +-
  drivers/md/raid1.c       | 4 ++--
  drivers/md/raid10.c      | 2 +-
  drivers/md/raid5-cache.c | 2 +-
  drivers/md/raid5.c       | 2 +-
  7 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 3d9fd74233df..1e26eb223349 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -952,8 +952,8 @@ static int join(struct mddev *mddev, int nodes)
  	return 0;
  err:
  	set_bit(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state);
-	md_unregister_thread(&cinfo->recovery_thread);
-	md_unregister_thread(&cinfo->recv_thread);
+	md_unregister_thread(mddev, &cinfo->recovery_thread);
+	md_unregister_thread(mddev, &cinfo->recv_thread);
  	lockres_free(cinfo->message_lockres);
  	lockres_free(cinfo->token_lockres);
  	lockres_free(cinfo->ack_lockres);
@@ -1015,8 +1015,8 @@ static int leave(struct mddev *mddev)
  		resync_bitmap(mddev);
  
  	set_bit(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state);
-	md_unregister_thread(&cinfo->recovery_thread);
-	md_unregister_thread(&cinfo->recv_thread);
+	md_unregister_thread(mddev, &cinfo->recovery_thread);
+	md_unregister_thread(mddev, &cinfo->recv_thread);
  	lockres_free(cinfo->message_lockres);
  	lockres_free(cinfo->token_lockres);
  	lockres_free(cinfo->ack_lockres);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index a3d98273b295..5c3c19b8d509 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6258,7 +6258,7 @@ static void mddev_detach(struct mddev *mddev)
  		mddev->pers->quiesce(mddev, 1);
  		mddev->pers->quiesce(mddev, 0);
  	}
-	md_unregister_thread(&mddev->thread);
+	md_unregister_thread(mddev, &mddev->thread);
  	if (mddev->queue)
  		blk_sync_queue(mddev->queue); /* the unplug fn references 'conf'*/
  }
@@ -7990,9 +7990,10 @@ struct md_thread *md_register_thread(void (*run) (struct md_thread *),
  }
  EXPORT_SYMBOL(md_register_thread);
  
-void md_unregister_thread(struct md_thread __rcu **threadp)
+void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **threadp)
  {
-	struct md_thread *thread = rcu_dereference_protected(*threadp, true);
+	struct md_thread *thread = rcu_dereference_protected(*threadp,
+					lockdep_is_held(&mddev->reconfig_mutex));
  
  	if (!thread)
  		return;
@@ -9484,7 +9485,7 @@ void md_reap_sync_thread(struct mddev *mddev)
  	bool is_reshaped = false;
  
  	/* resync has finished, collect result */
-	md_unregister_thread(&mddev->sync_thread);
+	md_unregister_thread(mddev, &mddev->sync_thread);
  	atomic_inc(&mddev->sync_seq);
  
  	if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) &&
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 8ae957480976..9bcb77bca963 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -761,7 +761,7 @@ extern struct md_thread *md_register_thread(
  	void (*run)(struct md_thread *thread),
  	struct mddev *mddev,
  	const char *name);
-extern void md_unregister_thread(struct md_thread __rcu **threadp);
+extern void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **threadp);
  extern void md_wakeup_thread(struct md_thread __rcu *thread);
  extern void md_check_recovery(struct mddev *mddev);
  extern void md_reap_sync_thread(struct mddev *mddev);
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 23d211969565..581dfbdfca89 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -3152,7 +3152,7 @@ static int raid1_run(struct mddev *mddev)
  	 * RAID1 needs at least one disk in active
  	 */
  	if (conf->raid_disks - mddev->degraded < 1) {
-		md_unregister_thread(&conf->thread);
+		md_unregister_thread(mddev, &conf->thread);
  		ret = -EINVAL;
  		goto abort;
  	}
@@ -3179,7 +3179,7 @@ static int raid1_run(struct mddev *mddev)
  
  	ret = md_integrity_register(mddev);
  	if (ret) {
-		md_unregister_thread(&mddev->thread);
+		md_unregister_thread(mddev, &mddev->thread);
  		goto abort;
  	}
  	return 0;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 16aa9d735880..6188b71186f4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4320,7 +4320,7 @@ static int raid10_run(struct mddev *mddev)
  	return 0;
  
  out_free_conf:
-	md_unregister_thread(&mddev->thread);
+	md_unregister_thread(mddev, &mddev->thread);
  	raid10_free_conf(conf);
  	mddev->private = NULL;
  out:
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 47ba7d9e81e1..ce9b42fd54b9 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -3171,7 +3171,7 @@ void r5l_exit_log(struct r5conf *conf)
  	/* Ensure disable_writeback_work wakes up and exits */
  	wake_up(&conf->mddev->sb_wait);
  	flush_work(&log->disable_writeback_work);
-	md_unregister_thread(&log->reclaim_thread);
+	md_unregister_thread(conf->mddev, &log->reclaim_thread);
  
  	conf->log = NULL;
  
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 4cdb35e54251..f41f9b712d3d 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8107,7 +8107,7 @@ static int raid5_run(struct mddev *mddev)
  
  	return 0;
  abort:
-	md_unregister_thread(&mddev->thread);
+	md_unregister_thread(mddev, &mddev->thread);
  	print_raid5_conf(conf);
  	free_conf(conf);
  	mddev->private = NULL;






