Messages in this thread | | | Date | Thu, 3 Aug 2023 14:49:18 +0800 | Subject | Re: [PATCH -next] md:ensure mddev->reconfig_mutex is hold when try to get mddev->sync_thread | From | Li Lingfeng <> |
| |
Thanks for your advice, I will send a new patch soon.
在 2023/7/28 15:21, Paul Menzel 写道: > 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. > This is not a bugfix patch , so I don't think it's necessary to add a fix tag. >> Signed-off-by: Li Lingfeng <lilingfeng@huaweicloud.com> >> --- >> 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;
| |