Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1415485 > unrolled thread

NVMe over Fabrics target implementation

Back to article view | Back to linux.kernel

Contents

  NVMe over Fabrics target implementation Christoph Hellwig <hch@lst.de> - 2016-06-06 23:30 +0200
    [PATCH 3/3] nvme-loop: add a NVMe loopback host driver Christoph Hellwig <hch@lst.de> - 2016-06-06 23:30 +0200
    Re: NVMe over Fabrics target implementation "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-06-07 08:30 +0200
      Re: NVMe over Fabrics target implementation Christoph Hellwig <hch@lst.de> - 2016-06-07 13:00 +0200
        Re: NVMe over Fabrics target implementation "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-06-08 07:30 +0200
          Re: NVMe over Fabrics target implementation Christoph Hellwig <hch@lst.de> - 2016-06-08 14:20 +0200
            Re: NVMe over Fabrics target implementation Sagi Grimberg <sagi@lightbits.io> - 2016-06-08 15:20 +0200
              Re: NVMe over Fabrics target implementation Christoph Hellwig <hch@lst.de> - 2016-06-08 15:50 +0200
              Re: NVMe over Fabrics target implementation "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-06-09 06:40 +0200
                Re: NVMe over Fabrics target implementation Christoph Hellwig <hch@lst.de> - 2016-06-09 15:50 +0200
            Re: NVMe over Fabrics target implementation "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2016-06-09 05:40 +0200
      Re: NVMe over Fabrics target implementation Andy Grover <agrover@redhat.com> - 2016-06-07 23:10 +0200
        Re: NVMe over Fabrics target implementation Ming Lin <mlin@kernel.org> - 2016-06-07 23:20 +0200
    Re: NVMe over Fabrics target implementation Christoph Hellwig <hch@lst.de> - 2016-06-07 19:40 +0200

#1415485 — NVMe over Fabrics target implementation

This patch set adds a generic NVMe over Fabrics target. The
implementation conforms to the NVMe 1.2b specification (which
includes Fabrics) and provides the NVMe over Fabrics access
to Linux block devices.

The target implementation consists of several elements:

- NVMe target core: defines and manages the NVMe entities (subsystems,
  controllers, namespaces, ...) and their allocation, responsible
  for initial commands processing and correct orchestration of
  the stack setup and tear down.

- NVMe admin command implementation: responsible for parsing and
  servicing admin commands such as controller identify, set features,
  keep-alive, log page, ...).

- NVMe I/O command implementation: responsible for performing the actual
  I/O (Read, Write, Flush, Deallocate (aka Discard).  It is a very thin
  layer on top of the block layer and implements no logic of it's own.
  To support exporting file systems please use the loopback block driver
  in direct I/O mode, which gives very good performance.

- NVMe over Fabrics support: responsible for servicing Fabrics commands
  (connect, property get/set).

- NVMe over Fabrics discovery service: responsible to serve the Discovery
  log page through a special cut down Discovery controller.

The target is configured using configfs, and configurable entities are:

 - NVMe subsystems and namespaces
 - NVMe over Fabrics ports and referrals
 - Host ACLs for primitive access control - NVMe over Fabrics access
   control is still work in progress at the specification level and
   will be implemented once that work has finished.

To configure the target use the nvmetcli tool from
http://git.infradead.org/users/hch/nvmetcli.git, which includes detailed
setup documentation.

In addition to the Fabrics target implementation we provide a loopback
driver which also conforms the NVMe over Fabrics specification and allows
evaluation of the target stack with local access without requiring a real
fabric.

Various test cases are provided for this implementation: nvmetcli
contains a python testsuite that mostly stresses the configfs interface
of the target, and we have various integration tests prepared for the
kernel host and target which are available at:

	git://git.infradead.org/nvme-fabrics.git nvmf-selftests

Gitweb:

	http://git.infradead.org/nvme-fabrics.git/shortlog/refs/heads/nvmf-selftests

This repository also contains patches from all the series posted today
in case you prefer using a git repository over collecting patches.

[toc] | [next] | [standalone]

#1415489 — [PATCH 3/3] nvme-loop: add a NVMe loopback host driver

This patch implements adds nvme-loop which allows to access local devices
exported as NVMe over Fabrics namespaces. This module can be useful for
easy evaluation, testing and also feature experimentation.

To createa nvme-loop device you need to configure the NVMe target to
export a loop port (see the nvmetcli documentaton for that) and then
connect to it using

	nvme connect-all -t loop

which requires the very latest nvme-cli version with Fabrics support.

Signed-off-by: Jay Freyensee <james.p.freyensee@intel.com>
Signed-off-by: Ming Lin <ming.l@ssi.samsung.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/target/Kconfig  |  10 +
 drivers/nvme/target/Makefile |   2 +
 drivers/nvme/target/loop.c   | 753 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 765 insertions(+)
 create mode 100644 drivers/nvme/target/loop.c

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index acf0c07..b77ce43 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -13,4 +13,14 @@ config NVME_TARGET
 	  To configure the NVMe target you probably want to use the nvmetcli
 	  tool from http://git.infradead.org/users/hch/nvmetcli.git.
 
+config NVME_TARGET_LOOP
+	tristate "NVMe loopback device support"
+	depends on BLK_DEV_NVME
+	select NVME_TARGET
+	select NVME_FABRICS
+	select SG_POOL
+	help
+	  This enables the NVMe loopback device support, which can be useful
+	  to test NVMe host and target side features.
+
 	  If unsure, say N.
diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index b4600b6..e49ba60 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -1,5 +1,7 @@
 
 obj-$(CONFIG_NVME_TARGET)		+= nvmet.o
+obj-$(CONFIG_NVME_TARGET_LOOP)		+= nvme-loop.o
 
 nvmet-y		+= core.o configfs.o admin-cmd.o io-cmd.o fabrics-cmd.o \
 			discovery.o
+nvme-loop-y	+= loop.o
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
new file mode 100644
index 0000000..08b4fbb
--- /dev/null
+++ b/drivers/nvme/target/loop.c
@@ -0,0 +1,753 @@
+/*
+ * NVMe over Fabrics loopback device.
+ * Copyright (c) 2015-2016 HGST, a Western Digital Company.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/scatterlist.h>
+#include <linux/delay.h>
+#include <linux/blk-mq.h>
+#include <linux/nvme.h>
+#include <linux/module.h>
+#include <linux/parser.h>
+#include <linux/t10-pi.h>
+#include "nvmet.h"
+#include "../host/nvme.h"
+#include "../host/fabrics.h"
+
+#define NVME_LOOP_AQ_DEPTH		256
+
+#define NVME_LOOP_MAX_SEGMENTS		256
+
+/*
+ * We handle AEN commands ourselves and don't even let the
+ * block layer know about them.
+ */
+#define NVME_LOOP_NR_AEN_COMMANDS	1
+#define NVME_LOOP_AQ_BLKMQ_DEPTH	\
+	(NVME_LOOP_AQ_DEPTH - NVME_LOOP_NR_AEN_COMMANDS)
+
+struct nvme_loop_iod {
+	struct nvme_command	cmd;
+	struct nvme_completion	rsp;
+	struct nvmet_req	req;
+	struct nvme_loop_queue	*queue;
+	struct work_struct	work;
+	struct sg_table		sg_table;
+	struct scatterlist	first_sgl[];
+};
+
+struct nvme_loop_ctrl {
+	spinlock_t		lock;
+	struct nvme_loop_queue	*queues;
+	u32			queue_count;
+
+	struct blk_mq_tag_set	admin_tag_set;
+
+	struct list_head	list;
+	u64			cap;
+	struct blk_mq_tag_set	tag_set;
+	struct nvme_loop_iod	async_event_iod;
+	struct nvme_ctrl	ctrl;
+
+	struct nvmet_ctrl	*target_ctrl;
+	struct work_struct	delete_work;
+	struct work_struct	reset_work;
+};
+
+static inline struct nvme_loop_ctrl *to_loop_ctrl(struct nvme_ctrl *ctrl)
+{
+	return container_of(ctrl, struct nvme_loop_ctrl, ctrl);
+}
+
+struct nvme_loop_queue {
+	struct nvmet_cq		nvme_cq;
+	struct nvmet_sq		nvme_sq;
+	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);
+
+static void nvme_loop_queue_response(struct nvmet_req *nvme_req);
+static void nvme_loop_delete_ctrl(struct nvmet_ctrl *ctrl);
+
+static struct nvmet_fabrics_ops nvme_loop_ops;
+
+static inline int nvme_loop_queue_idx(struct nvme_loop_queue *queue)
+{
+	return queue - queue->ctrl->queues;
+}
+
+static void nvme_loop_complete_rq(struct request *req)
+{
+	struct nvme_loop_iod *iod = blk_mq_rq_to_pdu(req);
+	int error = 0;
+
+	nvme_cleanup_cmd(req);
+	sg_free_table_chained(&iod->sg_table, true);
+
+	if (unlikely(req->errors)) {
+		if (nvme_req_needs_retry(req, req->errors)) {
+			nvme_requeue_req(req);
+			return;
+		}
+
+		if (req->cmd_type == REQ_TYPE_DRV_PRIV)
+			error = req->errors;
+		else
+			error = nvme_error_status(req->errors);
+	}
+
+	blk_mq_end_request(req, error);
+}
+
+static void nvme_loop_queue_response(struct nvmet_req *nvme_req)
+{
+	struct nvme_loop_iod *iod =
+		container_of(nvme_req, struct nvme_loop_iod, req);
+	struct nvme_completion *cqe = &iod->rsp;
+
+	/*
+	 * AEN requests are special as they don't time out and can
+	 * survive any kind of queue freeze and often don't respond to
+	 * aborts.  We don't even bother to allocate a struct request
+	 * for them but rather special case them here.
+	 */
+	if (unlikely(nvme_loop_queue_idx(iod->queue) == 0 &&
+			cqe->command_id >= NVME_LOOP_AQ_BLKMQ_DEPTH)) {
+		nvme_complete_async_event(&iod->queue->ctrl->ctrl, cqe);
+	} else {
+		struct request *req = blk_mq_rq_from_pdu(iod);
+
+		if (req->cmd_type == REQ_TYPE_DRV_PRIV && req->special)
+			memcpy(req->special, cqe, sizeof(*cqe));
+		blk_mq_complete_request(req, le16_to_cpu(cqe->status) >> 1);
+	}
+}
+
+static void nvme_loop_execute_work(struct work_struct *work)
+{
+	struct nvme_loop_iod *iod =
+		container_of(work, struct nvme_loop_iod, work);
+
+	iod->req.execute(&iod->req);
+}
+
+static enum blk_eh_timer_return
+nvme_loop_timeout(struct request *rq, bool reserved)
+{
+	struct nvme_loop_iod *iod = blk_mq_rq_to_pdu(rq);
+
+	/* queue error recovery */
+	schedule_work(&iod->queue->ctrl->reset_work);
+
+	/* fail with DNR on admin cmd timeout */
+	rq->errors = NVME_SC_ABORT_REQ | NVME_SC_DNR;
+
+	return BLK_EH_HANDLED;
+}
+
+static int nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
+		const struct blk_mq_queue_data *bd)
+{
+	struct nvme_ns *ns = hctx->queue->queuedata;
+	struct nvme_loop_queue *queue = hctx->driver_data;
+	struct request *req = bd->rq;
+	struct nvme_loop_iod *iod = blk_mq_rq_to_pdu(req);
+	int ret;
+
+	ret = nvme_setup_cmd(ns, req, &iod->cmd);
+	if (ret)
+		return ret;
+
+	iod->cmd.common.flags |= NVME_CMD_SGL_METABUF;
+	iod->req.port = nvmet_loop_port;
+	if (!nvmet_req_init(&iod->req, &queue->nvme_cq,
+			&queue->nvme_sq, &nvme_loop_ops)) {
+		nvme_cleanup_cmd(req);
+		blk_mq_start_request(req);
+		nvme_loop_queue_response(&iod->req);
+		return 0;
+	}
+
+	if (blk_rq_bytes(req)) {
+		iod->sg_table.sgl = iod->first_sgl;
+		ret = sg_alloc_table_chained(&iod->sg_table,
+			req->nr_phys_segments, iod->sg_table.sgl);
+		if (ret)
+			return BLK_MQ_RQ_QUEUE_BUSY;
+
+		iod->req.sg = iod->sg_table.sgl;
+		iod->req.sg_cnt = blk_rq_map_sg(req->q, req, iod->sg_table.sgl);
+		BUG_ON(iod->req.sg_cnt > req->nr_phys_segments);
+	}
+
+	iod->cmd.common.command_id = req->tag;
+	blk_mq_start_request(req);
+
+	schedule_work(&iod->work);
+	return 0;
+}
+
+static void nvme_loop_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
+{
+	struct nvme_loop_ctrl *ctrl = to_loop_ctrl(arg);
+	struct nvme_loop_queue *queue = &ctrl->queues[0];
+	struct nvme_loop_iod *iod = &ctrl->async_event_iod;
+
+	memset(&iod->cmd, 0, sizeof(iod->cmd));
+	iod->cmd.common.opcode = nvme_admin_async_event;
+	iod->cmd.common.command_id = NVME_LOOP_AQ_BLKMQ_DEPTH;
+
+	if (!nvmet_req_init(&iod->req, &queue->nvme_cq, &queue->nvme_sq,
+			&nvme_loop_ops)) {
+		dev_err(ctrl->ctrl.device, "failed async event work\n");
+		return;
+	}
+
+	schedule_work(&iod->work);
+}
+
+static int nvme_loop_init_iod(struct nvme_loop_ctrl *ctrl,
+		struct nvme_loop_iod *iod, unsigned int queue_idx)
+{
+	BUG_ON(queue_idx >= ctrl->queue_count);
+
+	iod->req.cmd = &iod->cmd;
+	iod->req.rsp = &iod->rsp;
+	iod->queue = &ctrl->queues[queue_idx];
+	INIT_WORK(&iod->work, nvme_loop_execute_work);
+	return 0;
+}
+
+static int nvme_loop_init_request(void *data, struct request *req,
+				unsigned int hctx_idx, unsigned int rq_idx,
+				unsigned int numa_node)
+{
+	return nvme_loop_init_iod(data, blk_mq_rq_to_pdu(req), hctx_idx + 1);
+}
+
+static int nvme_loop_init_admin_request(void *data, struct request *req,
+				unsigned int hctx_idx, unsigned int rq_idx,
+				unsigned int numa_node)
+{
+	return nvme_loop_init_iod(data, blk_mq_rq_to_pdu(req), 0);
+}
+
+static int nvme_loop_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
+		unsigned int hctx_idx)
+{
+	struct nvme_loop_ctrl *ctrl = data;
+	struct nvme_loop_queue *queue = &ctrl->queues[hctx_idx + 1];
+
+	BUG_ON(hctx_idx >= ctrl->queue_count);
+
+	hctx->driver_data = queue;
+	return 0;
+}
+
+static int nvme_loop_init_admin_hctx(struct blk_mq_hw_ctx *hctx, void *data,
+		unsigned int hctx_idx)
+{
+	struct nvme_loop_ctrl *ctrl = data;
+	struct nvme_loop_queue *queue = &ctrl->queues[0];
+
+	BUG_ON(hctx_idx != 0);
+
+	hctx->driver_data = queue;
+	return 0;
+}
+
+static struct blk_mq_ops nvme_loop_mq_ops = {
+	.queue_rq	= nvme_loop_queue_rq,
+	.complete	= nvme_loop_complete_rq,
+	.map_queue	= blk_mq_map_queue,
+	.init_request	= nvme_loop_init_request,
+	.init_hctx	= nvme_loop_init_hctx,
+	.timeout	= nvme_loop_timeout,
+};
+
+static struct blk_mq_ops nvme_loop_admin_mq_ops = {
+	.queue_rq	= nvme_loop_queue_rq,
+	.complete	= nvme_loop_complete_rq,
+	.map_queue	= blk_mq_map_queue,
+	.init_request	= nvme_loop_init_admin_request,
+	.init_hctx	= nvme_loop_init_admin_hctx,
+	.timeout	= nvme_loop_timeout,
+};
+
+static void nvme_loop_destroy_admin_queue(struct nvme_loop_ctrl *ctrl)
+{
+	blk_cleanup_queue(ctrl->ctrl.admin_q);
+	blk_mq_free_tag_set(&ctrl->admin_tag_set);
+	nvmet_sq_destroy(&ctrl->queues[0].nvme_sq);
+}
+
+static void nvme_loop_free_ctrl(struct nvme_ctrl *nctrl)
+{
+	struct nvme_loop_ctrl *ctrl = to_loop_ctrl(nctrl);
+
+	if (list_empty(&ctrl->list))
+		goto free_ctrl;
+
+	mutex_lock(&nvme_loop_ctrl_mutex);
+	list_del(&ctrl->list);
+	mutex_unlock(&nvme_loop_ctrl_mutex);
+
+	if (nctrl->tagset) {
+		blk_cleanup_queue(ctrl->ctrl.connect_q);
+		blk_mq_free_tag_set(&ctrl->tag_set);
+	}
+	kfree(ctrl->queues);
+	nvmf_free_options(nctrl->opts);
+free_ctrl:
+	kfree(ctrl);
+}
+
+static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
+{
+	int error;
+
+	memset(&ctrl->admin_tag_set, 0, sizeof(ctrl->admin_tag_set));
+	ctrl->admin_tag_set.ops = &nvme_loop_admin_mq_ops;
+	ctrl->admin_tag_set.queue_depth = NVME_LOOP_AQ_BLKMQ_DEPTH;
+	ctrl->admin_tag_set.reserved_tags = 2; /* connect + keep-alive */
+	ctrl->admin_tag_set.numa_node = NUMA_NO_NODE;
+	ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_loop_iod) +
+		SG_CHUNK_SIZE * sizeof(struct scatterlist);
+	ctrl->admin_tag_set.driver_data = ctrl;
+	ctrl->admin_tag_set.nr_hw_queues = 1;
+	ctrl->admin_tag_set.timeout = ADMIN_TIMEOUT;
+
+	ctrl->queues[0].ctrl = ctrl;
+	error = nvmet_sq_init(&ctrl->queues[0].nvme_sq);
+	if (error)
+		return error;
+	ctrl->queue_count = 1;
+
+	error = blk_mq_alloc_tag_set(&ctrl->admin_tag_set);
+	if (error)
+		goto out_free_sq;
+
+	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 = nvmf_connect_admin_queue(&ctrl->ctrl);
+	if (error)
+		goto out_cleanup_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_cleanup_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_cleanup_queue;
+
+	ctrl->ctrl.max_hw_sectors =
+		(NVME_LOOP_MAX_SEGMENTS - 1) << (PAGE_SHIFT - 9);
+
+	error = nvme_init_identify(&ctrl->ctrl);
+	if (error)
+		goto out_cleanup_queue;
+
+	nvme_start_keep_alive(&ctrl->ctrl);
+
+	return 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_sq:
+	nvmet_sq_destroy(&ctrl->queues[0].nvme_sq);
+	return error;
+}
+
+static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
+{
+	int i;
+
+	nvme_stop_keep_alive(&ctrl->ctrl);
+
+	if (ctrl->queue_count > 1) {
+		nvme_stop_queues(&ctrl->ctrl);
+		blk_mq_tagset_busy_iter(&ctrl->tag_set,
+					nvme_cancel_request, &ctrl->ctrl);
+
+		for (i = 1; i < ctrl->queue_count; i++)
+			nvmet_sq_destroy(&ctrl->queues[i].nvme_sq);
+	}
+
+	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
+		nvme_shutdown_ctrl(&ctrl->ctrl);
+
+	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
+	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
+				nvme_cancel_request, &ctrl->ctrl);
+	nvme_loop_destroy_admin_queue(ctrl);
+}
+
+static void nvme_loop_del_ctrl_work(struct work_struct *work)
+{
+	struct nvme_loop_ctrl *ctrl = container_of(work,
+				struct nvme_loop_ctrl, delete_work);
+
+	nvme_remove_namespaces(&ctrl->ctrl);
+	nvme_loop_shutdown_ctrl(ctrl);
+	nvme_uninit_ctrl(&ctrl->ctrl);
+	nvme_put_ctrl(&ctrl->ctrl);
+}
+
+static int __nvme_loop_del_ctrl(struct nvme_loop_ctrl *ctrl)
+{
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
+		return -EBUSY;
+
+	if (!schedule_work(&ctrl->delete_work))
+		return -EBUSY;
+
+	return 0;
+}
+
+static int nvme_loop_del_ctrl(struct nvme_ctrl *nctrl)
+{
+	struct nvme_loop_ctrl *ctrl = to_loop_ctrl(nctrl);
+	int ret;
+
+	ret = __nvme_loop_del_ctrl(ctrl);
+	if (ret)
+		return ret;
+
+	flush_work(&ctrl->delete_work);
+
+	return 0;
+}
+
+static void nvme_loop_delete_ctrl(struct nvmet_ctrl *nctrl)
+{
+	struct nvme_loop_ctrl *ctrl;
+
+	mutex_lock(&nvme_loop_ctrl_mutex);
+	list_for_each_entry(ctrl, &nvme_loop_ctrl_list, list) {
+		if (ctrl->ctrl.cntlid == nctrl->cntlid)
+			__nvme_loop_del_ctrl(ctrl);
+	}
+	mutex_unlock(&nvme_loop_ctrl_mutex);
+}
+
+static void nvme_loop_reset_ctrl_work(struct work_struct *work)
+{
+	struct nvme_loop_ctrl *ctrl = container_of(work,
+					struct nvme_loop_ctrl, reset_work);
+	bool changed;
+	int i, ret;
+
+	nvme_loop_shutdown_ctrl(ctrl);
+
+	ret = nvme_loop_configure_admin_queue(ctrl);
+	if (ret)
+		goto out_disable;
+
+	for (i = 1; i <= ctrl->ctrl.opts->nr_io_queues; i++) {
+		ctrl->queues[i].ctrl = ctrl;
+		ret = nvmet_sq_init(&ctrl->queues[i].nvme_sq);
+		if (ret)
+			goto out_free_queues;
+
+		ctrl->queue_count++;
+	}
+
+	for (i = 1; i <= ctrl->ctrl.opts->nr_io_queues; i++) {
+		ret = nvmf_connect_io_queue(&ctrl->ctrl, i);
+		if (ret)
+			goto out_free_queues;
+	}
+
+	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
+	WARN_ON_ONCE(!changed);
+
+	nvme_queue_scan(&ctrl->ctrl);
+	nvme_queue_async_events(&ctrl->ctrl);
+
+	nvme_start_queues(&ctrl->ctrl);
+
+	return;
+
+out_free_queues:
+	for (i = 1; i < ctrl->queue_count; i++)
+		nvmet_sq_destroy(&ctrl->queues[i].nvme_sq);
+	nvme_loop_destroy_admin_queue(ctrl);
+out_disable:
+	dev_warn(ctrl->ctrl.device, "Removing after reset failure\n");
+	nvme_remove_namespaces(&ctrl->ctrl);
+	nvme_uninit_ctrl(&ctrl->ctrl);
+	nvme_put_ctrl(&ctrl->ctrl);
+}
+
+static int nvme_loop_reset_ctrl(struct nvme_ctrl *nctrl)
+{
+	struct nvme_loop_ctrl *ctrl = to_loop_ctrl(nctrl);
+
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
+		return -EBUSY;
+
+	if (!schedule_work(&ctrl->reset_work))
+		return -EBUSY;
+
+	flush_work(&ctrl->reset_work);
+
+	return 0;
+}
+
+static const struct nvme_ctrl_ops nvme_loop_ctrl_ops = {
+	.name			= "loop",
+	.module			= THIS_MODULE,
+	.is_fabrics		= true,
+	.reg_read32		= nvmf_reg_read32,
+	.reg_read64		= nvmf_reg_read64,
+	.reg_write32		= nvmf_reg_write32,
+	.reset_ctrl		= nvme_loop_reset_ctrl,
+	.free_ctrl		= nvme_loop_free_ctrl,
+	.submit_async_event	= nvme_loop_submit_async_event,
+	.delete_ctrl		= nvme_loop_del_ctrl,
+	.get_subsysnqn		= nvmf_get_subsysnqn,
+};
+
+static int nvme_loop_create_io_queues(struct nvme_loop_ctrl *ctrl)
+{
+	struct nvmf_ctrl_options *opts = ctrl->ctrl.opts;
+	int ret, i;
+
+	ret = nvme_set_queue_count(&ctrl->ctrl, &opts->nr_io_queues);
+	if (ret || !opts->nr_io_queues)
+		return ret;
+
+	dev_info(ctrl->ctrl.device, "creating %d I/O queues.\n",
+		opts->nr_io_queues);
+
+	for (i = 1; i <= opts->nr_io_queues; i++) {
+		ctrl->queues[i].ctrl = ctrl;
+		ret = nvmet_sq_init(&ctrl->queues[i].nvme_sq);
+		if (ret)
+			goto out_destroy_queues;
+
+		ctrl->queue_count++;
+	}
+
+	memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
+	ctrl->tag_set.ops = &nvme_loop_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_loop_iod) +
+		SG_CHUNK_SIZE * sizeof(struct scatterlist);
+	ctrl->tag_set.driver_data = ctrl;
+	ctrl->tag_set.nr_hw_queues = ctrl->queue_count - 1;
+	ctrl->tag_set.timeout = NVME_IO_TIMEOUT;
+	ctrl->ctrl.tagset = &ctrl->tag_set;
+
+	ret = blk_mq_alloc_tag_set(&ctrl->tag_set);
+	if (ret)
+		goto out_destroy_queues;
+
+	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_tagset;
+	}
+
+	for (i = 1; i <= opts->nr_io_queues; i++) {
+		ret = nvmf_connect_io_queue(&ctrl->ctrl, i);
+		if (ret)
+			goto out_cleanup_connect_q;
+	}
+
+	return 0;
+
+out_cleanup_connect_q:
+	blk_cleanup_queue(ctrl->ctrl.connect_q);
+out_free_tagset:
+	blk_mq_free_tag_set(&ctrl->tag_set);
+out_destroy_queues:
+	for (i = 1; i < ctrl->queue_count; i++)
+		nvmet_sq_destroy(&ctrl->queues[i].nvme_sq);
+	return ret;
+}
+
+static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
+		struct nvmf_ctrl_options *opts)
+{
+	struct nvme_loop_ctrl *ctrl;
+	bool changed;
+	int ret;
+
+	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return ERR_PTR(-ENOMEM);
+	ctrl->ctrl.opts = opts;
+	INIT_LIST_HEAD(&ctrl->list);
+
+	INIT_WORK(&ctrl->delete_work, nvme_loop_del_ctrl_work);
+	INIT_WORK(&ctrl->reset_work, nvme_loop_reset_ctrl_work);
+
+	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_loop_ctrl_ops,
+				0 /* no quirks, we're perfect! */);
+	if (ret)
+		goto out_put_ctrl;
+
+	spin_lock_init(&ctrl->lock);
+
+	ret = -ENOMEM;
+
+	ctrl->ctrl.sqsize = opts->queue_size;
+	ctrl->ctrl.kato = opts->kato;
+
+	ctrl->queues = kcalloc(opts->nr_io_queues + 1, sizeof(*ctrl->queues),
+			GFP_KERNEL);
+	if (!ctrl->queues)
+		goto out_uninit_ctrl;
+
+	ret = nvme_loop_configure_admin_queue(ctrl);
+	if (ret)
+		goto out_free_queues;
+
+	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, clamping down\n",
+			opts->queue_size, ctrl->ctrl.maxcmd);
+		opts->queue_size = ctrl->ctrl.maxcmd;
+	}
+
+	if (opts->nr_io_queues) {
+		ret = nvme_loop_create_io_queues(ctrl);
+		if (ret)
+			goto out_remove_admin_queue;
+	}
+
+	nvme_loop_init_iod(ctrl, &ctrl->async_event_iod, 0);
+
+	dev_info(ctrl->ctrl.device,
+		 "new ctrl: \"%s\"\n", ctrl->ctrl.opts->subsysnqn);
+
+	kref_get(&ctrl->ctrl.kref);
+
+	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
+	WARN_ON_ONCE(!changed);
+
+	mutex_lock(&nvme_loop_ctrl_mutex);
+	list_add_tail(&ctrl->list, &nvme_loop_ctrl_list);
+	mutex_unlock(&nvme_loop_ctrl_mutex);
+
+	if (opts->nr_io_queues) {
+		nvme_queue_scan(&ctrl->ctrl);
+		nvme_queue_async_events(&ctrl->ctrl);
+	}
+
+	return &ctrl->ctrl;
+
+out_remove_admin_queue:
+	nvme_loop_destroy_admin_queue(ctrl);
+out_free_queues:
+	kfree(ctrl->queues);
+out_uninit_ctrl:
+	nvme_uninit_ctrl(&ctrl->ctrl);
+out_put_ctrl:
+	nvme_put_ctrl(&ctrl->ctrl);
+	if (ret > 0)
+		ret = -EIO;
+	return ERR_PTR(ret);
+}
+
+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.
+	 */
+
+	if (nvmet_loop_port)
+		return -EPERM;
+
+	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;
+}
+
+static struct nvmet_fabrics_ops nvme_loop_ops = {
+	.owner		= THIS_MODULE,
+	.type		= NVMF_TRTYPE_LOOP,
+	.add_port	= nvme_loop_add_port,
+	.remove_port	= nvme_loop_remove_port,
+	.queue_response = nvme_loop_queue_response,
+	.delete_ctrl	= nvme_loop_delete_ctrl,
+};
+
+static struct nvmf_transport_ops nvme_loop_transport = {
+	.name		= "loop",
+	.create_ctrl	= nvme_loop_create_ctrl,
+};
+
+static int __init nvme_loop_init_module(void)
+{
+	int ret;
+
+	ret = nvmet_register_transport(&nvme_loop_ops);
+	if (ret)
+		return ret;
+	nvmf_register_transport(&nvme_loop_transport);
+	return 0;
+}
+
+static void __exit nvme_loop_cleanup_module(void)
+{
+	struct nvme_loop_ctrl *ctrl, *next;
+
+	nvmf_unregister_transport(&nvme_loop_transport);
+	nvmet_unregister_transport(&nvme_loop_ops);
+
+	mutex_lock(&nvme_loop_ctrl_mutex);
+	list_for_each_entry_safe(ctrl, next, &nvme_loop_ctrl_list, list)
+		__nvme_loop_del_ctrl(ctrl);
+	mutex_unlock(&nvme_loop_ctrl_mutex);
+
+	flush_scheduled_work();
+}
+
+module_init(nvme_loop_init_module);
+module_exit(nvme_loop_cleanup_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("nvmet-transport-254"); /* 254 == NVMF_TRTYPE_LOOP */
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1415736

Hi HCH & Co,

On Mon, 2016-06-06 at 23:22 +0200, Christoph Hellwig wrote:
> This patch set adds a generic NVMe over Fabrics target. The
> implementation conforms to the NVMe 1.2b specification (which
> includes Fabrics) and provides the NVMe over Fabrics access
> to Linux block devices.
> 

Thanks for all of the development work by the fabric_linux_driver team
(HCH, Sagi, Ming, James F., James S., and Dave M.) over the last year. 

Very excited to see this code get a public release now that NVMf
specification is out.  Now that it's in the wild, it's a good
opportunity to discuss some of the more interesting implementation
details, beyond the new NVMf wire-protocol itself.

(Adding target-devel + linux-scsi CC')

> The target implementation consists of several elements:
> 
> - NVMe target core: defines and manages the NVMe entities (subsystems,
>   controllers, namespaces, ...) and their allocation, responsible
>   for initial commands processing and correct orchestration of
>   the stack setup and tear down.
> 
> - NVMe admin command implementation: responsible for parsing and
>   servicing admin commands such as controller identify, set features,
>   keep-alive, log page, ...).
> 
> - NVMe I/O command implementation: responsible for performing the actual
>   I/O (Read, Write, Flush, Deallocate (aka Discard).  It is a very thin
>   layer on top of the block layer and implements no logic of it's own.
>   To support exporting file systems please use the loopback block driver
>   in direct I/O mode, which gives very good performance.
> 
> - NVMe over Fabrics support: responsible for servicing Fabrics commands
>   (connect, property get/set).
> 
> - NVMe over Fabrics discovery service: responsible to serve the Discovery
>   log page through a special cut down Discovery controller.
> 
> The target is configured using configfs, and configurable entities are:
> 
>  - NVMe subsystems and namespaces
>  - NVMe over Fabrics ports and referrals
>  - Host ACLs for primitive access control - NVMe over Fabrics access
>    control is still work in progress at the specification level and
>    will be implemented once that work has finished.
> 
> To configure the target use the nvmetcli tool from
> http://git.infradead.org/users/hch/nvmetcli.git, which includes detailed
> setup documentation.
> 
> In addition to the Fabrics target implementation we provide a loopback
> driver which also conforms the NVMe over Fabrics specification and allows
> evaluation of the target stack with local access without requiring a real
> fabric.
> 

So as-is, I have two main objections that been discussed off-list for
some time, that won't be a big surprise to anyone following
fabrics_linux_driver list.  ;P

First topic, I think nvme-target name-spaces should be utilizing
existing configfs logic, and sharing /sys/kernel/config/target/core/
backend driver symlinks as individual nvme-target subsystem namespaces.

That is, we've already got a configfs ABI in place for target mode
back-ends that today is able to operate independently from SCSI
architecture model dependencies.

To that end, the prerequisite series to allow target-core backends to
operate independent of se_cmd, and allow se_device backends to be
configfs symlinked directly into /sys/kernel/config/nvmet/, outside
of /sys/kernel/config/target/$FABRIC/ has been posted earlier here:

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

Note the -v2 series has absorbed the nvmet/io-cmd execute_rw()
improvements from Sagi + Ming (inline bio/bvec and blk_poll) into
target_core_iblock.c driver code.

Second topic, and more important from a kernel ABI perspective are the
current scale limitations around the first pass of nvmet configfs.c
layout code in /sys/kernel/config/nvmet/.

Namely, the design of having three top level configfs groups in
/sys/kernel/config/nvmet/[subsystems,ports,hosts] that are configfs
symlinked between each other, with a single rw_mutex (nvmet_config_sem)
used for global list lookup and enforcing a globally synchronized
nvmet_fabrics_ops->add_port() creation across all subsystem NQN ports.

From the shared experience in target_core_fabric_configfs.c over the
last 8 years, perhaps the greatest strength of configfs has been it's
ability to allow config_item_type parent/child relationships to exist
and operate independently of one another.

Specifically in the context of storage tenants, this means creation +
deletion of one backend + target fabric endpoint tenant, should not
block creation + deletion of another backend + target fabric endpoint
tenant.

As-is, a nvmet configfs layout holding a global mutex across
subsystem/port/host creation + deletion, and doing internal list lookup
within configfs ->allow_link + ->drop_link callbacks ends up being
severely limiting when scaling up the total number of nvmet subsystem
NQNs and ports.

Specifically, modern deployments of /sys/kernel/config/target/iscsi/
expect backends + fabric endpoints to be configured in parallel at
< 100ms from user-space, in order to actively migrate and fail-over
100s of storage instances (eg: iscsi IQNs -> NVMf NQN) across physical
cluster nodes and L3 networks.

So in order to reach this level of scale with nvmet/configfs, the layout
I think is necessary to match iscsi-target in a multi-tenant environment
will, in it's most basic form look like:

/sys/kernel/config/nvmet/subsystems/
└── nqn.2003-01.org.linux-iscsi.NVMf.skylake-ep
    ├── hosts
    ├── namespaces
    │   └── ns_1
    │       └── 1 -> ../../../../../../target/core/rd_mcp_1/ramdisk0
    └── ports
        ├── pcie:$SUPER_TURBO_FABRIC_EAST
        ├── pcie:$SUPER_TURBO_FABRIC_WEST
        ├── rdma:[$IPV6_ADDR]:$PORT
        ├── rdma:10.10.1.75:$PORT
        └── loop

That is, both NQN ports groups and host ACL groups exist below the
nvmet_subsys->group, and NQN namespaces are configfs symlinked directly
from /sys/kernel/config/target/core/ backends as mentioned in point #1.

To that end, I'll be posting a nvmet series shortly that implements a
multi-tenant configfs layout WIP using nvme/loop, using existing
target-core backends as configfs symlinked nvme namespaces.

Comments..?

[toc] | [prev] | [next] | [standalone]

#1415985

There is absolutely no point in dragging in an overcomplicated configfs 
structure for a very simple protocol which also is very different from
SCSI in it's nitty gritty details.  Keeping the nvme target self contains
allows it to be both much simpler and much easier to understand, as well
as much better testable - see the amount of test coverage we could easily
add for example.

Or to put it the other way around - if there was any major synergy in
reusing the SCSI target code that just shows we're missing functionality
in the block layer or configfs.

The only thing where this is the case mid-term is persistent reservations,
but Mike Christie already has a plan for a pluggable PR API, which I'm
very interested in.

[toc] | [prev] | [next] | [standalone]

#1416875

On Tue, 2016-06-07 at 12:55 +0200, Christoph Hellwig wrote:
> There is absolutely no point in dragging in an overcomplicated configfs 
> structure for a very simple protocol which also is very different from
> SCSI in it's nitty gritty details.

Please be more specific wrt the two individual points that have been
raised.

>  Keeping the nvme target self contains
> allows it to be both much simpler and much easier to understand, as well
> as much better testable - see the amount of test coverage we could easily
> add for example.

I disagree.

> 
> Or to put it the other way around - if there was any major synergy in
> reusing the SCSI target code that just shows we're missing functionality
> in the block layer or configfs.
> 

To reiterate the points again.

*) Extensible to multiple types of backend drivers.

nvme-target needs a way to absorb new backend drivers, that
does not effect existing configfs group layout or attributes.

Looking at the nvmet/configfs layout as-is, there are no multiple
backend types defined, nor a way to control backend feature bits
exposed to nvme namespaces at runtime.

What is being proposed is a way to share target-core backends via
existing configfs symlinks across SCSI and NVMe targets.

Which means:

   - All I/O state + memory submission is done at RCU protected
     se_device level via sbc_ops
   - percpu reference counting is done outside of target-core
   - Absorb all nvmet/io-cmd optimizations into target_core_iblock.c
   - Base starting point for features in SCSI + NVMe that span
     across multiple endpoints and instances (reservations + APTPL, 
     multipath, copy-offload across fabric types)

Using target-core backends means we get features like T10-PI and
sbc_ops->write_same for free that don't exist in nvmet, and can
utilize a common set of backend drivers for SCSI and NVMe via an
existing configfs ABI and python userspace community.

And to the second, and more important point for defining a configfs ABI
that works for both today's requirements, as well into the 2020s
without breaking user-space compatibility.

As-is, the initial design using top level nvmet configfs symlinks of
subsystem groups into individual port + host groups does not scale.

That is, it currently does:

  - Sequential list lookup under global rw_mutex of top-level nvmet_port
    and nvmet_host symlink ->allow_link() and ->drop_link() configfs
    callbacks.
  - nvmet_fabrics_ops->add_port() callback invoked under same global
    rw mutex.

This is very bad for several reasons.

As-is, this blocks all other configfs port + host operations from
occurring even during normal operation, which makes it quite useless for
any type of multi-tenant target environment where the individual target
endpoints *must* be able to operate independently.

Seriously, there is never a good reason why configfs group or item
callbacks should be performing list lookup under a global lock at
this level.

Why does it ever make sense for $SUBSYSTEM_NQN_0 with $PORT_DRIVER_FOO
to block operation of $SUBSYSTEM_NQN_1 with $PORT_DRIVER_BAR..?

A simple example where this design breaks down quickly is a NVMf
ops->add_port() call that requires a HW reset, or say reloading of
firmware that can take multiple seconds. (qla2xxx comes to mind).

There is a simple test to highlight this limitation.  Take any
nvme-target driver that is capable of multiple ports, and introduce
a sleep(5) into each ops->add_port() call.

Now create 256 different subsystem NQNs with 256 different ports
across four different user-space processes.

What happens to other subsystems, ports and host groups configfs
symlinks when this occurs..?

What happens to the other user-space processes..?

[toc] | [prev] | [next] | [standalone]

#1417345

On Tue, Jun 07, 2016 at 10:21:41PM -0700, Nicholas A. Bellinger wrote:
> *) Extensible to multiple types of backend drivers.
> 
> nvme-target needs a way to absorb new backend drivers, that
> does not effect existing configfs group layout or attributes.
> 
> Looking at the nvmet/configfs layout as-is, there are no multiple
> backend types defined, nor a way to control backend feature bits
> exposed to nvme namespaces at runtime.

And that's very much intentional.  We have a very well working block
layer which we're going to use, no need to reivent it.  The block
layer supports NVMe pass through just fine in case we'll need it,
as I spent the last year preparing it for that.

> Why does it ever make sense for $SUBSYSTEM_NQN_0 with $PORT_DRIVER_FOO
> to block operation of $SUBSYSTEM_NQN_1 with $PORT_DRIVER_BAR..?

Because it keeps the code simple.  If you had actually participated
on our development list you might have seen that until not too long
ago we have very fine grainded locks here.  In the end Armen convinced
me that it's easier to maintain if we don't bother with fine grained
locking outside the fast path, especially as it significantly simplifies
the discovery implementation.   If if it ever turns out to be an
issue we can change it easily as the implementation is well encapsulated.

[toc] | [prev] | [next] | [standalone]

#1417410

>> *) Extensible to multiple types of backend drivers.
>>
>> nvme-target needs a way to absorb new backend drivers, that
>> does not effect existing configfs group layout or attributes.
>>
>> Looking at the nvmet/configfs layout as-is, there are no multiple
>> backend types defined, nor a way to control backend feature bits
>> exposed to nvme namespaces at runtime.

Hey Nic,

As for different type of backends, I still don't see a big justification
for adding the LIO backends pscsi (as it doesn't make sense),
ramdisk (we have brd), or file (losetup).

What kind of feature bits would you want to expose at runtime?

> And that's very much intentional.  We have a very well working block
> layer which we're going to use, no need to reivent it.  The block
> layer supports NVMe pass through just fine in case we'll need it,
> as I spent the last year preparing it for that.
>
>> Why does it ever make sense for $SUBSYSTEM_NQN_0 with $PORT_DRIVER_FOO
>> to block operation of $SUBSYSTEM_NQN_1 with $PORT_DRIVER_BAR..?
>
> Because it keeps the code simple.  If you had actually participated
> on our development list you might have seen that until not too long
> ago we have very fine grainded locks here.  In the end Armen convinced
> me that it's easier to maintain if we don't bother with fine grained
> locking outside the fast path, especially as it significantly simplifies
> the discovery implementation.   If if it ever turns out to be an
> issue we can change it easily as the implementation is well encapsulated.

We did change that, and Nic is raising a valid point in terms of having
a global mutex around all the ports. If the requirement of nvme
subsystems and ports configuration is that it should happen fast enough
and scale to the numbers that Nic is referring to, we'll need to change
that back.

Having said that, I'm not sure this is a real hard requirement for RDMA
and FC in the mid-term, because from what I've seen, the workloads Nic
is referring to are more typical for iscsi/tcp where connections are
cheaper and you need more to saturate a high-speed interconnects, so
we'll probably see this when we have nvme over tcp working.

[toc] | [prev] | [next] | [standalone]

#1417442

On Wed, Jun 08, 2016 at 04:12:27PM +0300, Sagi Grimberg wrote:
>> Because it keeps the code simple.  If you had actually participated
>> on our development list you might have seen that until not too long
>> ago we have very fine grainded locks here.  In the end Armen convinced
>> me that it's easier to maintain if we don't bother with fine grained
>> locking outside the fast path, especially as it significantly simplifies
>> the discovery implementation.   If if it ever turns out to be an
>> issue we can change it easily as the implementation is well encapsulated.
>
> We did change that, and Nic is raising a valid point in terms of having
> a global mutex around all the ports. If the requirement of nvme
> subsystems and ports configuration is that it should happen fast enough
> and scale to the numbers that Nic is referring to, we'll need to change
> that back.
>
> Having said that, I'm not sure this is a real hard requirement for RDMA
> and FC in the mid-term, because from what I've seen, the workloads Nic
> is referring to are more typical for iscsi/tcp where connections are
> cheaper and you need more to saturate a high-speed interconnects, so
> we'll probably see this when we have nvme over tcp working.

I'm not really worried about connection establishment - that can be
changed to RCU locking really easily.  I'm a bit more worried about
the case where a driver would block long in ->add_port.  But let's
worry about that if an actual user comes up.  The last thing we need
in a new driver is lots of complexity for hypothetical use cases,
I'm much more interested in having the driver simple, testable and
actually tested than optimizing for something.

That is to say the priorities here are very different from Nic's goals
for the target code.

[toc] | [prev] | [next] | [standalone]

#1418061

On Wed, 2016-06-08 at 16:12 +0300, Sagi Grimberg wrote:
> >> *) Extensible to multiple types of backend drivers.
> >>
> >> nvme-target needs a way to absorb new backend drivers, that
> >> does not effect existing configfs group layout or attributes.
> >>
> >> Looking at the nvmet/configfs layout as-is, there are no multiple
> >> backend types defined, nor a way to control backend feature bits
> >> exposed to nvme namespaces at runtime.
> 
> Hey Nic,
> 
> As for different type of backends, I still don't see a big justification
> for adding the LIO backends pscsi (as it doesn't make sense),
> ramdisk (we have brd), or file (losetup).
> 

The configfs ABI should not dictate a single backend use-case.

In the target-core ecosystem today, there are just as many
people using FILEIO atop local file-systems as there are
using IBLOCK and submit_bio().

As mentioned, target_core_iblock.c has absorbed the io-cmd.c
improvements so existing scsi target drivers can benefit
too.  Plus, having interested folks focusing on a single set of
backends for FILEIO + IBLOCK means both scsi and nvme
target drivers benefit from further improvements.

As we've already got both, a target backend configfs ABI
and user ecosystem using /sys/kernel/config/target/core/, it's
a straight forward way to share common code, while still allowing
scsi and nvme to function using their own independent fabric
configfs ABI layouts.

Along with having common code and existing configfs
ABI, we also get a proper starting point for target-core
features that span across endpoints, and are defined for
both scsi and nvme.  PR APTPL immediately comes to mind.

Namely, for the ability of one backend device to interact
between both scsi target luns and nvme target namespaces,
as well as different backends across scsi and nvme exports.

> What kind of feature bits would you want to expose at runtime?

As for feature bits, basically everything currently or
in the future to be reported by ID_NS.  There is T10-PI, and another
example is copy-offload support, once the NVMe spec gets that far..

The main point is that we should be able to add new feature bits to
common code in target-core backend configfs ABI, without having to
change the individual scsi or nvme configfs ABIs.

> 
> > And that's very much intentional.  We have a very well working block
> > layer which we're going to use, no need to reivent it.  The block
> > layer supports NVMe pass through just fine in case we'll need it,
> > as I spent the last year preparing it for that.
> >
> >> Why does it ever make sense for $SUBSYSTEM_NQN_0 with $PORT_DRIVER_FOO
> >> to block operation of $SUBSYSTEM_NQN_1 with $PORT_DRIVER_BAR..?
> >
> > Because it keeps the code simple.  If you had actually participated
> > on our development list you might have seen that until not too long
> > ago we have very fine grainded locks here.  In the end Armen convinced
> > me that it's easier to maintain if we don't bother with fine grained
> > locking outside the fast path, especially as it significantly simplifies
> > the discovery implementation.   If if it ever turns out to be an
> > issue we can change it easily as the implementation is well encapsulated.
> 
> We did change that, and Nic is raising a valid point in terms of having
> a global mutex around all the ports. If the requirement of nvme
> subsystems and ports configuration is that it should happen fast enough
> and scale to the numbers that Nic is referring to, we'll need to change
> that back.
> 
> Having said that, I'm not sure this is a real hard requirement for RDMA
> and FC in the mid-term, because from what I've seen, the workloads Nic
> is referring to are more typical for iscsi/tcp where connections are
> cheaper and you need more to saturate a high-speed interconnects, so
> we'll probably see this when we have nvme over tcp working.

Yes.

Further, my objections to the proposed nvmet configfs ABI are:

  - Doesn't support multiple backend types.
  - Doesn't provide a way to control backend feature bits separate from 
    fabric layout.
  - Doesn't provide a starting point between target features that span
    both scsi and nvme.
  - Doesn't allow for concurrent parallel configfs create + delete 
    operations of subsystem NQNs across ports and host acls.
  - Global synchronization of nvmet_fabric_ops->add_port()

[toc] | [prev] | [next] | [standalone]

#1418342

On Wed, Jun 08, 2016 at 09:36:15PM -0700, Nicholas A. Bellinger wrote:
> The configfs ABI should not dictate a single backend use-case.

And it doesn't.  I actually had a file backend implemented to
benchmark it against the loopback driver.  It needed absolutely
zero new configfs interface.  And if we at some point want
different backends using different attributes we can trivially
add them using configfs_register_group.

> Along with having common code and existing configfs
> ABI, we also get a proper starting point for target-core
> features that span across endpoints, and are defined for
> both scsi and nvme.  PR APTPL immediately comes to mind.

PRs are a useful feature on the road map.  However we need a separate
pluggable backend anyway for distributed backends like RBD or Bart's
DLM implementation.  Also the current LIO PR implementation will need
a lot of work to be usable for NVMe while actually following the
spec in all it's details and to be power ѕafe.   The right way to
go here is a PR API that allows different backends, and the existing
LIO one might be one of them after it's got the needed attention.

[toc] | [prev] | [next] | [standalone]

#1418047

On Wed, 2016-06-08 at 14:19 +0200, Christoph Hellwig wrote:
> On Tue, Jun 07, 2016 at 10:21:41PM -0700, Nicholas A. Bellinger wrote:
> > *) Extensible to multiple types of backend drivers.
> > 
> > nvme-target needs a way to absorb new backend drivers, that
> > does not effect existing configfs group layout or attributes.
> > 
> > Looking at the nvmet/configfs layout as-is, there are no multiple
> > backend types defined, nor a way to control backend feature bits
> > exposed to nvme namespaces at runtime.
> 
> And that's very much intentional.  We have a very well working block
> layer which we're going to use, no need to reivent it.  The block
> layer supports NVMe pass through just fine in case we'll need it,
> as I spent the last year preparing it for that.
> 
> > Why does it ever make sense for $SUBSYSTEM_NQN_0 with $PORT_DRIVER_FOO
> > to block operation of $SUBSYSTEM_NQN_1 with $PORT_DRIVER_BAR..?
> 
> Because it keeps the code simple.  If you had actually participated
> on our development list you might have seen that until not too long
> ago we have very fine grainded locks here.  In the end Armen convinced
> me that it's easier to maintain if we don't bother with fine grained
> locking outside the fast path, especially as it significantly simplifies
> the discovery implementation.   If if it ever turns out to be an
> issue we can change it easily as the implementation is well encapsulated.

Well, it's rather obvious from the design limitations highlighted
earlier that using a configfs model for nvmet was an after-thought.

You'll recall a prototype of nvmet using target_core_fabric_configfs.c,
which allowed nvmet to work out-of-the-box with LIO userspace, that fit
directly into target_core_pr.c, and other existing target-core features
that scsi and nvme share.

Since our discussions at LSF, I've taken the point that it makes sense
for nvmet to have it's own configfs layout using common configfs
backends, which is reflected in nvmet/configfs-ng RFC from monday.

However, I completely disagree with you that scsi (specifically iscsi)
and nvme models are somehow incompatible from a target subsystem
configfs perspective.

They are not.

Point being, if you took the exact same three top level configfs groups
in the nvmet implementation  using subsystem configfs symlinks into
ports and hosts, and did the same for iscsi by renaming them to:

    /sys/kernel/config/iscsit/

    subsystems -> iqns
    ports -> network portals
    hosts -> acls

You would have the exact same type of scale limitations with iscsi that
have already highlighted.  There is nothing iscsi or nvme specific about
these limitations.

The point is that it's not a question of if this configfs model is
required for a nvme target design, it's a bad model for any configfs
consumer design to follow.

To repeat, any time there are global locks and sequential lookups
across multiple top level configfs groups, you're doing configfs
wrong.

The whole point of configfs is to allow vfs to reference count data
structures using parent/child relationships, and let configfs give you
the reference counting for free.

And getting that reference counting for free in configfs is what allows
existing target-core backends + endpoints to scale creation, operation
and deletion independent from each other.

Any type of medium or larger service provider and hosting environment
requires the ability of it's target mode storage control plane to
function independent of individual tenants.

The limitations as-is of the nvmet/configfs design makes it rather
useless for these environments, and any type of upstream commitment to a
target mode configfs based ABI needs to be able to, at least, scale to
what we've already got in target_core_fabric_configfs.

Otherwise, it will eventually be thrown out for something that both fits
the needs of today's requirements, and can grow into the future while
never breaking user-space ABI compatibility.

We got the /sys/kernel/config/target/$FABRIC/ layout right the first
time back in 2008, have *never* had to break ABI compat, and we are
still able scale to the requirements of today.

I'd like to hope we'd be able to achieve the same goals for nvmet in
2024.

[toc] | [prev] | [next] | [standalone]

#1416633

On 06/06/2016 11:23 PM, Nicholas A. Bellinger wrote:
> Hi HCH & Co,
>
> On Mon, 2016-06-06 at 23:22 +0200, Christoph Hellwig wrote:
>> This patch set adds a generic NVMe over Fabrics target. The
>> implementation conforms to the NVMe 1.2b specification (which
>> includes Fabrics) and provides the NVMe over Fabrics access
>> to Linux block devices.
>>
>
> Thanks for all of the development work by the fabric_linux_driver team
> (HCH, Sagi, Ming, James F., James S., and Dave M.) over the last year.
>
> Very excited to see this code get a public release now that NVMf
> specification is out.  Now that it's in the wild, it's a good
> opportunity to discuss some of the more interesting implementation
> details, beyond the new NVMf wire-protocol itself.

I'm sorry but I missed it, can you repeat the link to the NVMe spec(s)?

Thanks -- Andy

[toc] | [prev] | [next] | [standalone]

#1416643

On Tue, Jun 7, 2016 at 2:02 PM, Andy Grover <agrover@redhat.com> wrote:
> On 06/06/2016 11:23 PM, Nicholas A. Bellinger wrote:
>>
>> Hi HCH & Co,
>>
>> On Mon, 2016-06-06 at 23:22 +0200, Christoph Hellwig wrote:
>>>
>>> This patch set adds a generic NVMe over Fabrics target. The
>>> implementation conforms to the NVMe 1.2b specification (which
>>> includes Fabrics) and provides the NVMe over Fabrics access
>>> to Linux block devices.
>>>
>>
>> Thanks for all of the development work by the fabric_linux_driver team
>> (HCH, Sagi, Ming, James F., James S., and Dave M.) over the last year.
>>
>> Very excited to see this code get a public release now that NVMf
>> specification is out.  Now that it's in the wild, it's a good
>> opportunity to discuss some of the more interesting implementation
>> details, beyond the new NVMf wire-protocol itself.
>
>
> I'm sorry but I missed it, can you repeat the link to the NVMe spec(s)?

http://www.nvmexpress.org/wp-content/uploads/NVM_Express_1_2_1_Gold_20160603.pdf
http://www.nvmexpress.org/wp-content/uploads/NVMe_over_Fabrics_1_0_Gold_20160605.pdf

[toc] | [prev] | [next] | [standalone]

#1416447

Hi Bart,

> As you know with configfs it is not allowed to create directories in the 
> configfs hierarchy from inside the kernel.

Of course you can, and every user of configfs relies on being able
to create directories from the kernel using either the default_groups
list, or through the use of configfs_register_group().

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web