[RFC 0/2] nvme/loop: Add support for controllers-per-port model

[RFC 0/2] nvme/loop: Add support for controllers-per-port model

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi again folks,

So building on the nvmet/configfs-ng WIP posted yesterday:

http://marc.info/?l=linux-scsi&m=146528147018404&w=2

This series adds support for nvme/loop to utilize a nvme host
controller per nvmet_port, instead of a single hardcoded entry
for nvmet_port pointer access in nvme_loop_queue_rq().

It uses a struct device model following what we've done in
drivers/target/loopback to allow multiple host controllers
to co-exist under configfs, and is driven by the nvmet_port
configfs enable attribute.

Which means that any arbitary number of nvme-loop controllers
can now exist, and each nvmet_subsystem->ports_group can
enable/disable it's own loopback controller!

Here is how it looks in action for controller id 1 + 2:

root@scsi-mq:/sys/kernel/config/nvmet/subsystems# tree -if .
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/namespaces
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/namespaces/1
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/namespaces/1/ramdisk1 -> ../../../../../target/core/rd_mcp_2/ramdisk1
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop/addr_adrfam
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop/addr_portid
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop/addr_traddr
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop/addr_treq
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop/addr_trsvcid
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop/addr_trtype
./nqn.2003-01.org.linux-iscsi.NVMf.newsilicon/ports/loop/enable
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/namespaces
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/namespaces/1
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/namespaces/1/ramdisk0 -> ../../../../../target/core/rd_mcp_1/ramdisk0
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop/addr_adrfam
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop/addr_portid
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop/addr_traddr
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop/addr_treq
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop/addr_trsvcid
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop/addr_trtype
./nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep/ports/loop/enable

# cat /proc/partitions              
major minor  #blocks  name

 259        0    4194304 nvme0n1
 259        1      65535 nvme1n1
 259        2      65535 nvme2n1

Comments..?

--nab

Nicholas Bellinger (2):
  nvme-fabrics: Add nvmf_get_default_host helper
  nvme/loop: Add support for controller-per-port model

 drivers/nvme/host/fabrics.c |  13 +++-
 drivers/nvme/host/fabrics.h |   1 +
 drivers/nvme/target/loop.c  | 183 +++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 177 insertions(+), 20 deletions(-)

-- 
1.9.1

[RFC 1/2] nvme-fabrics: Add nvmf_get_default_host helper

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a nvmf_get_default_host() helper used to
setup nvmf_ctrl_options->host to internal nvmf_default_host
reference, following existing nvmf_parse_options() code.

Note it's required for nvme-loop multi-controller support,
in order to drive nvmet_port creation directly via configfs
attribute write from in ../nvmet/subsystems/$NQN/ports/$PORT/
group context.

Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Jay Freyensee <james.p.freyensee@intel.com>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/nvme/host/fabrics.c | 13 +++++++++++--
 drivers/nvme/host/fabrics.h |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index ee4b7f1..04317be 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -497,6 +497,16 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
 	return NULL;
 }
 
+void nvmf_get_default_host(struct nvmf_ctrl_options *opts)
+{
+	if (opts->host)
+		return;
+
+	kref_get(&nvmf_default_host->ref);
+	opts->host = nvmf_default_host;
+}
+EXPORT_SYMBOL_GPL(nvmf_get_default_host);
+
 static const match_table_t opt_tokens = {
 	{ NVMF_OPT_TRANSPORT,		"transport=%s"		},
 	{ NVMF_OPT_TRADDR,		"traddr=%s"		},
@@ -686,8 +696,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	}
 
 	if (!opts->host) {
-		kref_get(&nvmf_default_host->ref);
-		opts->host = nvmf_default_host;
+		nvmf_get_default_host(opts);
 	}
 
 out:
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index b540674..73ef7a7 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -128,6 +128,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl);
 int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid);
 void nvmf_register_transport(struct nvmf_transport_ops *ops);
 void nvmf_unregister_transport(struct nvmf_transport_ops *ops);
+void nvmf_get_default_host(struct nvmf_ctrl_options *opts);
 void nvmf_free_options(struct nvmf_ctrl_options *opts);
 const char *nvmf_get_subsysnqn(struct nvme_ctrl *ctrl);
 int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
-- 
1.9.1

[RFC 2/2] nvme/loop: Add support for controller-per-port model

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch introduces loopback support for a nvme host
controller per nvmet_port instance model, following what
we've done in drivers/target/loopback/ for allowing
multiple host LLDs to co-exist.

It changes nvme_loop_add_port() to use struct nvme_loop_port
and take the nvmf_get_default_host() reference, and invokes
device_register() to nvme_loop_driver_probe() to kick off
controller creation within nvme_loop_create_ctrl().

This allows nvme_loop_queue_rq to setup iod->req.port to
the per nvmet_port pointer, instead of a single hardcoded
global nvmet_loop_port.

Subsequently, it also adds nvme_loop_remove_port() to call
device_unregister() and call nvme_loop_del_ctrl() and
nvmf_free_options() to drop vmet_port's nvme_default_host
rereference, when the nvmet_port port is being removed
from the associated nvmet_subsys.

Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Jay Freyensee <james.p.freyensee@intel.com>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/nvme/target/loop.c | 183 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 165 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index b4b4da9..01b73dc 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -46,6 +46,13 @@ struct nvme_loop_iod {
 	struct scatterlist	first_sgl[];
 };
 
+struct nvme_loop_port {
+	struct device		dev;
+	struct nvmf_ctrl_options *opts;
+	struct nvmet_port	*port;
+	struct nvme_ctrl	*ctrl;
+};
+
 struct nvme_loop_ctrl {
 	spinlock_t		lock;
 	struct nvme_loop_queue	*queues;
@@ -62,6 +69,8 @@ struct nvme_loop_ctrl {
 	struct nvmet_ctrl	*target_ctrl;
 	struct work_struct	delete_work;
 	struct work_struct	reset_work;
+
+	struct nvme_loop_port	*port;
 };
 
 static inline struct nvme_loop_ctrl *to_loop_ctrl(struct nvme_ctrl *ctrl)
@@ -75,8 +84,6 @@ struct nvme_loop_queue {
 	struct nvme_loop_ctrl	*ctrl;
 };
 
-static struct nvmet_port *nvmet_loop_port;
-
 static LIST_HEAD(nvme_loop_ctrl_list);
 static DEFINE_MUTEX(nvme_loop_ctrl_mutex);
 
@@ -173,7 +180,8 @@ static int nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		return ret;
 
 	iod->cmd.common.flags |= NVME_CMD_SGL_METABUF;
-	iod->req.port = nvmet_loop_port;
+	iod->req.port = queue->ctrl->port->port;
+
 	if (!nvmet_req_init(&iod->req, &queue->nvme_cq,
 			&queue->nvme_sq, &nvme_loop_ops)) {
 		nvme_cleanup_cmd(req);
@@ -618,6 +626,8 @@ out_destroy_queues:
 static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 		struct nvmf_ctrl_options *opts)
 {
+	struct nvme_loop_port *loop_port = container_of(dev,
+				struct nvme_loop_port, dev);
 	struct nvme_loop_ctrl *ctrl;
 	bool changed;
 	int ret;
@@ -626,6 +636,7 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 	if (!ctrl)
 		return ERR_PTR(-ENOMEM);
 	ctrl->ctrl.opts = opts;
+	ctrl->port = loop_port;
 	INIT_LIST_HEAD(&ctrl->list);
 
 	INIT_WORK(&ctrl->delete_work, nvme_loop_del_ctrl_work);
@@ -700,29 +711,117 @@ out_put_ctrl:
 	return ERR_PTR(ret);
 }
 
+static int nvme_loop_driver_probe(struct device *dev)
+{
+	struct nvme_loop_port *loop_port = container_of(dev,
+				struct nvme_loop_port, dev);
+	struct nvme_ctrl *ctrl;
+
+	ctrl = nvme_loop_create_ctrl(dev, loop_port->opts);
+	if (IS_ERR(ctrl))
+		return PTR_ERR(ctrl);
+
+	loop_port->ctrl = ctrl;
+	return 0;
+}
+
+static int nvme_loop_driver_remove(struct device *dev)
+{
+	struct nvme_loop_port *loop_port = container_of(dev,
+				struct nvme_loop_port, dev);
+	struct nvme_ctrl *ctrl = loop_port->ctrl;
+	struct nvmf_ctrl_options *opts = loop_port->opts;
+
+	nvme_loop_del_ctrl(ctrl);
+	nvmf_free_options(opts);
+	return 0;
+}
+
+static int pseudo_bus_match(struct device *dev,
+			    struct device_driver *dev_driver)
+{
+	return 1;
+}
+
+static struct bus_type nvme_loop_bus = {
+	.name			= "nvme_loop_bus",
+	.match			= pseudo_bus_match,
+	.probe			= nvme_loop_driver_probe,
+	.remove			= nvme_loop_driver_remove,
+};
+
+static struct device_driver nvme_loop_driverfs = {
+	.name			= "nvme_loop",
+	.bus			= &nvme_loop_bus,
+};
+
+static void nvme_loop_release_adapter(struct device *dev)
+{
+	struct nvme_loop_port *loop_port = container_of(dev,
+				struct nvme_loop_port, dev);
+
+	kfree(loop_port);
+}
+
+static struct device *nvme_loop_primary;
+
 static int nvme_loop_add_port(struct nvmet_port *port)
 {
-	/*
-	 * XXX: disalow adding more than one port so
-	 * there is no connection rejections when a
-	 * a subsystem is assigned to a port for which
-	 * loop doesn't have a pointer.
-	 * This scenario would be possible if we allowed
-	 * more than one port to be added and a subsystem
-	 * was assigned to a port other than nvmet_loop_port.
-	 */
+	struct nvmet_subsys *subsys = port->nf_subsys;
+	struct nvme_loop_port *loop_port;
+	struct nvmf_ctrl_options *opts;
+	struct device *dev;
+	int ret;
 
-	if (nvmet_loop_port)
-		return -EPERM;
+	loop_port = kzalloc(sizeof(*loop_port), GFP_KERNEL);
+	if (!loop_port)
+		return -ENOMEM;
+
+	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
+	if (!opts) {
+		kfree(loop_port);
+		return -ENOMEM;
+	}
+	loop_port->opts = opts;
+
+	/* Set defaults */
+	opts->queue_size = NVMF_DEF_QUEUE_SIZE;
+	opts->nr_io_queues = num_online_cpus();
+	opts->tl_retry_count = 2;
+	opts->reconnect_delay = NVMF_DEF_RECONNECT_DELAY;
+	opts->kato = NVME_DEFAULT_KATO;
+
+	nvmf_get_default_host(opts);
+	opts->transport = kstrdup("loop", GFP_KERNEL);
+	opts->subsysnqn = kstrdup(subsys->subsysnqn, GFP_KERNEL);
+
+	dev = &loop_port->dev;
+	dev->bus = &nvme_loop_bus;
+	dev->parent = nvme_loop_primary;
+	dev->release = &nvme_loop_release_adapter;
+	dev_set_name(dev, "nvme_loop_ctrl:%s", subsys->subsysnqn);
+
+	port->priv = loop_port;
+	loop_port->port = port;
+
+	ret = device_register(dev);
+	if (ret) {
+		pr_err("device_register() failed: %d\n", ret);
+		kfree(loop_port);
+		return ret;
+	}
 
-	nvmet_loop_port = port;
 	return 0;
 }
 
 static void nvme_loop_remove_port(struct nvmet_port *port)
 {
-	if (port == nvmet_loop_port)
-		nvmet_loop_port = NULL;
+	struct nvme_loop_port *loop_port = port->priv;
+
+	if (!loop_port)
+		return;
+
+	device_unregister(&loop_port->dev);
 }
 
 static struct nvmet_fabrics_ops nvme_loop_ops = {
@@ -739,13 +838,59 @@ static struct nvmf_transport_ops nvme_loop_transport = {
 	.create_ctrl	= nvme_loop_create_ctrl,
 };
 
+static int nvme_loop_alloc_core_bus(void)
+{
+	int ret;
+
+	nvme_loop_primary = root_device_register("nvme_loop_0");
+	if (IS_ERR(nvme_loop_primary)) {
+		pr_err("Unable to allocate nvme_loop_primary\n");
+		return PTR_ERR(nvme_loop_primary);
+	}
+
+	ret = bus_register(&nvme_loop_bus);
+	if (ret) {
+		pr_err("bus_register() failed for nvme_loop_bus\n");
+		goto dev_unreg;
+	}
+
+	ret = driver_register(&nvme_loop_driverfs);
+	if (ret) {
+		pr_err("driver_register() failed for"
+				" nvme_loop_driverfs\n");
+		goto bus_unreg;
+	}
+
+	return ret;
+
+bus_unreg:
+	bus_unregister(&nvme_loop_bus);
+dev_unreg:
+	root_device_unregister(nvme_loop_primary);
+	return ret;
+}
+
+static void nvme_loop_release_core_bus(void)
+{
+	driver_unregister(&nvme_loop_driverfs);
+	bus_unregister(&nvme_loop_bus);
+	root_device_unregister(nvme_loop_primary);
+}
+
 static int __init nvme_loop_init_module(void)
 {
 	int ret;
 
-	ret = nvmet_register_transport(&nvme_loop_ops);
+	ret = nvme_loop_alloc_core_bus();
 	if (ret)
 		return ret;
+
+	ret = nvmet_register_transport(&nvme_loop_ops);
+	if (ret) {
+		nvme_loop_release_core_bus();
+		return ret;
+	}
+
 	nvmf_register_transport(&nvme_loop_transport);
 	return 0;
 }
@@ -763,6 +908,8 @@ static void __exit nvme_loop_cleanup_module(void)
 	mutex_unlock(&nvme_loop_ctrl_mutex);
 
 	flush_scheduled_work();
+
+	nvme_loop_release_core_bus();
 }
 
 module_init(nvme_loop_init_module);
-- 
1.9.1

Re: [RFC 0/2] nvme/loop: Add support for controllers-per-port model

[Christoph Hellwig]

Hi Nic,

multiple ports have been on the todo list ever since we put that short
cut in place, but your patches aren't really how this should work.

The host NQN is not available for the actual drivers - we need to be able
to virtualize it for containers for example, that's why we need to pass
it by pointer depending on the arguments.  Also there is no need to
add another sysfs interface - just like for real drivers we can simply
create the ports in configfs and assign an address to it, we just need
to stop ignoring it.

Something like the patch below is the right way, it just needs a bit more
fine tuning and proper test cases:

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 94e7829..ecd2c4c 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -57,6 +57,7 @@ struct nvme_loop_ctrl {
 	struct blk_mq_tag_set	tag_set;
 	struct nvme_loop_iod	async_event_iod;
 	struct nvme_ctrl	ctrl;
+	struct nvme_loop_port	*port;
 
 	struct nvmet_ctrl	*target_ctrl;
 	struct work_struct	delete_work;
@@ -74,11 +75,17 @@ struct nvme_loop_queue {
 	struct nvme_loop_ctrl	*ctrl;
 };
 
-static struct nvmet_port *nvmet_loop_port;
-
 static LIST_HEAD(nvme_loop_ctrl_list);
 static DEFINE_MUTEX(nvme_loop_ctrl_mutex);
 
+struct nvme_loop_port {
+	struct list_head	entry;
+	struct nvmet_port	*port;
+};
+
+static LIST_HEAD(nvme_loop_ports);
+static DEFINE_SPINLOCK(nvme_loop_port_lock);
+
 static void nvme_loop_queue_response(struct nvmet_req *nvme_req);
 static void nvme_loop_delete_ctrl(struct nvmet_ctrl *ctrl);
 
@@ -172,7 +179,7 @@ static int nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		return ret;
 
 	iod->cmd.common.flags |= NVME_CMD_SGL_METABUF;
-	iod->req.port = nvmet_loop_port;
+	iod->req.port = queue->ctrl->port->port;
 	if (!nvmet_req_init(&iod->req, &queue->nvme_cq,
 			&queue->nvme_sq, &nvme_loop_ops)) {
 		nvme_cleanup_cmd(req);
@@ -210,6 +217,7 @@ static void nvme_loop_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
 	iod->cmd.common.opcode = nvme_admin_async_event;
 	iod->cmd.common.command_id = NVME_LOOP_AQ_BLKMQ_DEPTH;
 	iod->cmd.common.flags |= NVME_CMD_SGL_METABUF;
+	iod->req.port = queue->ctrl->port->port;
 
 	if (!nvmet_req_init(&iod->req, &queue->nvme_cq, &queue->nvme_sq,
 			&nvme_loop_ops)) {
@@ -597,6 +605,20 @@ out_destroy_queues:
 	return ret;
 }
 
+static struct nvme_loop_port *
+__nvme_loop_find_port(const char *traddr)
+{
+	struct nvme_loop_port *p;
+
+	list_for_each_entry(p, &nvme_loop_ports, entry) {
+		if (!strncmp(traddr, p->port->disc_addr.traddr,
+				NVMF_TRADDR_SIZE))
+			return p;
+	}
+
+	return NULL;
+}
+
 static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 		struct nvmf_ctrl_options *opts)
 {
@@ -610,6 +632,17 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 	ctrl->ctrl.opts = opts;
 	INIT_LIST_HEAD(&ctrl->list);
 
+	spin_lock(&nvme_loop_port_lock);
+	ctrl->port = __nvme_loop_find_port(opts->traddr);
+	spin_unlock(&nvme_loop_port_lock);
+	if (!ctrl->port) {
+		ret = -EINVAL;
+		dev_warn(ctrl->ctrl.device,
+			 "could not find port at addr %s\n",
+			 opts->traddr);
+		goto out_put_ctrl;
+	}
+
 	INIT_WORK(&ctrl->delete_work, nvme_loop_del_ctrl_work);
 	INIT_WORK(&ctrl->reset_work, nvme_loop_reset_ctrl_work);
 
@@ -684,27 +717,40 @@ out_put_ctrl:
 
 static int nvme_loop_add_port(struct nvmet_port *port)
 {
-	/*
-	 * XXX: disalow adding more than one port so
-	 * there is no connection rejections when a
-	 * a subsystem is assigned to a port for which
-	 * loop doesn't have a pointer.
-	 * This scenario would be possible if we allowed
-	 * more than one port to be added and a subsystem
-	 * was assigned to a port other than nvmet_loop_port.
-	 */
+	struct nvme_loop_port *p, *old;
+
+	p = kmalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	spin_lock(&nvme_loop_port_lock);
+	old = __nvme_loop_find_port(port->disc_addr.traddr);
+	if (old)
+		goto out_dup;
 
-	if (nvmet_loop_port)
-		return -EPERM;
+	p->port = port;
+	list_add_tail_rcu(&p->entry, &nvme_loop_ports);
+	port->priv = p;
+	spin_unlock(&nvme_loop_port_lock);
 
-	nvmet_loop_port = port;
 	return 0;
+out_dup:
+	spin_unlock(&nvme_loop_port_lock);
+	kfree(p);
+	pr_err("duplicate port name: %s\n", port->disc_addr.traddr);
+	return -EINVAL;
 }
 
 static void nvme_loop_remove_port(struct nvmet_port *port)
 {
-	if (port == nvmet_loop_port)
-		nvmet_loop_port = NULL;
+	struct nvme_loop_port *p = port->priv;
+
+	spin_lock(&nvme_loop_port_lock);
+	list_del_rcu(&p->entry);
+	spin_unlock(&nvme_loop_port_lock);
+
+	synchronize_rcu();
+	kfree(p);
 }
 
 static struct nvmet_fabrics_ops nvme_loop_ops = {
@@ -718,6 +764,7 @@ static struct nvmet_fabrics_ops nvme_loop_ops = {
 
 static struct nvmf_transport_ops nvme_loop_transport = {
 	.name		= "loop",
+	.required_opts	= NVMF_OPT_TRADDR,
 	.create_ctrl	= nvme_loop_create_ctrl,
 };
 

Re: [RFC 0/2] nvme/loop: Add support for controllers-per-port model

[Nicholas A. Bellinger]

On Wed, 2016-06-08 at 14:14 +0200, Christoph Hellwig wrote:
> Hi Nic,
> 
> multiple ports have been on the todo list ever since we put that short
> cut in place, but your patches aren't really how this should work.
> 
> The host NQN is not available for the actual drivers - we need to be able
> to virtualize it for containers for example, that's why we need to pass
> it by pointer depending on the arguments.  Also there is no need to
> add another sysfs interface - just like for real drivers we can simply
> create the ports in configfs and assign an address to it, we just need
> to stop ignoring it.
> 

Not sure what you mean, but my patch does not propose 'to add another
sysfs interface'.

It simply follows what drivers/target/loopback/ already does for scsi,
and allocates a controller from nvmet/configfs-ng at subsystem NQN
port enable time.

I still don't see why a loopback controller on a port of an individual
subsystem NQN can't be created + deleted directly from configfs, and
operate independently of other loopback controllers on a port of a
different subsystem NQNs.

> Something like the patch below is the right way, it just needs a bit more
> fine tuning and proper test cases:

I don't see how adding a internal mutex for loopback ports, and doing
internal sequential list lookup holding the global nvmet_config_sem
whilst doing nvmet_fabric_ops->add_port() helps to scale at all..

AFAICT for loopback, neither of these is necessary.  Why can't a
loopback controller on a port for a individual subsystem NQN be created
+ deleted directly from configfs, independent of other subsystem NQN's
loopback controller ports..?

Now for rdma, just like with iscsi-target + iser with network portals,
we do need to keep an internal list of ports, given that ports can be
shared across different target endpoints.

That's the part that I'm working on next for nvmet/configfs-ng.

Re: [RFC 0/2] nvme/loop: Add support for controllers-per-port model

[Christoph Hellwig]

On Wed, Jun 08, 2016 at 10:13:53PM -0700, Nicholas A. Bellinger wrote:
> I still don't see why a loopback controller on a port of an individual
> subsystem NQN can't be created + deleted directly from configfs, and
> operate independently of other loopback controllers on a port of a
> different subsystem NQNs.

I think you are misunderstanding the NVMe entities - there is no port
of a subsystem.  There are port, and there are subsystems and there
is an N to M relation between them, not an N to one relation.

Multiple controllers may and usually will use the same port to access
one or more subsystems, but multiple controllers may also use different
ports to use the same subsystem.

For full NVMe functionality we thus need to be able to create all
these objects indepdently.   While loopback ports are extremtly cheap
I see no reason to introduce a shortcut to break this model for our
debug local implementation - the whole point of the loopback driver
is to give you the full NVMe over Fabrics experience without needing
an (possibly expensive) transport.

> > Something like the patch below is the right way, it just needs a bit more
> > fine tuning and proper test cases:
> 
> I don't see how adding a internal mutex for loopback ports, and doing
> internal sequential list lookup holding the global nvmet_config_sem
> whilst doing nvmet_fabric_ops->add_port() helps to scale at all..

Scalability of the controller creation is not the prime goal for the
nvme-loop driver.  It's supposed to be the simples possible implementation
of a NVMe over Fabrics transport.

> AFAICT for loopback, neither of these is necessary.  Why can't a
> loopback controller on a port for a individual subsystem NQN be created
> + deleted directly from configfs,

We could trivially do this, but I think it's counter productive.  NVMe
over Fabrics does not automatically create one or multiple controllers,
and nvme-loop is just another transport that follows the higher level
spec.

> independent of other subsystem NQN's
> loopback controller ports..?

The only limitation at the moment is that there is only one port.  I've
shown you a fairly trivial patch to lift that limitation.  Subsystem
can be created and deleted independently from each other, as can
controllers, and as can ports for transport where more than one can
exists.

Re: [RFC 0/2] nvme/loop: Add support for controllers-per-port model

[Nicholas A. Bellinger]

On Thu, 2016-06-09 at 15:39 +0200, Christoph Hellwig wrote:
> On Wed, Jun 08, 2016 at 10:13:53PM -0700, Nicholas A. Bellinger wrote:
> > I still don't see why a loopback controller on a port of an individual
> > subsystem NQN can't be created + deleted directly from configfs, and
> > operate independently of other loopback controllers on a port of a
> > different subsystem NQNs.
> 
> I think you are misunderstanding the NVMe entities - there is no port
> of a subsystem.  There are port, and there are subsystems and there
> is an N to M relation between them, not an N to one relation.
> 

Yes, the mapping of subsystem NQNs and ports works identically to how
iscsi IQNs and network portals work.

That is, network portals can be setup across multiple IQNs, or setup
specifically to one IQN.

> Multiple controllers may and usually will use the same port to access
> one or more subsystems, but multiple controllers may also use different
> ports to use the same subsystem.
> 

Yes.  Also similar to MC/S in a single IQN across multiple network
portals setup.  ;)

> For full NVMe functionality we thus need to be able to create all
> these objects indepdently.

Yes.

> While loopback ports are extremtly cheap
> I see no reason to introduce a shortcut to break this model for our
> debug local implementation - the whole point of the loopback driver
> is to give you the full NVMe over Fabrics experience without needing
> an (possibly expensive) transport.
> 

I think making this extra step for loopback in pointless, and just adds
unnecessary list lookups, and is something that configfs is better
suited to handle directly.

But loopback is a special case, both for drivers/target/loopback/ and
driver/nvme/target/loop.c code.

So why not just let configfs do it..?

However, your point is taken about using the global hostnqn for loopback
controllers in this patch.  Following what we do for SCSI loopback, I
think it makes the sense to have each loopback subsystem port setup it's
own hostnqn instead.

Updating the patch to do this.

> > > Something like the patch below is the right way, it just needs a bit more
> > > fine tuning and proper test cases:
> > 
> > I don't see how adding a internal mutex for loopback ports, and doing
> > internal sequential list lookup holding the global nvmet_config_sem
> > whilst doing nvmet_fabric_ops->add_port() helps to scale at all..
> 
> Scalability of the controller creation is not the prime goal for the
> nvme-loop driver.  It's supposed to be the simples possible implementation
> of a NVMe over Fabrics transport.
> 

Btw, I consider adding a global list for loopback to be 'less' simple.

> > AFAICT for loopback, neither of these is necessary.  Why can't a
> > loopback controller on a port for a individual subsystem NQN be created
> > + deleted directly from configfs,
> 
> We could trivially do this, but I think it's counter productive.  NVMe
> over Fabrics does not automatically create one or multiple controllers,
> and nvme-loop is just another transport that follows the higher level
> spec.
> 

I disagree.  The more interesting question is if to enforce a limit to
the amount of loopback ports that be allocated with this patch
per /sys/kernel/config/nvmet/subsystems/$NQN.FOO/.

That is, each echo 1 > ../$NQN_FOO/$ANY_LOOPBACK/enable will create it's
own controller, regardless if another loopback controller already exists
on the subsystem NQN.

Leaving that as-is for -v2.