nvme-fabrics: Add host support for FC transport
Subsystems (2)
PATCH v3 5/7 nvme-fabrics: Add host support for FC transport
Add nvme-fabrics host support for FC transport
Implements the FC-NVME T11 definition of how nvme fabric capsules are performed on an FC fabric. Utilizes a lower-layer API to FC host adapters to send/receive FC-4 LS operations and FCP operations that comprise NVME over FC operation.
The T11 definitions for FC-4 Link Services are implemented which create NVMeOF connections. Implements the hooks with blk-mq to then submit admin and io requests to the different connections.
--- MAINTAINERS | 1 + drivers/nvme/host/Kconfig | 16 + drivers/nvme/host/Makefile | 3 + drivers/nvme/host/fc.c | 2351 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 2371 insertions(+) create mode 100644 drivers/nvme/host/fc.c
MAINTAINERS +1 -0
@@ -8665,6 +8665,7 @@ L: linux-nvme at lists.infradead.org S: Supported F: include/linux/nvme-fc.h F: include/linux/nvme-fc-driver.h +F: drivers/nvme/host/fc.c NVMEM FRAMEWORK M: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
drivers/nvme/host/Kconfig +16 -0
@@ -43,3 +43,19 @@ config NVME_RDMA from https://github.com/linux-nvme/nvme-cli. If unsure, say N. + +config NVME_FC + tristate "NVM Express over Fabrics FC host driver" + depends on BLOCK + select NVME_CORE + select NVME_FABRICS + select SG_POOL + help + This provides support for the NVMe over Fabrics protocol using + the FC transport. This allows you to use remote block devices + exported using the NVMe protocol set. + + To configure a NVMe over Fabrics controller use the nvme-cli tool + from https://github.com/linux-nvme/nvme-cli. + + If unsure, say N.
drivers/nvme/host/Makefile +3 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_NVME_CORE) += nvme-core.o obj-$(CONFIG_BLK_DEV_NVME) += nvme.o obj-$(CONFIG_NVME_FABRICS) += nvme-fabrics.o obj-$(CONFIG_NVME_RDMA) += nvme-rdma.o +obj-$(CONFIG_NVME_FC) += nvme-fc.o nvme-core-y := core.o nvme-core-$(CONFIG_BLK_DEV_NVME_SCSI) += scsi.o
@@ -12,3 +13,5 @@ nvme-y += pci.o nvme-fabrics-y += fabrics.o nvme-rdma-y += rdma.o + +nvme-fc-y += fc.o
drivers/nvme/host/fc.c new file +2351 -0
2,351 lines not on this page · raw message
Signature · 1 line
2.5.0
PATCH v3 5/7 nvme-fabrics: Add host support for FC transport
drivers/nvme/host/Kconfig config NVME_RDMA · line 52
Quoted 6 lines
drivers/nvme/host/Kconfig +6
-0+config NVME_FC + tristate "NVM Express over Fabrics FC host driver" + depends on BLOCK + select NVME_CORE + select NVME_FABRICS + select SG_POOL
needs HAS_DMA, but I already saw the patch for that.
drivers/nvme/host/fc.c line 1019
Quoted 8 lines
drivers/nvme/host/fc.c new file +8
-0+/* +* WARNING: +* The current linux implementation of a nvme controller +* allocates a single tag set for all io queues and sizes +* the io queues to fully hold all possible tags. Thus, the +* implementation does not reference or care about the sqhd +* value as it never needs to use the sqhd/sqtail pointers +* for submission pacing.
I've just send out two patches that get rid of the concept of a CQE in the core. I think all this code would benefit a lot from being rebase on top of that.
drivers/nvme/host/fc.c line 1087
Quoted 3 lines
drivers/nvme/host/fc.c new file +3
-0+if (unlikely (be16_to_cpu (op->rsp_iu.iu_len) != + (freq->rcv_rsplen/4 )) || +unlikely (op->rqno !=le16_to_cpu (cqe->command_id))) {
nitpick: missing whitespace around the operand, and not needed braces around the condition.
Also we usually use a single unlikely for the whole condition.
Quoted 8 lines
drivers/nvme/host/fc.c new file +8
-0+static void +nvme_fc_kill_request (struct request *req,void *data,bool reserved) +{ +struct nvme_ctrl *nctrl = data; +struct nvme_fc_ctrl *ctrl =to_fc_ctrl (nctrl); +struct nvme_fc_fcp_op *op =blk_mq_rq_to_pdu (req); +int status; +
drivers/nvme/host/fc.c line 1888
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+if (!blk_mq_request_started (req)) +return ; + + status =__nvme_fc_abort_op (ctrl, op); +if (status) { +/* io wasn't active to abort consider it done */ +/* assume completion path already completing in parallel */ +return ; + } +}
This looks odd - the point of the blk_mq_tagset_busy_iter loops is return the in-core request to the caller. The controller shutdown following this would take care of invalidating anything on the wire.
I have to admit that I'm still a bit confused how the FC abort fits into the whole NVMe architecture model.
PATCH v3 5/7 nvme-fabrics: Add host support for FC transport
Quoted 4 lines · Christoph Hellwig
I've just send out two patches that get rid of the concept of a CQE in the core. I think all this code would benefit a lot from being rebase on top of that.
Ok. Done. Didn't change things that much. Mainly eliminated some memsets/memcpy's. But is better.
Quoted 23 lines
Quoted 9 lines
drivers/nvme/host/fc.c new file +8
-0+static void +nvme_fc_kill_request (struct request *req,void *data,bool reserved) +{ +struct nvme_ctrl *nctrl = data; +struct nvme_fc_ctrl *ctrl =to_fc_ctrl (nctrl); +struct nvme_fc_fcp_op *op =blk_mq_rq_to_pdu (req); +int status; +drivers/nvme/host/fc.c line 1888
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+if (!blk_mq_request_started (req)) +return ; + + status =__nvme_fc_abort_op (ctrl, op); +if (status) { +/* io wasn't active to abort consider it done */ +/* assume completion path already completing in parallel */ +return ; + } +}This looks odd - the point of the
blk_mq_tagset_busy_iterloops is return the in-core request to the caller. The controller shutdown following this would take care of invalidating anything on the wire.
Quoted 2 lines
I have to admit that I'm still a bit confused how the FC abort fits into the whole NVMe architecture model.
Well, I used blk_mq_tagset_busy_iter a bit differently than I saw it used on rdma.
With RDMA, I saw it using the routine to call the core nvme_cancel_request, which indeed passed the request back up to the blk layer. It then scheduled the reconnect_ctrl_work routine, which would terminate the queues, tearing down the qp's. I assume the teardown of the qp's also terminated all the memory regions that may have been mapped for ios sent over the qps. I also assume the tgt side, upon the qp termination, also tears down and resolves any io state relative to
the qp, io-related mrs for the qp, etc. Note: I'm actually surprised.
I would have assumed it's not good practice to return things to the blk layer while there may be things coming off the wire that hit memory registrations and perhaps muck with memory. I assume either teardown is too quick or they aren't destructive accesses (memory related to the request's sgl stays around long enough that the cancelations finish before the request releases it, etc).
In FC, the queue is a logical thing, unlike rdma where it maps to the qp. At transport connect, the target gives the initiator a handle for the queue that the initiator is to pair on any io it sends to the
target. When an SQE is sent on a SQ, FC effectively considers the SQE,
or perhaps better said, the command within the SQE, an "io", and assigns a FC exchange to it. The SQE, and the queue handle, get sent in the initial CMD IU on the exchange. Everything relative to the io happens under the exchange context. The CQE is the last thing for the io, and the transport closes the exchange upon the commands-CQE being delivered.
So - if I'm tearing down queues/resetting controllers, not only do I send a DISCONNECT LS to kill the validity of the handle, but I also need to do something to cancel all those FC exchange contexts that corresponded to ios that were outstanding on the queue.
Given those exchange contexts are tied to LLDD contexts that also have dma resources mapped to them - it isn't correct for me to just return the requests back to the request queue. I want to cancel them in the LLDD, stopping any potential dma accessess and giving the resources back
to the LLDD, then pass the request back to the blk layer. So, I
replaced the blk_mq_tagset_busy_iter() with a local routine that kicks off the callbacks into the LLDD to cancel the jobs. From there, the LLDD will "finish" the jobs in a normal manner, getting them back to the blk layer.
As for aborts relative to individual io timeouts, the "normal thing" we did with scsi and other drive protocols, it really doesn't fit and the nvme escalation policy of failing the queue or resetting the controller should kick in (like on rdma). The FC transport could kill the one I/O, but as the SQE and CQEs are part of the io - there's queue-related things it messes with when it does so that aren't good. Granted linux ignores some of those making it tempting, but I don't want to go there. If anything were to be done - it should be something generic: have a core layer time out function that then generates an ABORT admin command to kill it. And if that fails or timesout, then it resorts to a transport controller reset routine.
hope that helps
-- james
PATCH v3 5/7 nvme-fabrics: Add host support for FC transport
Quoted 2 lines · James Smart
Well, I used
blk_mq_tagset_busy_itera bit differently than I saw it used on rdma.
Note that this code didn't come from rdma, but PCIe originally. And I had actually planned to move this into common code, but got sidetracked.
Quoted 5 lines
With RDMA, I saw it using the routine to call the core
nvme_cancel_request, which indeed passed the request back up to the blk layer. It then scheduled thereconnect_ctrl_workroutine, which would terminate the queues, tearing down the qp's. I assume the teardown of the qp's also terminated all the memory regions that may have been mapped for ios sent over the qps.
So on RDMA the QP itself doesn't reference physical memory - the send and recv WRs do. But a QP termination "flushes" all them, that is completes them with an error.
Quoted 9 lines
I also
assume the tgt side, upon the qp termination, also tears down and resolves
any io state relative to the qp, io-related mrs for the qp, etc. Note: I'm
actually surprised. I would have assumed it's not good practice to return things to the blk layer while there may be things coming off the wire that hit memory registrations and perhaps muck with memory. I assume either teardown is too quick or they aren't destructive accesses (memory related to the request's sgl stays around long enough that the cancelations finish before the request releases it, etc).
The queues are dead on the wire before we call nvme_cancel_request for both RDMA and PCI - the call to blk_mq_tagset_busy_iter is for requests we did not get a reply to before tearing down the queues, or those stuck being requeued in the block layer.
Quoted 10 lines
In FC, the queue is a logical thing, unlike rdma where it maps to the qp. At transport connect, the target gives the initiator a handle for the queue
that the initiator is to pair on any io it sends to the target. When an
SQE is sent on a SQ, FC effectively considers the SQE, or perhaps better said, the command within the SQE, an "io", and assigns a FC exchange to it. The SQE, and the queue handle, get sent in the initial CMD IU on the exchange. Everything relative to the io happens under the exchange context. The CQE is the last thing for the io, and the transport closes the exchange upon the commands-CQE being delivered.
Quoted 4 lines
So - if I'm tearing down queues/resetting controllers, not only do I send a DISCONNECT LS to kill the validity of the handle, but I also need to do something to cancel all those FC exchange contexts that corresponded to ios that were outstanding on the queue.
Ok, this makes more sense now. Can you refactor and comment the code a bit to make this more obvious?
Quoted 10 lines
As for aborts relative to individual io timeouts, the "normal thing" we did with scsi and other drive protocols, it really doesn't fit and the nvme escalation policy of failing the queue or resetting the controller should kick in (like on rdma). The FC transport could kill the one I/O, but as the SQE and CQEs are part of the io - there's queue-related things it messes with when it does so that aren't good. Granted linux ignores some of those making it tempting, but I don't want to go there. If anything were to be done - it should be something generic: have a core layer time out function that then generates an ABORT admin command to kill it. And if that fails or timesout, then it resorts to a transport controller reset routine.
The PCIe code does send aborts, but the problem with the NVMe abort command is that it's almost entirely useless - controllers can ignore it for any reason whatsover, and the spec only requires controllers to support 4 outstanding abort commands at a given time.
For now I haven't bothered to implement abort on RDMA for that reason, but I reall want a common error handler rather sooner than later instead of duplicating it in every driver. Given that Keith really likes the aborts because he looks at them in the bus analyzer traces we'll probably send them for RDMA as well in the future.
that beeing said the way I read nvme_fc_timeout in your patch is that it does the FCP abort for just that one command, but never escalates to a controller reset. It also completely ignores the reserved commands (which would be keep alive). So looking over this in a little more detail I think FC error handling needs at least a bit more work to force a controller reset.
PATCH v3 5/7 nvme-fabrics: Add host support for FC transport
In reply to James SmartQuoted 2 lines · James Smart
Add nvme-fabrics host support for FC transport
snip...
Quoted 97 lines
(quoted diff) +2
-0+static int +nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue*queue,
drivers/nvme/host/fc.c new file +36
-0+ struct nvme_fc_fcp_op *op, u32 data_len, + enum nvmefc_fcp_datadir io_dir) +{ + struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu; +struct nvme_command *sqe = &cmdiu->sqe; +u32 csn; +int ret; + +if (!nvme_fc_ctrl_get (ctrl)) +return BLK_MQ_RQ_QUEUE_ERROR ; + +/* format the FC-NVME CMD IU and fcp_req */ + cmdiu->connection_id =cpu_to_be64 (queue->connection_id); + csn =atomic_inc_return (&queue->csn); + cmdiu->csn =cpu_to_be32 (csn); + cmdiu->data_len =cpu_to_be32 (data_len); +switch (io_dir) { +case NVMEFC_FCP_WRITE : + cmdiu->flags =FCNVME_CMD_FLAGS_WRITE ; +break ; +case NVMEFC_FCP_READ : + cmdiu->flags =FCNVME_CMD_FLAGS_READ ; +break ; +case NVMEFC_FCP_NODATA : + cmdiu->flags =0 ; +break ; + } + op->fcp_req.payload_length = data_len; + op->fcp_req.io_dir = io_dir; + op->fcp_req.transferred_length =0 ; + op->fcp_req.rcv_rsplen =0 ; + op->fcp_req.status =0 ; + op->fcp_req.sqid =cpu_to_le16 (queue->qnum); + +/* +?* validate per fabric rules, set fields mandated by fabric spec
drivers/nvme/host/fc.c new file +17
-0+?* as well as those by FC-NVME spec. +?*/ +WARN_ON_ONCE (sqe->common.metadata); +WARN_ON_ONCE (sqe->common.dptr.prp1); +WARN_ON_ONCE (sqe->common.dptr.prp2); + sqe->common.flags |=NVME_CMD_SGL_METABUF ; + +/* +?* format SQE DPTR field per FC-NVME rules +?*????type=data block descr; subtype=offset; +?*????offset is currently 0. +?*/ + sqe->rw.dptr.sgl.type =NVME_SGL_FMT_OFFSET ; + sqe->rw.dptr.sgl.length =cpu_to_le32 (data_len); + sqe->rw.dptr.sgl.addr =0 ; + +/* odd that we set the command_id - should come from nvme- fabrics */
(quoted diff) +1
-0+ WARN_ON_ONCE(sqe->common.command_id != cpu_to_le16(op-Quoted 1 line
rqno));
(quoted diff) +2
-0+ + if (op->rq) { /* skipped onaens */
drivers/nvme/host/fc.c new file +8
-0+ ret =nvme_fc_map_data (ctrl, op->rq, op); +if (ret <0 ) { +dev_err (queue->ctrl->ctrl.device, + ?????"Failed to map data (%d)\n", ret); +nvme_cleanup_cmd (op->rq); +nvme_fc_ctrl_put (ctrl); +return (ret == -ENOMEM || ret == -EAGAIN ) ? + BLK_MQ_RQ_QUEUE_BUSY :
BLK_MQ_RQ_QUEUE_ERROR;drivers/nvme/host/fc.c new file +4
-0+ } + } + + dma_sync_single_for_device(ctrl->lport->dev, op-Quoted 1 line
fcp_req.cmddma,
(quoted diff) +1
-0+ ??sizeof(op->cmd_iu),
DMA_TO_DEVICE);drivers/nvme/host/fc.c new file +9
-0+ +atomic_set (&op->state,FCPOP_STATE_ACTIVE ); + +if (op->rq) +blk_mq_start_request (op->rq); + + ret = ctrl->lport->ops->fcp_io(&ctrl->lport->localport, + &ctrl->rport->remoteport, + queue->lldd_handle, &op-Quoted 1 line
fcp_req);drivers/nvme/host/fc.c new file +4
-0+ +if (ret) { + dev_err(ctrl->dev, + "Send nvme command failed - lldd returned%d.\n", ret);
(quoted diff) +1
-0+ if (op->rq) { /* normalrequest */
drivers/nvme/host/fc.c new file +1
-0+nvme_fc_unmap_data (ctrl, op->rq, op);
drivers/nvme/host/fc.c line 1645
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+nvme_cleanup_cmd (op->rq); + } +/* else - aen. no cleanup needed */ + +nvme_fc_ctrl_put (ctrl); + +if (ret != -EBUSY ) +return BLK_MQ_RQ_QUEUE_ERROR ; + +blk_mq_stop_hw_queues (op->rq->q);
It looks like 'if(op->rq)' is checked for NULL before this 'if (ret)' block, but it's theoretically possible 'if(ret)' is true and things completely blow up here by a op->rq de-reference? ?
Quoted 6 lines
drivers/nvme/host/fc.c new file +6
-0+blk_mq_delay_queue (queue->hctx,NVMEFC_QUEUE_DELAY ); +return BLK_MQ_RQ_QUEUE_BUSY ; + } + +return BLK_MQ_RQ_QUEUE_OK ; +}
snip...
drivers/nvme/host/fc.c line 1462
Quoted 11 lines
?
(quoted diff) +2
-0+static int +__nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op*op)
drivers/nvme/host/fc.c new file +7
-0+{ +int state; + + state =atomic_xchg (&op->state,FCPOP_STATE_ABORTED ); +if (state !=FCPOP_STATE_ACTIVE ) { +atomic_set (&op->state, state); +return 1 ;/* fail */
nitpick: Why would this function return a positive value for an error condition? ?Maybe use "-EACCES" instead?
Quoted 18 lines
drivers/nvme/host/fc.c new file +18
-0+ } + + ctrl->lport->ops->fcp_abort (&ctrl->lport->localport, + &ctrl->rport->remoteport, + op->queue->lldd_handle, + &op->fcp_req); + +return 0 ; +} + +enum blk_eh_timer_return +nvme_fc_timeout (struct request *rq,bool reserved) +{ +struct nvme_fc_fcp_op *op =blk_mq_rq_to_pdu (rq); +struct nvme_fc_ctrl *ctrl = op->ctrl; +int ret; + +if (reserved)
drivers/nvme/host/fc.c line 1490
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+return BLK_EH_RESET_TIMER ; + + ret =__nvme_fc_abort_op (ctrl, op); +if (ret) +/* io wasn't active to abort consider it done */ +return BLK_EH_HANDLED ; + +/* TODO: force a controller reset */ + +return BLK_EH_HANDLED ;
Another nitpick- that 'if(ret)' statement isn't adding any value, the function returns BLK_EH_HANDLED regardless if the conditional is taken.
snip...
Quoted 149 lines
?
?drivers/nvme/host/fc.c new file +3
-0+ +static struct nvme_ctrl * +__nvme_fc_create_ctrl(struct device *dev, struct nvmf_ctrl_options*opts,
drivers/nvme/host/fc.c new file +29
-0+ struct nvme_fc_lport *lport, struct nvme_fc_rport *rport) +{ + struct nvme_fc_ctrl *ctrl; +unsigned long flags; +int ret, idx; +bool changed; + + ctrl =kzalloc (sizeof (*ctrl),GFP_KERNEL ); +if (!ctrl) { + ret = -ENOMEM ; + goto out_fail; + } + + idx =ida_simple_get (&nvme_fc_ctrl_cnt,0 ,0 ,GFP_KERNEL ); +if (idx <0 ) { + ret = -ENOSPC ; + goto out_free_ctrl; + } + + ctrl->ctrl.opts = opts; +INIT_LIST_HEAD (&ctrl->ctrl_list); +INIT_LIST_HEAD (&ctrl->ls_req_list); + ctrl->lport = lport; + ctrl->rport = rport; + ctrl->dev = lport->dev; + ctrl->state =FCCTRL_INIT ; + ctrl->cnum = idx; + + ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_fc_ctrl_ops,0);
drivers/nvme/host/fc.c new file +14
-0+if (ret) + goto out_free_ida; + +get_device (ctrl->dev); +kref_init (&ctrl->ref); + +INIT_WORK (&ctrl->delete_work, nvme_fc_del_ctrl_work); +spin_lock_init (&ctrl->lock); + +/* io queue count */ + ctrl->queue_count = min_t(unsigned int, + opts->nr_io_queues, + lport->ops->max_hw_queues); + opts->nr_io_queues = ctrl->queue_count;/* so opts has valid value */
drivers/nvme/host/fc.c new file +7
-0+ ctrl->queue_count++;/* +1 for admin queue */ + + ctrl->ctrl.sqsize = opts->queue_size -1 ; + ctrl->ctrl.kato = opts->kato; + + ret = -ENOMEM ; + ctrl->queues = kcalloc(ctrl->queue_count, sizeof(struct
nvme_fc_queue),drivers/nvme/host/fc.c new file +13
-0+ GFP_KERNEL); +if (!ctrl->queues) + goto out_uninit_ctrl; + + ret =nvme_fc_configure_admin_queue (ctrl); +if (ret) + goto out_uninit_ctrl; + +/* sanity checks */ + +/* FC-NVME supports 64-byte SQE only */ +if (ctrl->ctrl.ioccsz !=4 ) { + dev_err(ctrl->ctrl.device, "ioccsz %d is notsupported!\n",
drivers/nvme/host/fc.c new file +6
-0+ ctrl->ctrl.ioccsz); + goto out_remove_admin_queue; + } +/* FC-NVME supports 16-byte CQE only */ +if (ctrl->ctrl.iorcsz !=1 ) { + dev_err(ctrl->ctrl.device, "iorcsz %d is notsupported!\n",
drivers/nvme/host/fc.c new file +6
-0+ ctrl->ctrl.iorcsz); + goto out_remove_admin_queue; + } +/* FC-NVME does not have other data in the capsule */ +if (ctrl->ctrl.icdoff) { + dev_err(ctrl->ctrl.device, "icdoff %d is notsupported!\n",
drivers/nvme/host/fc.c new file +30
-0+ ctrl->ctrl.icdoff); + goto out_remove_admin_queue; + } + +/* FC-NVME supports normal SGL Data Block Descriptors */ + +if (opts->queue_size > ctrl->ctrl.maxcmd) { +/* warn if maxcmd is lower than queue_size */ +dev_warn (ctrl->ctrl.device, +"queue_size %zu > ctrl maxcmd %u, reducing " +"to queue_size\n" , + opts->queue_size, ctrl->ctrl.maxcmd); + opts->queue_size = ctrl->ctrl.maxcmd; + } + + ret =nvme_fc_init_aen_ops (ctrl); +if (ret) + goto out_exit_aen_ops; + +if (ctrl->queue_count >1 ) { + ret =nvme_fc_create_io_queues (ctrl); +if (ret) + goto out_exit_aen_ops; + } + +spin_lock_irqsave (&ctrl->lock, flags); + ctrl->state =FCCTRL_ACTIVE ; +spin_unlock_irqrestore (&ctrl->lock, flags); + + changed = nvme_change_ctrl_state(&ctrl->ctrl,
NVME_CTRL_LIVE);drivers/nvme/host/fc.c new file +30
-0+WARN_ON_ONCE (!changed); + +dev_info (ctrl->ctrl.device, +"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n" , + ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl); + +kref_get (&ctrl->ctrl.kref); + +spin_lock_irqsave (&rport->lock, flags); +list_add_tail (&ctrl->ctrl_list, &rport->ctrl_list); +spin_unlock_irqrestore (&rport->lock, flags); + +if (opts->nr_io_queues) { +nvme_queue_scan (&ctrl->ctrl); +nvme_queue_async_events (&ctrl->ctrl); + } + +return &ctrl->ctrl; + +out_exit_aen_ops: +nvme_fc_exit_aen_ops (ctrl); +out_remove_admin_queue: +nvme_stop_keep_alive (&ctrl->ctrl); +nvme_fc_destroy_admin_queue (ctrl); +out_uninit_ctrl: +nvme_uninit_ctrl (&ctrl->ctrl); +nvme_put_ctrl (&ctrl->ctrl); +if (ret >0 ) + ret = -EIO ; +/* exit via here will follow ctlr ref point callbacks to free */
Review
Quoted 1 line
(quoted diff) +1
-0+ return ERR_PTR(ret);
Memory leak?? kfree(ctrl); i.e. the "struct_nvme_fc_ctrl *ctrl;" is not called before the return in this case I think?
Minor, so otherwise looks ok (minus Christoph's comments).
PATCH v3 5/7 nvme-fabrics: Add host support for FC transport
Quoted 3 lines · J Freyensee
It looks like '
if(op->rq)' is checked for NULL before this 'if (ret)' block, but it's theoretically possible 'if(ret)' is true and things completely blow up here by aop->rqde-reference?
yes - very good point.
Quoted 11 lines
drivers/nvme/host/fc.c line 1490
Quoted 9 lines
drivers/nvme/host/fc.c new file +8
-0+ +if (ret) +/* io wasn't active to abort consider it done */ +return BLK_EH_HANDLED ; + +/* TODO: force a controller reset */ + +return BLK_EH_HANDLED ;Another nitpick- that 'if(ret)' statement isn't adding any value, the function returns
BLK_EH_HANDLEDregardless if the conditional is taken.
Agreed, but leaving it in. The TODO will be addressed in a subsequent patch and it'll make sense then.
Quoted 13 lines
Quoted 13 lines
drivers/nvme/host/fc.c new file +3
-0+ +static struct nvme_ctrl * +__nvme_fc_create_ctrl(struct device *dev, struct nvmf_ctrl_options*opts,
drivers/nvme/host/fc.c new file +7
-0+ struct nvme_fc_lport *lport, struct nvme_fc_rport *rport) +{ + struct nvme_fc_ctrl *ctrl; +unsigned long flags; +int ret, idx; +bool changed; +
... snip
Quoted 10 lines
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+ +return &ctrl->ctrl; + +out_exit_aen_ops: +nvme_fc_exit_aen_ops (ctrl); +out_remove_admin_queue: +nvme_stop_keep_alive (&ctrl->ctrl); +nvme_fc_destroy_admin_queue (ctrl); +out_uninit_ctrl: +nvme_uninit_ctrl (&ctrl->ctrl);
Quoted 8 lines
Review
Quoted 6 lines
drivers/nvme/host/fc.c new file +4
-0+nvme_put_ctrl (&ctrl->ctrl); +if (ret >0 ) + ret = -EIO ; +/* exit via here will follow ctlr ref point callbacks to free */
(quoted diff) +1
-0+ return ERR_PTR(ret);Memory leak?? kfree(ctrl); i.e. the "
struct_nvme_fc_ctrl*ctrl;" is not called before the return in this case I think?
No leak. The nvme_uninit and nvpe_put_ctrl will fall into the nvme ctrl's ref counting, which will call back into the transport via free_ctrl callback. That path will end up converting to the fc-nvme ctrl struct and fall into the it's ref counting, which will eventually do the free() call. That's what drove the comment on "exit via here will follow ctlr ref point callbacks to free".