| From b7751788a18361ed5d4dc91911e900f0f25465c9 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Wed, 31 Jan 2024 09:51:01 +0100 |
| Subject: nvme-fc: do not wait in vain when unloading module |
| |
| From: Daniel Wagner <dwagner@suse.de> |
| |
| [ Upstream commit 70fbfc47a392b98e5f8dba70c6efc6839205c982 ] |
| |
| The module exit path has race between deleting all controllers and |
| freeing 'left over IDs'. To prevent double free a synchronization |
| between nvme_delete_ctrl and ida_destroy has been added by the initial |
| commit. |
| |
| There is some logic around trying to prevent from hanging forever in |
| wait_for_completion, though it does not handling all cases. E.g. |
| blktests is able to reproduce the situation where the module unload |
| hangs forever. |
| |
| If we completely rely on the cleanup code executed from the |
| nvme_delete_ctrl path, all IDs will be freed eventually. This makes |
| calling ida_destroy unnecessary. We only have to ensure that all |
| nvme_delete_ctrl code has been executed before we leave |
| nvme_fc_exit_module. This is done by flushing the nvme_delete_wq |
| workqueue. |
| |
| While at it, remove the unused nvme_fc_wq workqueue too. |
| |
| Reviewed-by: Christoph Hellwig <hch@lst.de> |
| Reviewed-by: Hannes Reinecke <hare@suse.de> |
| Signed-off-by: Daniel Wagner <dwagner@suse.de> |
| Signed-off-by: Keith Busch <kbusch@kernel.org> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| drivers/nvme/host/fc.c | 47 ++++++------------------------------------ |
| 1 file changed, 6 insertions(+), 41 deletions(-) |
| |
| diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c |
| index 177a365b8ec55..3dbf926fd99fd 100644 |
| --- a/drivers/nvme/host/fc.c |
| +++ b/drivers/nvme/host/fc.c |
| @@ -221,11 +221,6 @@ static LIST_HEAD(nvme_fc_lport_list); |
| static DEFINE_IDA(nvme_fc_local_port_cnt); |
| static DEFINE_IDA(nvme_fc_ctrl_cnt); |
| |
| -static struct workqueue_struct *nvme_fc_wq; |
| - |
| -static bool nvme_fc_waiting_to_unload; |
| -static DECLARE_COMPLETION(nvme_fc_unload_proceed); |
| - |
| /* |
| * These items are short-term. They will eventually be moved into |
| * a generic FC class. See comments in module init. |
| @@ -255,8 +250,6 @@ nvme_fc_free_lport(struct kref *ref) |
| /* remove from transport list */ |
| spin_lock_irqsave(&nvme_fc_lock, flags); |
| list_del(&lport->port_list); |
| - if (nvme_fc_waiting_to_unload && list_empty(&nvme_fc_lport_list)) |
| - complete(&nvme_fc_unload_proceed); |
| spin_unlock_irqrestore(&nvme_fc_lock, flags); |
| |
| ida_free(&nvme_fc_local_port_cnt, lport->localport.port_num); |
| @@ -3869,10 +3862,6 @@ static int __init nvme_fc_init_module(void) |
| { |
| int ret; |
| |
| - nvme_fc_wq = alloc_workqueue("nvme_fc_wq", WQ_MEM_RECLAIM, 0); |
| - if (!nvme_fc_wq) |
| - return -ENOMEM; |
| - |
| /* |
| * NOTE: |
| * It is expected that in the future the kernel will combine |
| @@ -3890,7 +3879,7 @@ static int __init nvme_fc_init_module(void) |
| ret = class_register(&fc_class); |
| if (ret) { |
| pr_err("couldn't register class fc\n"); |
| - goto out_destroy_wq; |
| + return ret; |
| } |
| |
| /* |
| @@ -3914,8 +3903,6 @@ static int __init nvme_fc_init_module(void) |
| device_destroy(&fc_class, MKDEV(0, 0)); |
| out_destroy_class: |
| class_unregister(&fc_class); |
| -out_destroy_wq: |
| - destroy_workqueue(nvme_fc_wq); |
| |
| return ret; |
| } |
| @@ -3935,45 +3922,23 @@ nvme_fc_delete_controllers(struct nvme_fc_rport *rport) |
| spin_unlock(&rport->lock); |
| } |
| |
| -static void |
| -nvme_fc_cleanup_for_unload(void) |
| +static void __exit nvme_fc_exit_module(void) |
| { |
| struct nvme_fc_lport *lport; |
| struct nvme_fc_rport *rport; |
| - |
| - list_for_each_entry(lport, &nvme_fc_lport_list, port_list) { |
| - list_for_each_entry(rport, &lport->endp_list, endp_list) { |
| - nvme_fc_delete_controllers(rport); |
| - } |
| - } |
| -} |
| - |
| -static void __exit nvme_fc_exit_module(void) |
| -{ |
| unsigned long flags; |
| - bool need_cleanup = false; |
| |
| spin_lock_irqsave(&nvme_fc_lock, flags); |
| - nvme_fc_waiting_to_unload = true; |
| - if (!list_empty(&nvme_fc_lport_list)) { |
| - need_cleanup = true; |
| - nvme_fc_cleanup_for_unload(); |
| - } |
| + list_for_each_entry(lport, &nvme_fc_lport_list, port_list) |
| + list_for_each_entry(rport, &lport->endp_list, endp_list) |
| + nvme_fc_delete_controllers(rport); |
| spin_unlock_irqrestore(&nvme_fc_lock, flags); |
| - if (need_cleanup) { |
| - pr_info("%s: waiting for ctlr deletes\n", __func__); |
| - wait_for_completion(&nvme_fc_unload_proceed); |
| - pr_info("%s: ctrl deletes complete\n", __func__); |
| - } |
| + flush_workqueue(nvme_delete_wq); |
| |
| nvmf_unregister_transport(&nvme_fc_transport); |
| |
| - ida_destroy(&nvme_fc_local_port_cnt); |
| - ida_destroy(&nvme_fc_ctrl_cnt); |
| - |
| device_destroy(&fc_class, MKDEV(0, 0)); |
| class_unregister(&fc_class); |
| - destroy_workqueue(nvme_fc_wq); |
| } |
| |
| module_init(nvme_fc_init_module); |
| -- |
| 2.43.0 |
| |