[PATCH 0/3] Add duplicate_connect option for ctlr connect requests

[PATCH 0/3] Add duplicate_connect option for ctlr connect requests

[James Smart]

This patch adds a duplicate_connect option option to those supported by
fabric connection requests. When set to true (1), duplicate connections
are allowed. When set to false (0), duplicate connections are rejected.

The option is set in the fabrics layer and acted upon by each transport.

This patch set came about as of this prior patch review:
http://lists.infradead.org/pipermail/linux-nvme/2017-September/012783.html

James Smart (3):
  nvme: add duplicate_connect option
  nvme_fc: add support for duplicate_connect option
  RFC: nvme rdma: add support for duplicate_connect option

 drivers/nvme/host/fabrics.c | 19 +++++++++-
 drivers/nvme/host/fabrics.h |  2 ++
 drivers/nvme/host/fc.c      | 46 +++++++++++++++++++++++++
 drivers/nvme/host/rdma.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 150 insertions(+), 1 deletion(-)

-- 
2.13.1

[PATCH 1/3] nvme: add duplicate_connect option

[James Smart]

Add the option "duplicate_connect", which can be 1 for true, 0 for false.
Default is false.

When false, the transport should validate whether a new controller request
is targeted for the same host transport addressing and target transport
addressing as an existing controller. If so, the new controller request
should be rejected.

When true, the callee is explicitly requesting a duplicate controller
connection to be made and the new request should be attempted.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/fabrics.c | 19 ++++++++++++++++++-
 drivers/nvme/host/fabrics.h |  2 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 8bca36a46924..4177137a2ee6 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -548,6 +548,7 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_HOSTNQN,		"hostnqn=%s"		},
 	{ NVMF_OPT_HOST_TRADDR,		"host_traddr=%s"	},
 	{ NVMF_OPT_HOST_ID,		"hostid=%s"		},
+	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect=%d"	},
 	{ NVMF_OPT_ERR,			NULL			}
 };
 
@@ -566,6 +567,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	opts->nr_io_queues = num_online_cpus();
 	opts->reconnect_delay = NVMF_DEF_RECONNECT_DELAY;
 	opts->kato = NVME_DEFAULT_KATO;
+	opts->duplicate_connect = false;
 
 	options = o = kstrdup(buf, GFP_KERNEL);
 	if (!options)
@@ -742,6 +744,21 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 				goto out;
 			}
 			break;
+		case NVMF_OPT_DUP_CONNECT:
+			if (match_int(args, &token)) {
+				ret = -EINVAL;
+				goto out;
+			}
+			if (token == 0)
+				opts->duplicate_connect = false;
+			else if (token == 1)
+				opts->duplicate_connect = true;
+			else {
+				pr_err("Invalid duplicate_connect %d\n", token);
+				ret = -EINVAL;
+				goto out;
+			}
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);
@@ -823,7 +840,7 @@ EXPORT_SYMBOL_GPL(nvmf_free_options);
 #define NVMF_REQUIRED_OPTS	(NVMF_OPT_TRANSPORT | NVMF_OPT_NQN)
 #define NVMF_ALLOWED_OPTS	(NVMF_OPT_QUEUE_SIZE | NVMF_OPT_NR_IO_QUEUES | \
 				 NVMF_OPT_KATO | NVMF_OPT_HOSTNQN | \
-				 NVMF_OPT_HOST_ID)
+				 NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT)
 
 static struct nvme_ctrl *
 nvmf_create_ctrl(struct device *dev, const char *buf, size_t count)
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index bf33663218cd..a7590cc59ba2 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -57,6 +57,7 @@ enum {
 	NVMF_OPT_HOST_TRADDR	= 1 << 10,
 	NVMF_OPT_CTRL_LOSS_TMO	= 1 << 11,
 	NVMF_OPT_HOST_ID	= 1 << 12,
+	NVMF_OPT_DUP_CONNECT	= 1 << 13,
 };
 
 /**
@@ -96,6 +97,7 @@ struct nvmf_ctrl_options {
 	unsigned int		nr_io_queues;
 	unsigned int		reconnect_delay;
 	bool			discovery_nqn;
+	bool			duplicate_connect;
 	unsigned int		kato;
 	struct nvmf_host	*host;
 	int			max_reconnects;
-- 
2.13.1

[PATCH 1/3] nvme: add duplicate_connect option

[Christoph Hellwig]

> +	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect=%d"	},

This is a boolean parameter, so please drop the "=%d here".

> +		case NVMF_OPT_DUP_CONNECT:
> +			if (match_int(args, &token)) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			if (token == 0)
> +				opts->duplicate_connect = false;
> +			else if (token == 1)
> +				opts->duplicate_connect = true;
> +			else {
> +				pr_err("Invalid duplicate_connect %d\n", token);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			break;

And replace all this code by:

	case NVMF_OPT_DUP_CONNECT:
		opts->duplicate_connect = true;
		break;

That being said I'd be tempted to keep the existing behavior as
the default, and make noduplicates the option.

[PATCH 1/3] nvme: add duplicate_connect option

[James Smart]

On 10/19/2017 8:20 AM, Christoph Hellwig wrote:
>> +	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect=%d"	},
> 
> This is a boolean parameter, so please drop the "=%d here".
> 
>> +		case NVMF_OPT_DUP_CONNECT:
>> +			if (match_int(args, &token)) {
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +			if (token == 0)
>> +				opts->duplicate_connect = false;
>> +			else if (token == 1)
>> +				opts->duplicate_connect = true;
>> +			else {
>> +				pr_err("Invalid duplicate_connect %d\n", token);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +			break;
> 
> And replace all this code by:
> 
> 	case NVMF_OPT_DUP_CONNECT:
> 		opts->duplicate_connect = true;
> 		break;
> 
> That being said I'd be tempted to keep the existing behavior as
> the default, and make noduplicates the option.
> 

ok, so the mere existence of the parameter means it's true.  That's fine 
and I can repost.

I disagree with allowing duplicates to be the default.  Generically, I 
think it should be the exception. Let the admin that will create 
duplicates, thus is well aware of the ramifications and that it is a 
duplication, use the explicit syntax.  Note: currently patches only 
allow the option for "nvme connect", not "connect-all".

Selfishly, making duplicates allowed the defaults also impacts the 
scripts posted for FC, and widens the nvme cli syntax to connect-all.

-- james

[PATCH 2/3] nvme_fc: add support for duplicate_connect option

[James Smart]

Adds support for the duplicate_connect option. When set to true,
checks whether there's an existing controller via the same host port
and target port for the same host (hostnqn, hostid) to the same
subsystem. Fails the connection request if an existing controller.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/fc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 18e036c6833b..48c241620d2b 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2784,6 +2784,46 @@ static const struct blk_mq_ops nvme_fc_admin_mq_ops = {
 };
 
 
+static inline bool
+__nvme_fc_options_match(struct nvmf_ctrl_options *opts,
+			struct nvme_fc_ctrl *ctrl)
+{
+	if (strcmp(opts->subsysnqn, ctrl->ctrl.opts->subsysnqn) ||
+	    strcmp(opts->host->nqn, ctrl->ctrl.opts->host->nqn) ||
+	    memcmp(&opts->host->id, &ctrl->ctrl.opts->host->id,
+				sizeof(uuid_t)))
+		return false;
+
+	return true;
+}
+
+/*
+ * Fails a controller request if it matches an existing controller
+ * (association) with the same tuple:
+ * <Host NQN, Host ID, local FC port, remote FC port, SUBSYS NQN>
+ *
+ * The ports don't need to be compared as they are intrinsically
+ * already matched by the port pointers supplied.
+ */
+static bool
+nvme_fc_existing_controller(struct nvme_fc_rport *rport,
+		struct nvmf_ctrl_options *opts)
+{
+	struct nvme_fc_ctrl *ctrl;
+	unsigned long flags;
+	bool found = false;
+
+	spin_lock_irqsave(&rport->lock, flags);
+	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
+		found = __nvme_fc_options_match(opts, ctrl);
+		if (found)
+			break;
+	}
+	spin_unlock_irqrestore(&rport->lock, flags);
+
+	return found;
+}
+
 static struct nvme_ctrl *
 nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	struct nvme_fc_lport *lport, struct nvme_fc_rport *rport)
@@ -2798,6 +2838,12 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 		goto out_fail;
 	}
 
+	if (!opts->duplicate_connect &&
+	    nvme_fc_existing_controller(rport, opts)) {
+		ret = -EALREADY;
+		goto out_fail;
+	}
+
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
 	if (!ctrl) {
 		ret = -ENOMEM;
-- 
2.13.1

[PATCH 2/3] nvme_fc: add support for duplicate_connect option

[Christoph Hellwig]

> +static inline bool
> +__nvme_fc_options_match(struct nvmf_ctrl_options *opts,
> +			struct nvme_fc_ctrl *ctrl)
> +{
> +	if (strcmp(opts->subsysnqn, ctrl->ctrl.opts->subsysnqn) ||
> +	    strcmp(opts->host->nqn, ctrl->ctrl.opts->host->nqn) ||
> +	    memcmp(&opts->host->id, &ctrl->ctrl.opts->host->id,
> +				sizeof(uuid_t)))
> +		return false;

This just checks generic options and isn't FC specific.  As a properly
named helper it might make sense in common code.

[PATCH 2/3] nvme_fc: add support for duplicate_connect option

[James Smart]

On 10/19/2017 8:23 AM, Christoph Hellwig wrote:
>> +static inline bool
>> +__nvme_fc_options_match(struct nvmf_ctrl_options *opts,
>> +			struct nvme_fc_ctrl *ctrl)
>> +{
>> +	if (strcmp(opts->subsysnqn, ctrl->ctrl.opts->subsysnqn) ||
>> +	    strcmp(opts->host->nqn, ctrl->ctrl.opts->host->nqn) ||
>> +	    memcmp(&opts->host->id, &ctrl->ctrl.opts->host->id,
>> +				sizeof(uuid_t)))
>> +		return false;
> 
> This just checks generic options and isn't FC specific.  As a properly
> named helper it might make sense in common code.
> 

Agree, I can make a short helper

-- james

[PATCH/RFC 3/3] nvme rdma: add support for duplicate_connect option

[James Smart]

Adds support for the duplicate_connect option. When set to true,
checks whether there's an existing controller via the same target
address (traddr), target port (trsvcid), and if specified, host
address (host_traddr). Fails the connection request if there is
an existing controller.

Signed-off-by: James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/rdma.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 92a03ff5fb4d..b5f45d64cf73 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1847,6 +1847,85 @@ static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
 	.get_address		= nvmf_get_address,
 };
 
+static inline bool
+__nvme_rdma_options_match(struct nvme_rdma_ctrl *ctrl,
+	struct nvmf_ctrl_options *opts)
+{
+	char *stdport = __stringify(NVME_RDMA_IP_PORT);
+
+	if (strcmp(opts->subsysnqn, ctrl->ctrl.opts->subsysnqn) ||
+	    strcmp(opts->host->nqn, ctrl->ctrl.opts->host->nqn) ||
+	    memcmp(&opts->host->id, &ctrl->ctrl.opts->host->id,
+				sizeof(uuid_t)) ||
+	    strcmp(opts->traddr, ctrl->ctrl.opts->traddr))
+		return false;
+
+	if (opts->mask & NVMF_OPT_TRSVCID &&
+	    ctrl->ctrl.opts->mask & NVMF_OPT_TRSVCID) {
+		if (strcmp(opts->trsvcid, ctrl->ctrl.opts->trsvcid))
+			return false;
+	} else if (opts->mask & NVMF_OPT_TRSVCID) {
+		if (strcmp(opts->trsvcid, stdport))
+			return false;
+	} else if (ctrl->ctrl.opts->mask & NVMF_OPT_TRSVCID) {
+		if (strcmp(stdport, ctrl->ctrl.opts->trsvcid))
+			return false;
+	}
+	/* else, it's a match as both have stdport. Fall to next checks */
+
+	/*
+	 * checking the local address is rough. In most cases, one
+	 * is not specified and the host port is selected by the stack.
+	 *
+	 * Assume no match if:
+	 *  local address is specified and address is not the same
+	 *  local address is not specified but remote is, or vice versa
+	 *    (admin using specific host_traddr when it matters).
+	 */
+	if (opts->mask & NVMF_OPT_HOST_TRADDR &&
+	    ctrl->ctrl.opts->mask & NVMF_OPT_HOST_TRADDR) {
+		if (strcmp(opts->host_traddr, ctrl->ctrl.opts->host_traddr))
+			return false;
+	} else if (opts->mask & NVMF_OPT_HOST_TRADDR ||
+		   ctrl->ctrl.opts->mask & NVMF_OPT_HOST_TRADDR)
+		return false;
+	/*
+	 * if neither controller had an host port specified, assume it's
+	 * a match as everything else matched.
+	 */
+
+	return true;
+}
+
+/*
+ * Fails a connection request if it matches an existing controller
+ * (association) with the same tuple:
+ * <Host NQN, Host ID, local address, remote address, remote port, SUBSYS NQN>
+ *
+ * if local address is not specified in the request, it will match an
+ * existing controller with all the other parameters the same and no
+ * local port address specified as well.
+ *
+ * The ports don't need to be compared as they are intrinsically
+ * already matched by the port pointers supplied.
+ */
+static bool
+nvme_rdma_existing_controller(struct nvmf_ctrl_options *opts)
+{
+	struct nvme_rdma_ctrl *ctrl;
+	bool found = false;
+
+	mutex_lock(&nvme_rdma_ctrl_mutex);
+	list_for_each_entry(ctrl, &nvme_rdma_ctrl_list, list) {
+		found = __nvme_rdma_options_match(ctrl, opts);
+		if (found)
+			break;
+	}
+	mutex_unlock(&nvme_rdma_ctrl_mutex);
+
+	return found;
+}
+
 static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		struct nvmf_ctrl_options *opts)
 {
@@ -1883,6 +1962,11 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		}
 	}
 
+	if (!opts->duplicate_connect && nvme_rdma_existing_controller(opts)) {
+		ret = -EALREADY;
+		goto out_free_ctrl;
+	}
+
 	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_rdma_ctrl_ops,
 				0 /* no quirks, we're perfect! */);
 	if (ret)
-- 
2.13.1