nvme-fabrics: Add host FC transport support
Subsystems (3)
PATCH 3/5 nvme-fabrics: Add host FC transport support
Add nvme-fabrics host FC transport support
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.
--- drivers/nvme/host/Kconfig | 15 + drivers/nvme/host/Makefile | 3 + drivers/nvme/host/fc.c | 2113 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 2131 insertions(+) create mode 100644 drivers/nvme/host/fc.c
drivers/nvme/host/Kconfig +15 -0
@@ -43,3 +43,18 @@ 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 BLK_DEV_NVME + 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 +2113 -0
2,113 lines not on this page · raw message
Signature · 1 line
2.5.0
PATCH 3/5 nvme-fabrics: Add host FC transport support
On Fri, 2016-07-22@17:23 -0700, James Smart wrote:
A couple of minor comments:
Quoted 17 lines
Add nvme-fabrics host FC transport support
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.Signed-off-by James Smart <james.smart at broadcom.com>
snip...
drivers/nvme/host/fc.c line 560
Quoted 7 lines
drivers/nvme/host/fc.c new file +7
-0+/* TODO: +* assoc_rqst->assoc_cmd.cntlid = cpu_to_be16(?); +* strncpy(assoc_rqst->assoc_cmd.hostid, ?, +* min(FCNVME_ASSOC_HOSTID_LEN, NVMF_NQN_SIZE)); +* strncpy(assoc_rqst->assoc_cmd.hostnqn, ?, +* min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE]; +*/
What is the TODO here?
more snip...
Quoted 38 lines
drivers/nvme/host/fc.c new file +3
-0+ +static int +nvme_fc_init_queue(struct nvme_fc_ctrl *ctrl, int idx, size_t
queue_size)drivers/nvme/host/fc.c new file +14
-0+{ +struct nvme_fc_queue *queue; + + queue = &ctrl->queues[idx]; +memset (queue,0 ,sizeof (*queue)); + queue->ctrl = ctrl; + queue->qnum = idx; +atomic_set (&queue->csn,1 ); + queue->dev = ctrl->dev; + +if (idx >0 ) + queue->cmnd_capsule_len = ctrl->ctrl.ioccsz *16 ; +else + queue->cmnd_capsule_len = sizeof(struct
nvme_command);drivers/nvme/host/fc.c new file +5
-0+ + queue->queue_size = queue_size; + +/* +* Considered whether we should allocate buffers for all SQEs
(quoted diff) +1
-0+ * and CQEs and dma map them - mapping their respectiveentries
(quoted diff) +1
-0+ * into the request structures (kernel vm addr and dmaaddress)
drivers/nvme/host/fc.c new file +3
-0+* thus the driver could use the buffers/mappings directly. +* It only makes sense if the LLDD would use them for its +* messaging api. It's very unlikely most adapter api's would use
(quoted diff) +1
-0+ * a native NVME sqe/cqe. More reasonable if FC-NVME IUpayload
(quoted diff) +2
-0+ * structures were used instead. For now - just pass the + * sqe/cqes to the driver and let it deal with it. We'llfigure
drivers/nvme/host/fc.c line 1134
Quoted 4 lines
drivers/nvme/host/fc.c new file +4
-0+* out if the FC-NVME IUs make sense later. +*/ + +return 0 ;
Slightly confused. Looks like in nvme_fc_configure_admin_queue() and nvme_fc_init_io_queues() check for this function returning an error, but nvme_fc_init_queue() never returns anything but 0. Should it return an error? Does the comments above imply that this function could change in the future such that it would return something other than 0?
more more snip...
Quoted 13 lines
drivers/nvme/host/fc.c new file +11
-1+ +static int +nvme_fc_init_io_queues(struct nvme_fc_ctrl *ctrl) +{ + int i, ret; + + for (i = 1; i < ctrl->queue_count; i++) { + ret = nvme_fc_init_queue(ctrl, i, ctrl ->ctrl.sqsize); + if (ret) { + dev_info(ctrl->ctrl.device, + "failed to initialize i/o queue %d:%d\n",
drivers/nvme/host/fc.c line 1253
Quoted 5 lines
drivers/nvme/host/fc.c new file +5
-0+ i, ret); + } + } + +return 0 ;
Right now as-is nvme_fc_init_queue() will always return 0, but this function is hard-coded to return 0. Independent of what nvme_fc_init_queue() returns, this function should be returning 'ret' as "nvme_fc_create_io_queues()" has code to check if this function fails:
drivers/nvme/host/fc.c line 1769
Quoted 3 lines
drivers/nvme/host/fc.c new file +3
-0+static int +nvme_fc_create_io_queues (struct nvme_fc_ctrl *ctrl) +{
.
.
.
drivers/nvme/host/fc.c line 1790
Quoted 7 lines
drivers/nvme/host/fc.c new file +7
-0+dev_info (ctrl->ctrl.device,"creating %d I/O queues.\n" , + opts->nr_io_queues); + + ret =nvme_fc_init_io_queues (ctrl); +if (ret) +return ret; +
.
.
more more more snip...
Quoted 88 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 +32
-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; + +/* 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 ; + +/* +* validate per fabric rules, set fields mandated by fabric spec
drivers/nvme/host/fc.c new file +20
-2+* 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 */ +WARN_ON_ONCE(sqe->common.command_id != cpu_to_le16(op ->rqno)); + +if (op->rq) { /* skipped on aens */
drivers/nvme/host/fc.c new file +7
-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); +return (ret == -ENOMEM || ret == -EAGAIN ) ? + BLK_MQ_RQ_QUEUE_BUSY :
BLK_MQ_RQ_QUEUE_ERROR;drivers/nvme/host/fc.c new file +5
-1+ } + } + + dma_sync_single_for_device(ctrl->lport->dev, op ->fcp_req.cmddma, + sizeof(op->cmd_iu),
DMA_TO_DEVICE);drivers/nvme/host/fc.c new file +13
-1+ +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 ->fcp_req); + + if (ret) { + dev_err(ctrl->dev, + "Send nvme command failed - lldd returned
Quoted 1 line
%d.\n", ret);
see a bit lower for a comment on this if(ret) evaluating to TRUE.
Quoted 23 lines
(quoted diff) +2
-0+ + if (op->rq) { /* normalrequest */
drivers/nvme/host/fc.c new file +4
-0+nvme_fc_unmap_data (ctrl, op->rq, op); +nvme_cleanup_cmd (op->rq); +if (ret != -EBUSY ) { +/* complete the io w/ error status */
(quoted diff) +2
-0+ blk_mq_complete_request(op->rq, + NVME_SC_FC_TRANSPORT
_ERROR);drivers/nvme/host/fc.c new file +10
-1+ }else { +blk_mq_stop_hw_queues (op->rq->q); +nvme_requeue_req (op->rq); +blk_mq_delay_queue (queue->hctx, +NVMEFC_QUEUE_DELAY ); + } + }else {/* aen */ + struct nvme_completion *cqe = &op ->rsp_iu.cqe; + + cqe->status = (NVME_SC_FC_TRANSPORT_ERROR <<1);
drivers/nvme/host/fc.c line 1476
Quoted 6 lines
drivers/nvme/host/fc.c new file +5
-1+ nvme_complete_async_event(&queue->ctrl ->ctrl, cqe); + } + } + +return BLK_MQ_RQ_QUEUE_OK ;
Is this right? We want to return 'BLK_MQ_RQ_QUEUE_OK' and not something to signify the 'ret' value was not 0?
4 x more snip...
Quoted 84 lines
drivers/nvme/host/fc.c new file +21
-1+ +static int +nvme_fc_configure_admin_queue (struct nvme_fc_ctrl *ctrl) +{ +u32 segs; +int error; + + error =nvme_fc_init_queue (ctrl,0 ,NVME_FC_AQ_BLKMQ_DEPTH ); +if (error) +return error; + + error =nvme_fc_connect_admin_queue (ctrl, &ctrl->queues[0 ], +NVME_FC_AQ_BLKMQ_DEPTH , + (NVME_FC_AQ_BLKMQ_DEPTH /4 )); +if (error) +return error; + + memset(&ctrl->admin_tag_set, 0, sizeof(ctrl ->admin_tag_set)); + ctrl->admin_tag_set.ops = &nvme_fc_admin_mq_ops; + ctrl->admin_tag_set.queue_depth = NVME_FC_AQ_BLKMQ_DEPTH; + ctrl->admin_tag_set.reserved_tags = 2; /* fabric connect +Keep-Alive */
(quoted diff) +5
-0+ ctrl->admin_tag_set.numa_node = NUMA_NO_NODE; + ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) + + (SG_CHUNK_SIZE * + sizeof(structscatterlist)) +
drivers/nvme/host/fc.c new file +51
-4+ ctrl->lport->ops ->fcprqst_priv_sz; + ctrl->admin_tag_set.driver_data = ctrl; + ctrl->admin_tag_set.nr_hw_queues =1 ; + ctrl->admin_tag_set.timeout =ADMIN_TIMEOUT ; + + error =blk_mq_alloc_tag_set (&ctrl->admin_tag_set); +if (error) + goto out_free_queue; + + ctrl->ctrl.admin_q = blk_mq_init_queue(&ctrl ->admin_tag_set); + if (IS_ERR(ctrl->ctrl.admin_q)) { + error = PTR_ERR(ctrl->ctrl.admin_q); + goto out_free_tagset; + } + + error =__nvme_fc_create_hw_queue (ctrl, &ctrl->queues[0 ],0 , + NVME_FC_AQ_BLKMQ_DEPTH); +if (error) + goto out_cleanup_queue; + + error =nvmf_connect_admin_queue (&ctrl->ctrl); +if (error) + goto out_delete_hw_queue; + + error = nvmf_reg_read64(&ctrl->ctrl, NVME_REG_CAP, &ctrl ->cap); + if (error) { + dev_err(ctrl->ctrl.device, + "prop_get NVME_REG_CAP failed\n"); + goto out_delete_hw_queue; + } + + ctrl->ctrl.sqsize = + min_t(int, NVME_CAP_MQES(ctrl->cap) + 1, ctrl ->ctrl.sqsize); + + error = nvme_enable_ctrl(&ctrl->ctrl, ctrl->cap); + if (error) + goto out_delete_hw_queue; + + segs = min_t(u32, NVME_FC_MAX_SEGMENTS, + ctrl->lport->ops->max_sgl_segments); + ctrl->ctrl.max_hw_sectors = (segs - 1) << (PAGE_SHIFT - 9); + + error = nvme_init_identify(&ctrl->ctrl); + if (error) + goto out_delete_hw_queue; + + nvme_start_keep_alive(&ctrl->ctrl); + + return 0; + +out_delete_hw_queue:
drivers/nvme/host/fc.c line 1664
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+__nvme_fc_delete_hw_queue (ctrl, &ctrl->queues[0 ],0 ); +out_cleanup_queue: +blk_cleanup_queue (ctrl->ctrl.admin_q); +out_free_tagset: +blk_mq_free_tag_set (&ctrl->admin_tag_set); +out_free_queue: +nvme_fc_free_queue (&ctrl->queues[0 ]); +return error; +} +
leaving in the above function from the referral comments I made before.
again snip...
Quoted 69 lines
drivers/nvme/host/fc.c new file +33
-1+static int +nvme_fc_create_io_queues (struct nvme_fc_ctrl *ctrl) +{ +struct nvmf_ctrl_options *opts = ctrl->ctrl.opts; +int ret; + + ret = nvme_set_queue_count(&ctrl->ctrl, &opts ->nr_io_queues); + if (ret) { + dev_info(ctrl->ctrl.device, + "set_queue_count failed: %d\n", ret); + return ret; + } + + ctrl->queue_count = opts->nr_io_queues + 1; +if (!opts->nr_io_queues) +return 0 ; + +dev_info (ctrl->ctrl.device,"creating %d I/O queues.\n" , + opts->nr_io_queues); + + ret =nvme_fc_init_io_queues (ctrl); +if (ret) +return ret; + +memset (&ctrl->tag_set,0 ,sizeof (ctrl->tag_set)); + ctrl->tag_set.ops = &nvme_fc_mq_ops; + ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize; + ctrl->tag_set.reserved_tags =1 ;/* fabric connect */ + ctrl->tag_set.numa_node =NUMA_NO_NODE ; + ctrl->tag_set.flags =BLK_MQ_F_SHOULD_MERGE ; + ctrl->tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) + + (SG_CHUNK_SIZE * + sizeof(structscatterlist)) +
drivers/nvme/host/fc.c new file +30
-1+ ctrl->lport->ops ->fcprqst_priv_sz; + ctrl->tag_set.driver_data = ctrl; + ctrl->tag_set.nr_hw_queues = ctrl->queue_count -1 ; + ctrl->tag_set.timeout =NVME_IO_TIMEOUT ; + + ret =blk_mq_alloc_tag_set (&ctrl->tag_set); +if (ret) +return ret; + + ctrl->ctrl.tagset = &ctrl->tag_set; + + ctrl->ctrl.connect_q =blk_mq_init_queue (&ctrl->tag_set); +if (IS_ERR (ctrl->ctrl.connect_q)) { + ret =PTR_ERR (ctrl->ctrl.connect_q); + goto out_free_tag_set; + } + + ret =nvme_fc_create_hw_io_queues (ctrl, ctrl->ctrl.sqsize); +if (ret) + goto out_cleanup_blk_queue; + + ret =nvme_fc_connect_io_queues (ctrl, ctrl->ctrl.sqsize); +if (ret) + goto out_delete_hw_queues; + +return 0 ; + +out_delete_hw_queues: +nvme_fc_delete_hw_io_queues (ctrl); +out_cleanup_blk_queue:
drivers/nvme/host/fc.c line 1839
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+nvme_stop_keep_alive (&ctrl->ctrl); +blk_cleanup_queue (ctrl->ctrl.connect_q); +out_free_tag_set: +blk_mq_free_tag_set (&ctrl->tag_set); +nvme_fc_free_io_queues (ctrl); + +return ret; +} + +
leaving in from the referral comments I made earlier...
snippy-snip...
Quoted 43 lines
(quoted diff) +2
-0+static struct nvme_ctrl * +__nvme_fc_create_ctrl(struct device *dev, struct nvmf_ctrl_options*opts,
drivers/nvme/host/fc.c new file +18
-0+ struct nvme_fc_lport *lport, struct nvme_fc_rport *rport) +{ + struct nvme_fc_ctrl *ctrl; +int ret; +bool changed; + + ctrl =kzalloc (sizeof (*ctrl),GFP_KERNEL ); +if (!ctrl) +return ERR_PTR (-ENOMEM ); + ctrl->ctrl.opts = opts; +INIT_LIST_HEAD (&ctrl->ctrl_list); + ctrl->lport = lport; + ctrl->l_id = lport->localport.port_num; + ctrl->rport = rport; + ctrl->r_id = rport->remoteport.port_num; + ctrl->dev = lport->dev; + + ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_fc_ctrl_ops,0);
drivers/nvme/host/fc.c new file +11
-0+if (ret) + goto out_free_ctrl; + +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; + 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 +1
-0+ GFP_KERNEL);
drivers/nvme/host/fc.c line 1888
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+if (!ctrl->queues) + goto out_uninit_ctrl; + + ret =nvme_fc_configure_admin_queue (ctrl); +if (ret) + goto out_kfree_queues; + +/* sanity checks */ + +if (ctrl->ctrl.ioccsz !=4 ) {
Slightly confused, the spec says the minimum value that may be specified is 4. So 4 is not a legal value for fc transport?
My preference would be to make this
if (ctrl->ctrl.ioccsz <= 4) {
even though the spec says the minimum value for ioccsz is 4 (to catch weird programming errors).
drivers/nvme/host/fc.c line 1893
Quoted 6 lines
(quoted diff) +1
-0+ dev_err(ctrl->ctrl.device, "ioccsz %d is notsupported!\n",
drivers/nvme/host/fc.c new file +4
-0+ ctrl->ctrl.ioccsz); + goto out_remove_admin_queue; + } +if (ctrl->ctrl.iorcsz !=1 ) {
dido here, so the fc transport does not support the minimum value the spec says, 1?
Quoted 59 lines
(quoted diff) +1
-0+ dev_err(ctrl->ctrl.device, "iorcsz %d is notsupported!\n",
drivers/nvme/host/fc.c new file +5
-0+ ctrl->ctrl.iorcsz); + goto out_remove_admin_queue; + } +if (ctrl->ctrl.icdoff) { + dev_err(ctrl->ctrl.device, "icdoff %d is notsupported!\n",
drivers/nvme/host/fc.c new file +8
-0+ ctrl->ctrl.icdoff); + goto out_remove_admin_queue; + } + +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, clampingdown\n",
drivers/nvme/host/fc.c new file +15
-0+ 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; + } + + changed = nvme_change_ctrl_state(&ctrl->ctrl,
NVME_CTRL_LIVE);drivers/nvme/host/fc.c new file +5
-0+WARN_ON_ONCE (!changed); + + dev_info(ctrl->ctrl.device, + "NVME-FC[%d.%d]: new ctrl: NQN \"%s\" (%p)\n", + ctrl->l_id, ctrl->r_id, ctrl->ctrl.opts->subsysnqn,&ctrl);
drivers/nvme/host/fc.c new file +20
-0+ +kref_get (&ctrl->ctrl.kref); + +mutex_lock (&nvme_fc_ctrl_mutex); +list_add_tail (&ctrl->ctrl_list, &nvme_fc_ctrl_list); +mutex_unlock (&nvme_fc_ctrl_mutex); + +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_fc_destroy_admin_queue (ctrl); +out_kfree_queues: +kfree (ctrl->queues);
drivers/nvme/host/fc.c line 1957
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+out_uninit_ctrl: +nvme_uninit_ctrl (&ctrl->ctrl); +nvme_put_ctrl (&ctrl->ctrl); +if (ret >0 ) + ret = -EIO ; +return ERR_PTR (ret); +out_free_ctrl: +kfree (ctrl); +return ERR_PTR (ret); +}
whew...I'll try to get to patch 4 and 5 soon...
Jay
PATCH 3/5 nvme-fabrics: Add host FC transport support
Quoted 8 lines · J Freyensee
drivers/nvme/host/fc.c line 560
Quoted 7 lines
drivers/nvme/host/fc.c new file +7
-0+/* TODO: +* assoc_rqst->assoc_cmd.cntlid = cpu_to_be16(?); +* strncpy(assoc_rqst->assoc_cmd.hostid, ?, +* min(FCNVME_ASSOC_HOSTID_LEN, NVMF_NQN_SIZE)); +* strncpy(assoc_rqst->assoc_cmd.hostnqn, ?, +* min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE]; +*/ What is the TODO here?
main todo is that the ctrlid is not known yet - not set in the opts struct yet. It's set within the fabrics connect
routines that build the capsule, which are called after this transport action. So the todo is to modify
the core to add it to the opts struct as well. Then these lines can be added in.
Quoted 40 lines
more snip...
drivers/nvme/host/fc.c line 235
Quoted 8 lines
drivers/nvme/host/fc.c new file +3
-0+ +static int +nvme_fc_init_queue(struct nvme_fc_ctrl *ctrl, int idx, size_t
queue_size)drivers/nvme/host/fc.c new file +4
-0+{ + ... + +return 0 ;Slightly confused. Looks like in
nvme_fc_configure_admin_queue()andnvme_fc_init_io_queues()check for this function returning an error, butnvme_fc_init_queue()never returns anything but 0. Should it return an error? Does the comments above imply that this function could change in the future such that it would return something other than 0?more more snip...
drivers/nvme/host/fc.c line 1253
Quoted 7 lines
drivers/nvme/host/fc.c new file +7
-0+ +static int +nvme_fc_init_io_queues (struct nvme_fc_ctrl *ctrl) +{ + ... + +return 0 ;Right now as-is
nvme_fc_init_queue()will always return 0, but this function is hard-coded to return 0. Independent of whatnvme_fc_init_queue()returns, this function should be returning 'ret' as "nvme_fc_create_io_queues()" has code to check if this function fails:drivers/nvme/host/fc.c line 1769
Quoted 3 lines
drivers/nvme/host/fc.c new file +3
-0+static int +nvme_fc_create_io_queues (struct nvme_fc_ctrl *ctrl) +{.
.
.
Quoted 7 lines
drivers/nvme/host/fc.c line 1790
Quoted 7 lines
drivers/nvme/host/fc.c new file +7
-0+dev_info (ctrl->ctrl.device,"creating %d I/O queues.\n" , + opts->nr_io_queues); + + ret =nvme_fc_init_io_queues (ctrl); +if (ret) +return ret; +
Well, at the time it was written, it wasn't clear it would always return
0. As it does, I'll see about cleaning it up.
Quoted 46 lines
Quoted 20 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 +15
-1+ struct nvme_fc_fcp_op *op, u32 data_len, + enum nvmefc_fcp_datadir io_dir) +{ + ... + + 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 ->fcp_req); + + if (ret) { + dev_err(ctrl->dev, + "Send nvme command failed - lldd returnedQuoted 1 line
%d.\n", ret);
see a bit lower for a comment on this if(ret) evaluating to TRUE.
Quoted 23 lines
(quoted diff) +2
-0+ + if (op->rq) { /* normalrequest */
drivers/nvme/host/fc.c new file +4
-0+nvme_fc_unmap_data (ctrl, op->rq, op); +nvme_cleanup_cmd (op->rq); +if (ret != -EBUSY ) { +/* complete the io w/ error status */
(quoted diff) +2
-0+ blk_mq_complete_request(op->rq, + NVME_SC_FC_TRANSPORT
_ERROR);drivers/nvme/host/fc.c new file +10
-1+ }else { +blk_mq_stop_hw_queues (op->rq->q); +nvme_requeue_req (op->rq); +blk_mq_delay_queue (queue->hctx, +NVMEFC_QUEUE_DELAY ); + } + }else {/* aen */ + struct nvme_completion *cqe = &op ->rsp_iu.cqe; + + cqe->status = (NVME_SC_FC_TRANSPORT_ERROR <<1);
Quoted 8 lines
drivers/nvme/host/fc.c line 1476
Quoted 6 lines
drivers/nvme/host/fc.c new file +5
-1+ nvme_complete_async_event(&queue->ctrl ->ctrl, cqe); + } + } + +return BLK_MQ_RQ_QUEUE_OK ;Is this right? We want to return '
BLK_MQ_RQ_QUEUE_OK' and not something to signify the 'ret' value was not 0?
yeah - this could probably use fixing. I should be returning BLK_MQ_RQ_QUEUE_BUSY for some of the cases and BLK_MQ_RQ_QUEUE_ERROR on some others.
Quoted 57 lines
Quoted 44 lines
(quoted diff) +2
-0+static struct nvme_ctrl * +__nvme_fc_create_ctrl(struct device *dev, struct nvmf_ctrl_options*opts,
drivers/nvme/host/fc.c new file +18
-0+ struct nvme_fc_lport *lport, struct nvme_fc_rport *rport) +{ + struct nvme_fc_ctrl *ctrl; +int ret; +bool changed; + + ctrl =kzalloc (sizeof (*ctrl),GFP_KERNEL ); +if (!ctrl) +return ERR_PTR (-ENOMEM ); + ctrl->ctrl.opts = opts; +INIT_LIST_HEAD (&ctrl->ctrl_list); + ctrl->lport = lport; + ctrl->l_id = lport->localport.port_num; + ctrl->rport = rport; + ctrl->r_id = rport->remoteport.port_num; + ctrl->dev = lport->dev; + + ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_fc_ctrl_ops,0);
drivers/nvme/host/fc.c new file +11
-0+if (ret) + goto out_free_ctrl; + +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; + 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 +1
-0+ GFP_KERNEL);drivers/nvme/host/fc.c line 1888
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+if (!ctrl->queues) + goto out_uninit_ctrl; + + ret =nvme_fc_configure_admin_queue (ctrl); +if (ret) + goto out_kfree_queues; + +/* sanity checks */ + +if (ctrl->ctrl.ioccsz !=4 ) {Slightly confused, the spec says the minimum value that may be specified is 4. So 4 is not a legal value for fc transport?
Quoted 6 lines
My preference would be to make this
if (
ctrl->ctrl.ioccsz<= 4) {even though the spec says the minimum value for ioccsz is 4 (to catch weird programming errors).
?? The only value supported by FC is a 64byte capsule (really SQE),
thus only 4 is supported. Although, you're touching on a point in the
fabrics spec that I think is really screwy. ping me offline if you want more opinion.
Quoted 9 lines
drivers/nvme/host/fc.c line 1893
Quoted 7 lines
(quoted diff) +1
-0+ dev_err(ctrl->ctrl.device, "ioccsz %d is notsupported!\n",
drivers/nvme/host/fc.c new file +4
-0+ ctrl->ctrl.ioccsz); + goto out_remove_admin_queue; + } +if (ctrl->ctrl.iorcsz !=1 ) {dido here, so the fc transport does not support the minimum value the spec says, 1?
No - FC says it must be 1. Thus a 16-byte CQE.
FC could expand to larger SQE/CQE, but not right now.
Thanks
-- james
PATCH 3/5 nvme-fabrics: Add host FC transport support
In reply to James SmartThis looks mostly fine, a few nitpicks below:
drivers/nvme/host/Kconfig config NVME_RDMA · line 49
Quoted 3 lines
drivers/nvme/host/Kconfig +3
-0+config NVME_FC + tristate "NVM Express over Fabrics FC host driver" + depends on BLK_DEV_NVME
This should be
select NVME_CORE
instead. The existing RDMA and loop drivers also get this wrong, but I'll sned a patch to fix it up.
drivers/nvme/host/Kconfig config NVME_RDMA · line 58
Quoted 9 lines
drivers/nvme/host/Kconfig +9
-0+ 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.
Are you going to send a FC support patch for nvme-cli as well?
drivers/nvme/host/fc.c line 436
Quoted 4 lines
drivers/nvme/host/fc.c new file +4
-0+/* TODO: better to use dma_map_page() ?*/ + lsreq->rqstdma =dma_map_single (ctrl->dev, lsreq->rqstaddr, + (lsreq->rqstlen + lsreq->rsplen), +DMA_BIDIRECTIONAL );
Either one is fine, so no need to the TODO comment, please remove it and all the other similar ones.
Also while I'm at it: no need for the braces around the "lsreq->rqstlen + lsreq->rsplen".
drivers/nvme/host/fc.c line 560
Quoted 9 lines
drivers/nvme/host/fc.c new file +9
-0+ assoc_rqst->assoc_cmd.ersp_ratio =cpu_to_be16 (ersp_ratio); + assoc_rqst->assoc_cmd.sqsize =cpu_to_be16 (qsize); +/* TODO: +* assoc_rqst->assoc_cmd.cntlid = cpu_to_be16(?); +* strncpy(assoc_rqst->assoc_cmd.hostid, ?, +* min(FCNVME_ASSOC_HOSTID_LEN, NVMF_NQN_SIZE)); +* strncpy(assoc_rqst->assoc_cmd.hostnqn, ?, +* min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE]; +*/
What is the problem with filling all these out based on the information in the nvme_host structure?
Quoted 9 lines
drivers/nvme/host/fc.c new file +9
-0+nvme_fc_free_nvme_ctrl(struct nvme_ctrl *nctrl) +{ +struct nvme_fc_ctrl *ctrl =to_fc_ctrl (nctrl); + +/* +* if we've already started down this path, controller +* will already be unlinked. +*/ +if (list_empty (&ctrl->ctrl_list))
drivers/nvme/host/fc.c line 1280
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+ goto free_ctrl; + +mutex_lock (&nvme_fc_ctrl_mutex); +list_del (&ctrl->ctrl_list); +mutex_unlock (&nvme_fc_ctrl_mutex); + +if (nctrl->tagset) { +blk_cleanup_queue (ctrl->ctrl.connect_q); +blk_mq_free_tag_set (&ctrl->tag_set); + }
This probably needs to be moved, similar to the patch Sagi just did for RDMA. In general it might be a good idea to look at the various recent RDMA changes and check if they need to be brought over.
drivers/nvme/host/fc.c line 1351
Quoted 3 lines
drivers/nvme/host/fc.c new file +3
-0+/* +* TODO: blk_integrity_rq(rq) for DIX +*/
We'll hopefully be able to just move the DIX handling to common code. Any help appreciated..
Quoted 9 lines
drivers/nvme/host/fc.c new file +9
-0+/* find the host and remote ports to connect together */ +spin_lock_irqsave (&nvme_fc_lock, flags); +list_for_each_entry (lport, &nvme_fc_lport_list, port_list) { +if ((lport->localport.fabric_name != laddr.fab) || + (lport->localport.node_name != laddr.nn) || + (lport->localport.port_name != laddr.pn)) +continue ; + +list_for_each_entry (rport, &lport->endp_list, endp_list) {
drivers/nvme/host/fc.c line 2068
Quoted 10 lines
drivers/nvme/host/fc.c new file +10
-0+if ((rport->remoteport.node_name != raddr.nn) || + (rport->remoteport.port_name != raddr.pn)) +continue ; + +spin_unlock_irqrestore (&nvme_fc_lock, flags); + +return __nvme_fc_create_ctrl (dev, opts, lport, rport); + } + } +spin_unlock_irqrestore (&nvme_fc_lock, flags);
This seems to miss reference counting on the lport and rport structure.
drivers/nvme/host/fc.c line 2107
Quoted 5 lines
drivers/nvme/host/fc.c new file +5
-0+/* release topology elements +* TODO: This is broken: as ctrl delete is async - need to tie +* final topology delete to last controller detach +*/ +__nvme_fc_free_ports ();
It would be good to sort this out before merging.
PATCH 3/5 nvme-fabrics: Add host FC transport support
Quoted 2 lines
Are you going to send a FC support patch for nvme-cli as well?
I will, but concentrating on core support for now. I expect a fair number of things need to be done in the cli.
Quoted 12 lines
drivers/nvme/host/fc.c line 560
Quoted 10 lines
drivers/nvme/host/fc.c new file +9
-0+ assoc_rqst->assoc_cmd.ersp_ratio =cpu_to_be16 (ersp_ratio); + assoc_rqst->assoc_cmd.sqsize =cpu_to_be16 (qsize); +/* TODO: +* assoc_rqst->assoc_cmd.cntlid = cpu_to_be16(?); +* strncpy(assoc_rqst->assoc_cmd.hostid, ?, +* min(FCNVME_ASSOC_HOSTID_LEN, NVMF_NQN_SIZE)); +* strncpy(assoc_rqst->assoc_cmd.hostnqn, ?, +* min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE]; +*/ What is the problem with filling all these out based on the information in the
nvme_hoststructure?
Well, I want to get them from the same place that the fabrics routines do when they build a capsule. So they should come from the opts struct. It's missing the controller id, so I'll need to address that.
Quoted 4 lines
This probably needs to be moved, similar to the patch Sagi just did for RDMA. In general it might be a good idea to look at the various recent RDMA changes and check if they need to be brought over.
yeah - I typically do a resync every time I repost.
Quoted 6 lines
drivers/nvme/host/fc.c line 1351
Quoted 4 lines
drivers/nvme/host/fc.c new file +3
-0+/* +* TODO: blk_integrity_rq(rq) for DIX +*/ We'll hopefully be able to just move the DIX handling to common code. Any help appreciated..
Yes and no.... and likely a separate discussion. If DIF is passed,
you should involve the host port to validate on egress as well has what the target controller will do. Although it brings up some headaches if the host detects an error (e.g. protocol requires target to be part of it). If DIF is inserted, the question will be whether you want to insert DIF at the egress point of the host port or do you pass it over the fabric w/o dif and have the tgt controller insert it. I assume we'll need to talk more.
Quoted 2 lines
This seems to miss reference counting on the lport and rport structure.
yes - an area of weakness right now.
Quoted 1 line
It would be good to sort this out before merging.
I'll make a pass at it.
-- james