[PATCH V3 7/8] nvmet-loop: do not alloc admin tag set during reset
Maurizio Lombardi
mlombard at arkamax.eu
Tue Apr 28 04:17:17 PDT 2026
On Mon Apr 27, 2026 at 11:46 AM CEST, Daniel Wagner wrote:
> On Fri, Apr 10, 2026 at 09:39:23AM +0200, Maurizio Lombardi wrote:
>> @@ -375,12 +376,15 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
>> }
>> ctrl->ctrl.queue_count = 1;
>>
>> - error = nvme_alloc_admin_tag_set(&ctrl->ctrl, &ctrl->admin_tag_set,
>> - &nvme_loop_admin_mq_ops,
>> - sizeof(struct nvme_loop_iod) +
>> - NVME_INLINE_SG_CNT * sizeof(struct scatterlist));
>> - if (error)
>> - goto out_free_sq;
>> + if (new) {
>> + error = nvme_alloc_admin_tag_set(&ctrl->ctrl,
>> + &ctrl->admin_tag_set,
>> + &nvme_loop_admin_mq_ops,
>> + sizeof(struct nvme_loop_iod) +
>> + NVME_INLINE_SG_CNT * sizeof(struct scatterlist));
>> + if (error)
>> + goto out_free_sq;
>> + }
>>
>> /* reset stopped state for the fresh admin queue */
>> clear_bit(NVME_CTRL_ADMIN_Q_STOPPED, &ctrl->ctrl.flags);
>> @@ -415,7 +419,7 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
>> return error;
>> }
>
> Personally, I am just not a big fan of conditional creation/removal of
> resources controlled via a bool. If it's the cleanest solution fine with
> me, but I wonder if here it wouldn't be cleaner to allocated the tagset
> outside of nvme_loop_configure_admin_queue and obviously the same idea
> for destroy.
>
> Have you tried this approach, does it look too ugly?
It would be something like this (just a compile tested draft,
I still need to properly test it, but it should give you an idea):
nvmet-loop: do not alloc admin tag set during reset
Currently, resetting a loopback controller unconditionally invokes
nvme_alloc_admin_tag_set() inside nvme_loop_configure_admin_queue().
Doing so drops the old queue and allocates a new one. Consequently,
this reverts the admin queue's timeout (q->rq_timeout) back to the
module default (NVME_ADMIN_TIMEOUT), completely wiping out any custom
timeout values the user may have configured via sysfs and potentially
racing against the sysfs nvme_admin_timeout_store() function
that may dereference the admin_q pointer during the RESETTING state.
Decouple the admin tag set lifecycle from the admin queue
configuration and destruction paths, which are executed during resets;
Specifically:
* Move nvme_alloc_admin_tag_set() into nvme_loop_create_ctrl() so it
is only allocated once during the initial controller creation.
* Defer the destruction of the admin tag set to
nvme_loop_delete_ctrl_host(), the terminal error-handling
paths of nvme_loop_reset_ctrl_work() and
nvme_loop_create_ctrl().
Signed-off-by: Maurizio Lombardi <mlombard at redhat.com>
---
drivers/nvme/target/loop.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index d98d0cdc5d6f..070d16068e6b 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -274,7 +274,6 @@ static void nvme_loop_destroy_admin_queue(struct nvme_loop_ctrl *ctrl)
nvmet_sq_destroy(&ctrl->queues[0].nvme_sq);
nvmet_cq_put(&ctrl->queues[0].nvme_cq);
- nvme_remove_admin_tag_set(&ctrl->ctrl);
}
static void nvme_loop_free_ctrl(struct nvme_ctrl *nctrl)
@@ -375,25 +374,18 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
}
ctrl->ctrl.queue_count = 1;
- error = nvme_alloc_admin_tag_set(&ctrl->ctrl, &ctrl->admin_tag_set,
- &nvme_loop_admin_mq_ops,
- sizeof(struct nvme_loop_iod) +
- NVME_INLINE_SG_CNT * sizeof(struct scatterlist));
- if (error)
- goto out_free_sq;
-
/* reset stopped state for the fresh admin queue */
clear_bit(NVME_CTRL_ADMIN_Q_STOPPED, &ctrl->ctrl.flags);
error = nvmf_connect_admin_queue(&ctrl->ctrl);
if (error)
- goto out_cleanup_tagset;
+ goto out_free_sq;
set_bit(NVME_LOOP_Q_LIVE, &ctrl->queues[0].flags);
error = nvme_enable_ctrl(&ctrl->ctrl);
if (error)
- goto out_cleanup_tagset;
+ goto out_free_sq;
ctrl->ctrl.max_hw_sectors =
(NVME_LOOP_MAX_SEGMENTS - 1) << PAGE_SECTORS_SHIFT;
@@ -402,14 +394,12 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
error = nvme_init_ctrl_finish(&ctrl->ctrl, false);
if (error)
- goto out_cleanup_tagset;
+ goto out_free_sq;
return 0;
-out_cleanup_tagset:
- clear_bit(NVME_LOOP_Q_LIVE, &ctrl->queues[0].flags);
- nvme_remove_admin_tag_set(&ctrl->ctrl);
out_free_sq:
+ clear_bit(NVME_LOOP_Q_LIVE, &ctrl->queues[0].flags);
nvmet_sq_destroy(&ctrl->queues[0].nvme_sq);
nvmet_cq_put(&ctrl->queues[0].nvme_cq);
return error;
@@ -432,6 +422,7 @@ static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
static void nvme_loop_delete_ctrl_host(struct nvme_ctrl *ctrl)
{
nvme_loop_shutdown_ctrl(to_loop_ctrl(ctrl));
+ nvme_remove_admin_tag_set(ctrl);
}
static void nvme_loop_delete_ctrl(struct nvmet_ctrl *nctrl)
@@ -494,6 +485,7 @@ static void nvme_loop_reset_ctrl_work(struct work_struct *work)
nvme_cancel_admin_tagset(&ctrl->ctrl);
nvme_loop_destroy_admin_queue(ctrl);
out_disable:
+ nvme_remove_admin_tag_set(&ctrl->ctrl);
dev_warn(ctrl->ctrl.device, "Removing after reset failure\n");
nvme_uninit_ctrl(&ctrl->ctrl);
}
@@ -594,10 +586,17 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
if (!ctrl->queues)
goto out_uninit_ctrl;
- ret = nvme_loop_configure_admin_queue(ctrl);
+ ret = nvme_alloc_admin_tag_set(&ctrl->ctrl, &ctrl->admin_tag_set,
+ &nvme_loop_admin_mq_ops,
+ sizeof(struct nvme_loop_iod) +
+ NVME_INLINE_SG_CNT * sizeof(struct scatterlist));
if (ret)
goto out_free_queues;
+ ret = nvme_loop_configure_admin_queue(ctrl);
+ if (ret)
+ goto out_remove_admin_tagset;
+
if (opts->queue_size > ctrl->ctrl.maxcmd) {
/* warn if maxcmd is lower than queue_size */
dev_warn(ctrl->ctrl.device,
@@ -633,6 +632,8 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
nvme_quiesce_admin_queue(&ctrl->ctrl);
nvme_cancel_admin_tagset(&ctrl->ctrl);
nvme_loop_destroy_admin_queue(ctrl);
+out_remove_admin_tagset:
+ nvme_remove_admin_tag_set(&ctrl->ctrl);
out_free_queues:
kfree(ctrl->queues);
out_uninit_ctrl:
--
2.53.0
More information about the Linux-nvme
mailing list