nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Subsystems (3)
PATCH v2 7/7 nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Add FC LLDD loopback driver to test FC host and target transport within nvme-fabrics
To aid in the development and testing of the lower-level api of the FC transport, this loopback driver has been created to act as if it were a FC hba driver supporting both the host interfaces as well as the target interfaces with the nvme FC transport.
The changes made since the prior patch:
- Revise Kconfig for changes in select/depends
- Created defines for template constants per review
- Removal of fabric name for nport qualifier. Not necessary in T11
- Reworked teardown paths with ref counting and proper locking
- Teardown now invokes callbackes on completion of unregister functions
- Added callbacks for unregister complete interfaces
- Split sysfs add of remoteport and targetport for better testing.
- Modifications from prior reviews:
- Create inline xx_t_yy() wrappers for container_of's
--- drivers/nvme/target/Kconfig | 13 + drivers/nvme/target/Makefile | 2 + drivers/nvme/target/fcloop.c | 1142 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1157 insertions(+) create mode 100644 drivers/nvme/target/fcloop.c
drivers/nvme/target/Kconfig +13 -0
@@ -44,3 +44,16 @@ config NVME_TARGET_FC If unsure, say N. +config NVME_TARGET_FCLOOP + tristate "NVMe over Fabrics FC Transport Loopback Test driver" + depends on NVME_TARGET + select NVME_CORE + select NVME_FABRICS + select SG_POOL + select NVME_FC + select NVME_TARGET_FC + help + This enables the NVMe FC loopback test support, which can be useful + to test NVMe-FC transport interfaces. + + If unsure, say N.
drivers/nvme/target/Makefile +2 -0
@@ -3,9 +3,11 @@ obj-$(CONFIG_NVME_TARGET) += nvmet.o obj-$(CONFIG_NVME_TARGET_LOOP) += nvme-loop.o obj-$(CONFIG_NVME_TARGET_RDMA) += nvmet-rdma.o obj-$(CONFIG_NVME_TARGET_FC) += nvmet-fc.o +obj-$(CONFIG_NVME_TARGET_FCLOOP) += nvme-fcloop.o nvmet-y += core.o configfs.o admin-cmd.o io-cmd.o fabrics-cmd.o \ discovery.o nvme-loop-y += loop.o nvmet-rdma-y += rdma.o nvmet-fc-y += fc.o +nvme-fcloop-y += fcloop.o
drivers/nvme/target/fcloop.c new file +1142 -0
1,142 lines not on this page · raw message
Signature · 1 line
2.5.0
PATCH v2 7/7 nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Quoted 1 line
+#define
is_end_of_list(pos, head, member)((&pos->member) == (head))
This one isn't actually used in the code.
drivers/nvme/target/fcloop.c line 336
Quoted 1 line
drivers/nvme/target/fcloop.c new file +1
-0+ (tgt_lsreq->done)(tgt_lsreq);
no need for the first set of braces.
drivers/nvme/target/fcloop.c line 390
Quoted 3 lines
drivers/nvme/target/fcloop.c new file +3
-0+void +fcloop_fcp_copy_data (u8 op,struct scatterlist *data_sg, +struct scatterlist *io_sg,u32 offset,u32 length)
Should be marked static (as should most of the functions in this file)
Quoted 6 lines
drivers/nvme/target/fcloop.c new file +6
-0+int +fcloop_fcp_op (struct nvmet_fc_target_port *tgtport, +struct nvmefc_tgt_fcp_req *tgt_fcpreq) +{ +struct fcloop_fcpreq *tfcp_req =tgt_fcp_req_to_fcpreq (tgt_fcpreq); +struct nvmefc_fcp_req *fcpreq = tfcp_req->fcpreq;
drivers/nvme/target/fcloop.c line 457
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+u32 rsplen =0 , xfrlen =0 ; +int fcp_err =0 ; +u8 op = tgt_fcpreq->op; + +switch (op) { +case NVMET_FCOP_WRITEDATA : + xfrlen = tgt_fcpreq->transfer_length; +fcloop_fcp_copy_data (op, tgt_fcpreq->sg, fcpreq->first_sgl, + tgt_fcpreq->offset, xfrlen); + fcpreq->transferred_length += xfrlen;
.. but I don't even understand why we need to copy between these SGLs?
Quoted 5 lines
drivers/nvme/target/fcloop.c new file +5
-0+spin_lock_irqsave (&fcloop_lock, flags); + +list_for_each_entry (tlport, &fcloop_lports, lport_list) { +if ((tlport->localport->node_name == nodename) && + (tlport->localport->port_name == portname)) {
drivers/nvme/target/fcloop.c line 683
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+ lport = tlport; +break ; + } + } +spin_unlock_irqrestore (&fcloop_lock, flags); + +if (!lport) +return -ENOENT ; + + ret =__delete_local_port (lport);
We either need a reference or do the actual deletion under the lock.
drivers/nvme/target/fcloop.c line 779
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+list_for_each_entry (nport, &fcloop_nports, nport_list) { +if ((nport->node_name == opts->wwnn) && + (nport->port_name == opts->wwpn)) { +if ((remoteport && nport->rport) || + (!remoteport && nport->tport)) { + nport =NULL ; + goto out_invalid_opts; + } + +fcloop_nport_get (nport);
Needs to check the return value.
Quoted 4 lines
drivers/nvme/target/fcloop.c new file +4
-0+list_for_each_entry (tnport, &fcloop_nports, nport_list) { +if ((tnport->node_name == nodename) && + (tnport->port_name == portname) && tnport->rport) { + nport = tnport;
drivers/nvme/target/fcloop.c line 905
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+break ; + } + } + +spin_unlock_irqrestore (&fcloop_lock, flags); + +if (!nport) +return -ENOENT ; + + ret =__delete_remote_port (nport);
The deletion needs to be done under fcloop_lock, or we need a refcount.
Quoted 4 lines
drivers/nvme/target/fcloop.c new file +4
-0+list_for_each_entry (tnport, &fcloop_nports, nport_list) { +if ((tnport->node_name == nodename) && + (tnport->port_name == portname) && tnport->tport) { + nport = tnport;
drivers/nvme/target/fcloop.c line 1004
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+break ; + } + } + +spin_unlock_irqrestore (&fcloop_lock, flags); + +if (!nport) +return -ENOENT ; + + ret =__delete_target_port (nport);
Same here.
Quoted 6 lines
drivers/nvme/target/fcloop.c new file +6
-0+ + fcloop_device = +device_create (fcloop_class,NULL ,MKDEV (0 ,0 ),NULL ,"ctl" ); +if (IS_ERR (fcloop_device)) { +pr_err ("couldn't create ctl device!\n" ); + ret =PTR_ERR (fcloop_device);
drivers/nvme/target/fcloop.c line 1047
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+ goto out_destroy_class; + } + + ret =device_create_file (fcloop_device, &dev_attr_add_local_port); +if (ret) { +pr_err ("couldn't add device add_local_port attr.\n" ); + goto out_destroy_device; + } + + ret =device_create_file (fcloop_device, &dev_attr_del_local_port);
...
Please use device_create_with_groups.
Quoted 8 lines
drivers/nvme/target/fcloop.c new file +8
-0+for (;;) { + nport =list_first_entry_or_null (&fcloop_nports, +typeof (*nport), nport_list); +if (!nport) +break ; + +spin_unlock_irqrestore (&fcloop_lock, flags); +
drivers/nvme/target/fcloop.c line 1112
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+ ret =__delete_target_port (nport); +if (ret) +pr_warn ("%s: Failed deleting target port\n" , __func__); + + ret =__delete_remote_port (nport); +if (ret) +pr_warn ("%s: Failed deleting remote port\n" , __func__); + +spin_lock_irqsave (&fcloop_lock, flags); + }
The deletions need to happen without dropping the lock, or you need refcounts.
Quoted 8 lines
drivers/nvme/target/fcloop.c new file +8
-0+ +for (;;) { + lport =list_first_entry_or_null (&fcloop_lports, +typeof (*lport), lport_list); +if (!lport) +break ; + +spin_unlock_irqrestore (&fcloop_lock, flags);
drivers/nvme/target/fcloop.c line 1130
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+ + ret =__delete_local_port (lport); +if (ret) +pr_warn ("%s: Failed deleting local port\n" , __func__); + +spin_lock_irqsave (&fcloop_lock, flags); + + } + +spin_unlock_irqrestore (&fcloop_lock, flags);
Same here.
PATCH v2 7/7 nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Quoted 2 lines · Christoph Hellwig
Quoted 1 line
+#define
is_end_of_list(pos, head, member)((&pos->member) == (head))This one isn't actually used in the code.
Yep. I realize I removed it in the last rework.
Quoted 11 lines
Quoted 11 lines
drivers/nvme/target/fcloop.c new file +10
-0+int +fcloop_fcp_op (struct nvmet_fc_target_port *tgtport, +struct nvmefc_tgt_fcp_req *tgt_fcpreq) +{ +struct fcloop_fcpreq *tfcp_req =tgt_fcp_req_to_fcpreq (tgt_fcpreq); +struct nvmefc_fcp_req *fcpreq = tfcp_req->fcpreq; +u32 rsplen =0 , xfrlen =0 ; +int fcp_err =0 ; +u8 op = tgt_fcpreq->op; +
Quoted 8 lines
drivers/nvme/target/fcloop.c line 457
Quoted 6 lines
drivers/nvme/target/fcloop.c new file +6
-0+switch (op) { +case NVMET_FCOP_WRITEDATA : + xfrlen = tgt_fcpreq->transfer_length; +fcloop_fcp_copy_data (op, tgt_fcpreq->sg, fcpreq->first_sgl, + tgt_fcpreq->offset, xfrlen); + fcpreq->transferred_length += xfrlen;.. but I don't even understand why we need to copy between these SGLs?
You've lost me. The fc-nvme api on the initiator side has a sgl relative to the original io request, while the fc-nvmet api on the target side has a different sgl from the target fc-nvme transport to the LLDD for the range of data to exchange. They are sgl's as they expect to go to fc hw on each side. As they are independent sgl's on each side, we have to copy between them.
Quoted 10 lines
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +9
-0+spin_lock_irqsave (&fcloop_lock, flags); + +list_for_each_entry (tlport, &fcloop_lports, lport_list) { +if ((tlport->localport->node_name == nodename) && + (tlport->localport->port_name == portname)) { + lport = tlport; +break ; + } + }
Quoted 8 lines
drivers/nvme/target/fcloop.c line 683
Quoted 6 lines
drivers/nvme/target/fcloop.c new file +6
-0+spin_unlock_irqrestore (&fcloop_lock, flags); + +if (!lport) +return -ENOENT ; + + ret =__delete_local_port (lport);We either need a reference or do the actual deletion under the lock.
Well, there are only 2 conditions it matters - the user invokes the sysfs point twice for the same port, or sysfs and module unload occur at the same time. I guess I assumed both of these to be operator issues. But it's no big deal to address them. Yes - same conditions occurs on the other port delete calls.
Quoted 19 lines
Quoted 17 lines
drivers/nvme/target/fcloop.c new file +16
-0+ + fcloop_device = +device_create (fcloop_class,NULL ,MKDEV (0 ,0 ),NULL ,"ctl" ); +if (IS_ERR (fcloop_device)) { +pr_err ("couldn't create ctl device!\n" ); + ret =PTR_ERR (fcloop_device); + goto out_destroy_class; + } + + ret =device_create_file (fcloop_device, &dev_attr_add_local_port); +if (ret) { +pr_err ("couldn't add device add_local_port attr.\n" ); + goto out_destroy_device; + } + + ret =device_create_file (fcloop_device, &dev_attr_del_local_port);...
Quoted 1 line
Please use
device_create_with_groups.
ok
Quoted 13 lines
Quoted 13 lines
drivers/nvme/target/fcloop.c new file +12
-0+for (;;) { + nport =list_first_entry_or_null (&fcloop_nports, +typeof (*nport), nport_list); +if (!nport) +break ; + +spin_unlock_irqrestore (&fcloop_lock, flags); + + ret =__delete_target_port (nport); +if (ret) +pr_warn ("%s: Failed deleting target port\n" , __func__); +
Quoted 8 lines
drivers/nvme/target/fcloop.c line 1112
Quoted 6 lines
drivers/nvme/target/fcloop.c new file +6
-0+ ret =__delete_remote_port (nport); +if (ret) +pr_warn ("%s: Failed deleting remote port\n" , __func__); + +spin_lock_irqsave (&fcloop_lock, flags); + }The deletions need to happen without dropping the lock, or you need refcounts.
yes - I'll address it.
Ok to the other comments
-- james
PATCH v2 7/7 nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Quoted 5 lines · James Smart
You've lost me. The fc-nvme api on the initiator side has a sgl relative to the original io request, while the fc-nvmet api on the target side has a different sgl from the target fc-nvme transport to the LLDD for the range of data to exchange. They are sgl's as they expect to go to fc hw on each side. As they are independent sgl's on each side, we have to copy between them.
Oh, right - your target sgls aren't caller provided. That's a little sad when looking at this code, but if it doesn't affect real drivers I guess it's fine in the end.