IB/srp: Maintain a single connection per I_T nexus

Commit Message

Sebastian Riemer June 12, 2013, 11:29 a.m. UTC

The sysfs attribute 'add_target' may not be used for multiple logins to
the same target. If doing so with multipathing, this crashes the
multipath-tools. Furthermore, we want to prevent the possibility of data
corruption here. If manual reconnect is necessary, then the user can use
the 'delete' sysfs attribute of the remote port before connecting.

Note: The function srp_conn_unique() has been taken from Bart Van Assche.

Cc: Bart Van Assche <[e-mail]>
Cc: David Dillow <[e-mail]>
Cc: Vu Pham <[e-mail]>
Cc: Sagi Grimberg <[e-mail]>
Cc: Oren Duer <[e-mail]>
Cc: Or Gerlitz <[e-mail]>
Signed-off-by: Sebastian Riemer <[e-mail]>
Reviewed-by: Christoph Hellwig <[e-mail]>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Sebastian Riemer June 12, 2013, 11:51 a.m. UTC | #1

Hi all,

as proposed by Or, let's discuss this on the mailing list.

This is a fundamental change required for everything related to
multipathing. It influences automatic reconnect patches which will
follow. So let's agree on the right solution here first before looking
at other patches.

In my opinion the 'add_target' sysfs attribute shouldn't be used for any
manual reconnect as well. This is why my patch rejects the double login
attempt instead of reconnecting an existing connection.
This can help to find scripting issues and things like this. We can't
expect that all users are using the srp-tools.

Please compare with Bart's version and let's discuss this here.
https://github.com/bvanassche/ib_srp-backport/commit/7d8774ff58d489858b1c046b2bf01b4e84e8dd9b

Cheers,
Sebastian


On 12.06.2013 13:29, Sebastian Riemer wrote:
> The sysfs attribute 'add_target' may not be used for multiple logins to
> the same target. If doing so with multipathing, this crashes the
> multipath-tools. Furthermore, we want to prevent the possibility of data
> corruption here. If manual reconnect is necessary, then the user can use
> the 'delete' sysfs attribute of the remote port before connecting.
> 
> Note: The function srp_conn_unique() has been taken from Bart Van Assche.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sebastian Riemer June 13, 2013, 2 p.m. UTC | #2

Bart's version also has the printing of the connection string if the
double login fails.

So forget about this version here.

On 12.06.2013 13:51, Sebastian Riemer wrote:
> Hi all,
> 
> as proposed by Or, let's discuss this on the mailing list.
> 
> This is a fundamental change required for everything related to
> multipathing. It influences automatic reconnect patches which will
> follow. So let's agree on the right solution here first before looking
> at other patches.
> 
> In my opinion the 'add_target' sysfs attribute shouldn't be used for any
> manual reconnect as well. This is why my patch rejects the double login
> attempt instead of reconnecting an existing connection.
> This can help to find scripting issues and things like this. We can't
> expect that all users are using the srp-tools.
> 
> Please compare with Bart's version and let's discuss this here.
> https://github.com/bvanassche/ib_srp-backport/commit/7d8774ff58d489858b1c046b2bf01b4e84e8dd9b
> 
> Cheers,
> Sebastian
> 
> 
> On 12.06.2013 13:29, Sebastian Riemer wrote:
>> The sysfs attribute 'add_target' may not be used for multiple logins to
>> the same target. If doing so with multipathing, this crashes the
>> multipath-tools. Furthermore, we want to prevent the possibility of data
>> corruption here. If manual reconnect is necessary, then the user can use
>> the 'delete' sysfs attribute of the remote port before connecting.
>>
>> Note: The function srp_conn_unique() has been taken from Bart Van Assche.
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 7ccf328..d14cc4b 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -2219,6 +2219,36 @@  out:
 	return ret;
 }
 
+/**
+ * srp_conn_unique() - Check whether the connection to a target is unique.
+ *
+ * Consider targets in state SRP_TARGET_REMOVED as not unique because these
+ * may already have been removed from the target list.
+ */
+static bool srp_conn_unique(struct srp_host *host,
+			    struct srp_target_port *target)
+{
+	struct srp_target_port *t;
+	bool ret = true;
+
+	if (target->state == SRP_TARGET_REMOVED)
+		return false;
+
+	spin_lock(&host->target_lock);
+	list_for_each_entry(t, &host->target_list, list) {
+		if (t != target &&
+		    target->id_ext == t->id_ext &&
+		    target->ioc_guid == t->ioc_guid &&
+		    target->initiator_ext == t->initiator_ext) {
+			ret = false;
+			break;
+		}
+	}
+	spin_unlock(&host->target_lock);
+
+	return ret;
+}
+
 static ssize_t srp_create_target(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
@@ -2257,6 +2287,14 @@  static ssize_t srp_create_target(struct device *dev,
 	if (ret)
 		goto err;
 
+	/* Don't allow multiple logins to the same target port */
+	if (!srp_conn_unique(target->srp_host, target)) {
+		shost_printk(KERN_INFO, target->scsi_host,
+			     PFX "SRP target port is already connected\n");
+		ret = -EEXIST;
+		goto err;
+	}
+
 	if (!host->srp_dev->fmr_pool && !target->allow_ext_sg &&
 				target->cmd_sg_cnt < target->sg_tablesize) {
 		pr_warn("No FMR pool and no external indirect descriptors, limiting sg_tablesize to cmd_sg_cnt\n");