RE:[PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

RE:[PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Jack Wang]

> An SRP target is required to maintain a single connection between
> initiator and target. This means that if the 'add_target' attribute
> is used to create a second connection to a target that the first
> connection will be logged out and that the SCSI error handler will
> kick in. The SCSI error handler will cause the SRP initiator to
> reconnect, which will cause I/O over the second connection to fail.
> Avoid such ping-pong behavior by disabling relogins. Note: if
> reconnecting manually is necessary, that is possible by deleting
> and recreating an rport via sysfs.
> 
> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
> Cc: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>

Thanks Bart for refreshing this new patch set. I think you should add
Signed-off-by for Sebastian ?

Best regards,
Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

On 06/12/13 16:29, Jack Wang wrote:
>> An SRP target is required to maintain a single connection between
>> initiator and target. This means that if the 'add_target' attribute
>> is used to create a second connection to a target that the first
>> connection will be logged out and that the SCSI error handler will
>> kick in. The SCSI error handler will cause the SRP initiator to
>> reconnect, which will cause I/O over the second connection to fail.
>> Avoid such ping-pong behavior by disabling relogins. Note: if
>> reconnecting manually is necessary, that is possible by deleting
>> and recreating an rport via sysfs.
>>
>> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
>> Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
>> Cc: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
>
> Thanks Bart for refreshing this new patch set. I think you should add
> Signed-off-by for Sebastian ?

This patch differs slightly from what Sebastian had posted. But if 
Sebastian agrees with this version, I'll be happy to add his signed-off-by.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/14] IB SRP initiator patches for kernel 3.11

[Bart Van Assche]

The purpose of this InfiniBand SRP initiator patch series is as follows:
- Make the SRP initiator driver better suited for use in a H.A. setup.
   Speed up failover by reducing the IB RC retry count and by notifying
   multipathd faster about transport layer failures by adding
   fast_io_fail_tmo and dev_loss_tmo parameters.
- Make the SRP initiator better suited for use on NUMA systems by
   making the HCA completion vector configurable.

Some of the patches in this patch series were posted previously as "Make 
ib_srp better suited for H.A. purposes". The changes compared to version 
five of that patch series are:
- Left out patches that are already upstream.
- Made it possible to set dev_loss_tmo to "off". This is useful in a
   setup using initiator side mirroring to avoid that new /dev/sd* names
   are reassigned after a failover or cable pull and reinsert.
- Added kernel module parameters to ib_srp for configuring default
   values of the fast_io_fail_tmo and dev_loss_tmo parameters.
- Added a patch from Dotan Barak that fixes a kernel oops during rmmod
   triggered by resource allocation failure at module load time.
- Avoid duplicate connections by refusing relogins instead of dropping
   duplicate connections, as proposed by Sebastian Riemer.
- Added a patch from Sebastian Riemer for failing SCSI commands
   silently.
- Added a patch from Vu Pham to make the transport layer (IB RC) retry
   count configurable.
- Made HCA completion vector configurable.

Changes since v4:
- Added a patch for removing SCSI devices upon a port down event

Changes since v3:
- Restored the dev_loss_tmo and fast_io_fail_tmo sysfs attributes.
- Included a patch to fix an ib_srp crash that could be triggered by
   cable pulling.

Changes since v2:
- Addressed the v2 review comments.
- Dropped the patches that have already been merged.
- Dropped the patches for integration with multipathd.
- Dropped the micro-optimization of the IB completion handlers.

The individual patches in this series are as follows:
0001-IB-srp-Fix-remove_one-crash-due-to-resource-exhausti.patch
0002-IB-srp-Fix-race-between-srp_queuecommand-and-srp_cla.patch
0003-IB-srp-Avoid-that-srp_reset_host-is-skipped-after-a-.patch
0004-IB-srp-Skip-host-settle-delay.patch
0005-IB-srp-Maintain-a-single-connection-per-I_T-nexus.patch
0006-IB-srp-Keep-rport-as-long-as-the-IB-transport-layer.patch
0007-scsi_transport_srp-Add-transport-layer-error-handlin.patch
0008-IB-srp-Add-srp_terminate_io.patch
0009-IB-srp-Use-SRP-transport-layer-error-recovery.patch
0010-IB-srp-Start-timers-if-a-transport-layer-error-occur.patch
0011-IB-srp-Fail-SCSI-commands-silently.patch
0012-IB-srp-Make-HCA-completion-vector-configurable.patch
0013-IB-srp-Make-transport-layer-retry-count-configurable.patch
0014-IB-srp-Bump-driver-version-and-release-date.patch

[PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

An SRP target is required to maintain a single connection between
initiator and target. This means that if the 'add_target' attribute
is used to create a second connection to a target that the first
connection will be logged out and that the SCSI error handler will
kick in. The SCSI error handler will cause the SRP initiator to
reconnect, which will cause I/O over the second connection to fail.
Avoid such ping-pong behavior by disabling relogins. Note: if
reconnecting manually is necessary, that is possible by deleting
and recreating an rport via sysfs.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index be12780..1a73b24 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -556,6 +556,36 @@ static void srp_rport_delete(struct srp_rport *rport)
 	srp_queue_remove_work(target);
 }
 
+/**
+ * srp_conn_unique() - check whether the connection to a target is unique
+ */
+static bool srp_conn_unique(struct srp_host *host,
+			    struct srp_target_port *target)
+{
+	struct srp_target_port *t;
+	bool ret = false;
+
+	if (target->state == SRP_TARGET_REMOVED)
+		goto out;
+
+	ret = true;
+
+	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);
+
+out:
+	return ret;
+}
+
 static int srp_connect_target(struct srp_target_port *target)
 {
 	int retries = 3;
@@ -2261,6 +2291,14 @@ static ssize_t srp_create_target(struct device *dev,
 	if (ret)
 		goto err;
 
+	if (!srp_conn_unique(target->srp_host, target)) {
+		shost_printk(KERN_INFO, target->scsi_host,
+			     PFX "Already connected to target port %.*s\n",
+			     (int)count, buf);
+		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");
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Sebastian Riemer]

Hi Bart,

thanks for picking up the idea not to use this 'add_target' file for
manual reconnects. I have only small remarks but basically you've got my
Acked-by and Tested-by.

Please find the remarks in-line.

Cheers,
Sebastian

On 12.06.2013 15:25, Bart Van Assche wrote:
> An SRP target is required to maintain a single connection between
> initiator and target. This means that if the 'add_target' attribute
> is used to create a second connection to a target that the first
> connection will be logged out and that the SCSI error handler will
> kick in. The SCSI error handler will cause the SRP initiator to
> reconnect, which will cause I/O over the second connection to fail.
> Avoid such ping-pong behavior by disabling relogins. Note: if
> reconnecting manually is necessary, that is possible by deleting
> and recreating an rport via sysfs.
> 
> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
> Cc: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c |   38 +++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index be12780..1a73b24 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -556,6 +556,36 @@ static void srp_rport_delete(struct srp_rport *rport)
>  	srp_queue_remove_work(target);
>  }
>  
> +/**
> + * srp_conn_unique() - check whether the connection to a target is unique
> + */
> +static bool srp_conn_unique(struct srp_host *host,
> +			    struct srp_target_port *target)
> +{
> +	struct srp_target_port *t;
> +	bool ret = false;
> +
> +	if (target->state == SRP_TARGET_REMOVED)
> +		goto out;
> +
> +	ret = true;
> +
> +	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);
> +
> +out:
> +	return ret;
> +}
> +

You've only changed the style of this function. Functionality is still
the same. Fine for me.

But why do you put it that high in the source code?
Do you (still) need it for something else?

I would put it directly in front of srp_create_target() or even in front
of that option parsing stuff for correct bottom-up.

>  static int srp_connect_target(struct srp_target_port *target)
>  {
>  	int retries = 3;
> @@ -2261,6 +2291,14 @@ static ssize_t srp_create_target(struct device *dev,
>  	if (ret)
>  		goto err;
>  
> +	if (!srp_conn_unique(target->srp_host, target)) {
> +		shost_printk(KERN_INFO, target->scsi_host,
> +			     PFX "Already connected to target port %.*s\n",
> +			     (int)count, buf);
> +		ret = -EEXIST;
> +		goto err;
> +	}
> +

Yes, this looks good! Nice idea to print the connection string!
Would be even cooler without trailing '\n' from within 'buf' but that's
okay.

I was a little bit afraid of overflows here so I did security testing.
But srp_parse_options() already rejected my evil connection strings. :-)

I've tried things like this:
id_ext=0002c903004ed0b2,\
ioc_guid=0002c903004ed0b2,\
dgid=fe800000000000000002c903004ed0b4,\
pkey=ffff,service_id=0002c903004ed0b2,\
xxxxxxxxxxxxxxxxxxxxxxxxx... until 4096 chars

id_ext=0002c903004ed0b2,\
ioc_guid=0002c903004ed0b2,\
dgid=fe800000000000000002c903004ed0b4,\
pkey=ffff,service_id=0002c903004ed0b2,\
id_ext=0002c903004ed0b2,\
ioc_guid=0002c903004ed0b2,\
dgid=fe800000000000000002c903004ed0b4,\
pkey=ffff,service_id=0002c903004ed0b2,\
... until 4096 chars

This string looked kind of funny. Also in the kernel message it was a
little bit longer than usual but the parsing detected that I have too
many parameters. So everything is fine in terms of security.

>  	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");
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

On 06/13/13 15:57, Sebastian Riemer wrote:
> You've only changed the style of this function. Functionality is still
> the same. Fine for me.
> 
> But why do you put it that high in the source code?
> Do you (still) need it for something else?
> 
> I would put it directly in front of srp_create_target() or even in front
> of that option parsing stuff for correct bottom-up.

Good idea. Will move the definition of that function down.
 
>>   static int srp_connect_target(struct srp_target_port *target)
>>   {
>>   	int retries = 3;
>> @@ -2261,6 +2291,14 @@ static ssize_t srp_create_target(struct device *dev,
>>   	if (ret)
>>   		goto err;
>>   
>> +	if (!srp_conn_unique(target->srp_host, target)) {
>> +		shost_printk(KERN_INFO, target->scsi_host,
>> +			     PFX "Already connected to target port %.*s\n",
>> +			     (int)count, buf);
>> +		ret = -EEXIST;
>> +		goto err;
>> +	}
>> +
> 
> Yes, this looks good! Nice idea to print the connection string!
> Would be even cooler without trailing '\n' from within 'buf' but that's
> okay.
> 
> I was a little bit afraid of overflows here so I did security testing.
> But srp_parse_options() already rejected my evil connection strings. :-)
> 
> I've tried things like this:
> id_ext=0002c903004ed0b2,\
> ioc_guid=0002c903004ed0b2,\
> dgid=fe800000000000000002c903004ed0b4,\
> pkey=ffff,service_id=0002c903004ed0b2,\
> xxxxxxxxxxxxxxxxxxxxxxxxx... until 4096 chars
> 
> id_ext=0002c903004ed0b2,\
> ioc_guid=0002c903004ed0b2,\
> dgid=fe800000000000000002c903004ed0b4,\
> pkey=ffff,service_id=0002c903004ed0b2,\
> id_ext=0002c903004ed0b2,\
> ioc_guid=0002c903004ed0b2,\
> dgid=fe800000000000000002c903004ed0b4,\
> pkey=ffff,service_id=0002c903004ed0b2,\
> ... until 4096 chars
> 
> This string looked kind of funny. Also in the kernel message it was a
> little bit longer than usual but the parsing detected that I have too
> many parameters. So everything is fine in terms of security.

The "%.*s" should only copy the data provided by the user, even if it
is not '\0' terminated. Stripping the trailing newline is probably
possible with something like the (untested) code below (will only work
if there is only one newline in the input string and if it's at the
end):
		shost_printk(KERN_INFO, target->scsi_host,
			     PFX "Already connected to target port %.*s\n",
			     (int)count - (memchr(buf, '\n', count) ==
					   buf + count - 1), buf);

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Sebastian Riemer]

On 13.06.2013 17:07, Bart Van Assche wrote:
[...]
> The "%.*s" should only copy the data provided by the user, even if it
> is not '\0' terminated. Stripping the trailing newline is probably
> possible with something like the (untested) code below (will only work
> if there is only one newline in the input string and if it's at the
> end):
> 		shost_printk(KERN_INFO, target->scsi_host,
> 			     PFX "Already connected to target port %.*s\n",
> 			     (int)count - (memchr(buf, '\n', count) ==
> 					   buf + count - 1), buf);

I thought more like this existing message (as the input string by the
user is possibly long with a lot of configuration options):

shost_printk(KERN_DEBUG, target->scsi_host, PFX
	     "new target: id_ext %016llx ioc_guid %016llx pkey %04x "
	     "service_id %016llx dgid %pI6\n",
	(unsigned long long) be64_to_cpu(target->id_ext),
	(unsigned long long) be64_to_cpu(target->ioc_guid),
	be16_to_cpu(target->path.pkey),
	(unsigned long long) be64_to_cpu(target->service_id),
	target->path.dgid.raw);

But this thing takes a lot of code lines. Perhaps this string formatting
should be put into a macro/inline function then.

Cheers,
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Vu Pham]

Hello Bart,

> +/**
> + * srp_conn_unique() - check whether the connection to a target is unique
> + */
> +static bool srp_conn_unique(struct srp_host *host,
> +			    struct srp_target_port *target)
> +{
> +	struct srp_target_port *t;
> +	bool ret = false;
> +
> +	if (target->state == SRP_TARGET_REMOVED)
> +		goto out;
> +
> +	ret = true;
> +
> +	spin_lock(&host->target_lock);
> +	list_for_each_entry(t, &host->target_list, list) {
> +		if (t != target &&
> +		    target->id_ext == t->id_ext &&
>   

Targets may advertise/expose on different pkeys
You can have multiple connections  (or paths/scsi hosts) to same target 
with different pkeys.
We need extra check to detect the uniqueness:
                                             target->path.pkey == 
t->path.pkey &&

-vu
> +		    target->ioc_guid == t->ioc_guid &&
> +		    target->initiator_ext == t->initiator_ext) {
> +			ret = false;
> +			break;
> +		}
> +	}
> +	spin_unlock(&host->target_lock);
> +
> +out:
> +	return ret;
> +}
> +
>  static int srp_connect_target(struct srp_target_port *target)
>  {
>  	int retries = 3;
> @@ -2261,6 +2291,14 @@ static ssize_t srp_create_target(struct device *dev,
>  	if (ret)
>  		goto err;
>  
> +	if (!srp_conn_unique(target->srp_host, target)) {
> +		shost_printk(KERN_INFO, target->scsi_host,
> +			     PFX "Already connected to target port %.*s\n",
> +			     (int)count, buf);
> +		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");
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

On 06/13/13 19:50, Vu Pham wrote:
> Hello Bart,
>
>> +/**
>> + * srp_conn_unique() - check whether the connection to a target is
>> unique
>> + */
>> +static bool srp_conn_unique(struct srp_host *host,
>> +                struct srp_target_port *target)
>> +{
>> +    struct srp_target_port *t;
>> +    bool ret = false;
>> +
>> +    if (target->state == SRP_TARGET_REMOVED)
>> +        goto out;
>> +
>> +    ret = true;
>> +
>> +    spin_lock(&host->target_lock);
>> +    list_for_each_entry(t, &host->target_list, list) {
>> +        if (t != target &&
>> +            target->id_ext == t->id_ext &&
>
> Targets may advertise/expose on different pkeys
> You can have multiple connections  (or paths/scsi hosts) to same target
> with different pkeys.
> We need extra check to detect the uniqueness:
>                                              target->path.pkey ==
> t->path.pkey &&

Hello Vu,

Thanks for the feedback. This is something I have already thinking about 
myself. Unfortunately I have not found any requirements in the T10 SRP 
standard with regard to InfiniBand partitions. However, in that document 
there is a section about single RDMA channel operation. In that section 
it is explained that an SRP target must log out established sessions 
upon receipt of a new login request. What I'm not sure about is whether 
only sessions with the same P_Key must be logged out or all established 
sessions if a new login request is received. I assume the latter since 
otherwise that would mean that an SRP target would be required to 
maintain multiple sessions if it allows connections with more than one 
P_Key to a target port ? My concern about adding a pkey comparison in 
the function srp_conn_unique() is that if a target allows an initiator 
to choose which partition to use when logging in, that this could result 
in the undesired SRP initiator ping-pong effect this patch tries to avoid.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Vu Pham]

Bart Van Assche wrote:
> On 06/13/13 19:50, Vu Pham wrote:
>> Hello Bart,
>>
>>> +/**
>>> + * srp_conn_unique() - check whether the connection to a target is
>>> unique
>>> + */
>>> +static bool srp_conn_unique(struct srp_host *host,
>>> +                struct srp_target_port *target)
>>> +{
>>> +    struct srp_target_port *t;
>>> +    bool ret = false;
>>> +
>>> +    if (target->state == SRP_TARGET_REMOVED)
>>> +        goto out;
>>> +
>>> +    ret = true;
>>> +
>>> +    spin_lock(&host->target_lock);
>>> +    list_for_each_entry(t, &host->target_list, list) {
>>> +        if (t != target &&
>>> +            target->id_ext == t->id_ext &&
>>
>> Targets may advertise/expose on different pkeys
>> You can have multiple connections  (or paths/scsi hosts) to same target
>> with different pkeys.
>> We need extra check to detect the uniqueness:
>>                                              target->path.pkey ==
>> t->path.pkey &&
>
> Hello Vu,
>
> Thanks for the feedback. This is something I have already thinking 
> about myself. Unfortunately I have not found any requirements in the 
> T10 SRP standard with regard to InfiniBand partitions. However, in 
> that document there is a section about single RDMA channel operation. 
> In that section it is explained that an SRP target must log out 
> established sessions upon receipt of a new login request. What I'm not 
> sure about is whether only sessions with the same P_Key must be logged 
> out or all established sessions if a new login request is received. I 
> assume the latter since otherwise that would mean that an SRP target 
> would be required to maintain multiple sessions if it allows 
> connections with more than one P_Key to a target port ? My concern 
> about adding a pkey comparison in the function srp_conn_unique() is 
> that if a target allows an initiator to choose which partition to use 
> when logging in, that this could result in the undesired SRP initiator 
> ping-pong effect this patch tries to avoid.
>
> Bart.
>
Hello Bart,

Yes, you pointed out the unclear/undefined area.

If we stick to single RDMA channel per IT Nexus with unique tuple 
Initiator Port Identier - Target Port Indentifier then newly created 
connection with same tuple (I_port_id, T_port_id) but with different 
P_Key or different DGID is not unique.

Sticking to this rule by excluding P_Key and DGID  out of rdma channel 
indentity, your srp_conn_unique() checking is ok; however, some SRP 
target implementations may include DGID as part of rdma channel 
identifier. I'm not sure about different p_key part.

-vu
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Sebastian Riemer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 14.06.2013 01:27, Vu Pham wrote:
> Bart Van Assche wrote:
>> On 06/13/13 19:50, Vu Pham wrote:
>>> Hello Bart,
>>> 
>>>> +/** + * srp_conn_unique() - check whether the connection to
>>>> a target is unique + */ +static bool srp_conn_unique(struct
>>>> srp_host *host, +                struct srp_target_port
>>>> *target) +{ +    struct srp_target_port *t; +    bool ret =
>>>> false; + +    if (target->state == SRP_TARGET_REMOVED) +
>>>> goto out; + +    ret = true; + +
>>>> spin_lock(&host->target_lock); +    list_for_each_entry(t,
>>>> &host->target_list, list) { +        if (t != target && +
>>>> target->id_ext == t->id_ext &&
>>> 
>>> Targets may advertise/expose on different pkeys You can have
>>> multiple connections  (or paths/scsi hosts) to same target with
>>> different pkeys. We need extra check to detect the uniqueness: 
>>> target->path.pkey == t->path.pkey &&
>> 
>> Hello Vu,
>> 
>> Thanks for the feedback. This is something I have already
>> thinking about myself. Unfortunately I have not found any
>> requirements in the T10 SRP standard with regard to InfiniBand
>> partitions. However, in that document there is a section about
>> single RDMA channel operation. In that section it is explained
>> that an SRP target must log out established sessions upon receipt
>> of a new login request. What I'm not sure about is whether only
>> sessions with the same P_Key must be logged out or all
>> established sessions if a new login request is received. I assume
>> the latter since otherwise that would mean that an SRP target 
>> would be required to maintain multiple sessions if it allows 
>> connections with more than one P_Key to a target port ? My
>> concern about adding a pkey comparison in the function
>> srp_conn_unique() is that if a target allows an initiator to
>> choose which partition to use when logging in, that this could
>> result in the undesired SRP initiator ping-pong effect this patch
>> tries to avoid.
>> 
>> Bart.
>> 
> Hello Bart,
> 
> Yes, you pointed out the unclear/undefined area.
> 
> If we stick to single RDMA channel per IT Nexus with unique tuple 
> Initiator Port Identier - Target Port Indentifier then newly
> created connection with same tuple (I_port_id, T_port_id) but with
> different P_Key or different DGID is not unique.
> 
> Sticking to this rule by excluding P_Key and DGID  out of rdma
> channel indentity, your srp_conn_unique() checking is ok; however,
> some SRP target implementations may include DGID as part of rdma
> channel identifier. I'm not sure about different p_key part.
> 
> -vu

Hi Vu!

For what do you need the same target with multiple pkeys on the same
local SRP port?

Which other SRP targets exist?

I only know SCST, Solaris COMSTAR and that broken LIO stuff.
Does SCST still not support to set the pkey?

Why should we check the dgid?

Doesn't make any sense to me to connect both target ports to the same
local port. If you do so, the multipath-tools will crash. Note: This
function is called per local SRP port. Perhaps, a note should be added
to that function that it only has to be called per local SRP port.

Cheers,
Sebastian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJRuuSCAAoJEH4DRb7WXajZcFcH+gKsSs64Js/CUqMSyPeFPQ7u
7jKHvLr2wqHqSMIg5rEeZxZJpE9rL+wi8k5TMAMBrV+Povdwr8tWHgdq7mh5N1xO
V517YTgdzrwPIFy9e2uktxx4VYpsFGrV8iw3rdAzXRmcYa5U8feXhiD1VZyKjs4p
3//wvGAR0po7Pm0WgU9Q+h0arQos8CmeHkpoaNp/nNINXpXlTX21WVvHjwQrMFhC
Kr8zoCOTd0Sn+WoSs+CT/7Y4oTknukwR5vh6wfKgz2W74YkMKpD658QZozlafyK/
rwdajV19YYvi8YRTjUXuY5TN0qshYOGDxJDtNFkRGbx+IxIqFkGyyFCp0LPCfto=
=nlf2
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Vu Pham]

Hello Sebastian,

> On 14.06.2013 01:27, Vu Pham wrote:
>   
>> Bart Van Assche wrote:
>>     
>>> On 06/13/13 19:50, Vu Pham wrote:
>>>       
>>>> Hello Bart,
>>>>
>>>>         
>>>>> +/** + * srp_conn_unique() - check whether the connection to
>>>>> a target is unique + */ +static bool srp_conn_unique(struct
>>>>> srp_host *host, +                struct srp_target_port
>>>>> *target) +{ +    struct srp_target_port *t; +    bool ret =
>>>>> false; + +    if (target->state == SRP_TARGET_REMOVED) +
>>>>> goto out; + +    ret = true; + +
>>>>> spin_lock(&host->target_lock); +    list_for_each_entry(t,
>>>>> &host->target_list, list) { +        if (t != target && +
>>>>> target->id_ext == t->id_ext &&
>>>>>           
>>>> Targets may advertise/expose on different pkeys You can have
>>>> multiple connections  (or paths/scsi hosts) to same target with
>>>> different pkeys. We need extra check to detect the uniqueness: 
>>>> target->path.pkey == t->path.pkey &&
>>>>         
>>> Hello Vu,
>>>
>>> Thanks for the feedback. This is something I have already
>>> thinking about myself. Unfortunately I have not found any
>>> requirements in the T10 SRP standard with regard to InfiniBand
>>> partitions. However, in that document there is a section about
>>> single RDMA channel operation. In that section it is explained
>>> that an SRP target must log out established sessions upon receipt
>>> of a new login request. What I'm not sure about is whether only
>>> sessions with the same P_Key must be logged out or all
>>> established sessions if a new login request is received. I assume
>>> the latter since otherwise that would mean that an SRP target 
>>> would be required to maintain multiple sessions if it allows 
>>> connections with more than one P_Key to a target port ? My
>>> concern about adding a pkey comparison in the function
>>> srp_conn_unique() is that if a target allows an initiator to
>>> choose which partition to use when logging in, that this could
>>> result in the undesired SRP initiator ping-pong effect this patch
>>> tries to avoid.
>>>
>>> Bart.
>>>
>>>       
>> Hello Bart,
>>
>> Yes, you pointed out the unclear/undefined area.
>>
>> If we stick to single RDMA channel per IT Nexus with unique tuple 
>> Initiator Port Identier - Target Port Indentifier then newly
>> created connection with same tuple (I_port_id, T_port_id) but with
>> different P_Key or different DGID is not unique.
>>
>> Sticking to this rule by excluding P_Key and DGID  out of rdma
>> channel indentity, your srp_conn_unique() checking is ok; however,
>> some SRP target implementations may include DGID as part of rdma
>> channel identifier. I'm not sure about different p_key part.
>>
>> -vu
>>     
>
> Hi Vu!
>
> For what do you need the same target with multiple pkeys on the same
> local SRP port?
>   
There is no need, it's just a gray area that you can choose to have 
multiple connections to same target using different pkeys (same as dgid)
> Which other SRP targets exist?
>   

Netapp/LSI/Engenio, DDN, TexasMemorySystem Ramsan (IBM), Nimbus, Violin 
Memory, StreamScale
The last three may be derived from SCST base target.

> I only know SCST, Solaris COMSTAR and that broken LIO stuff.
> Does SCST still not support to set the pkey?
>
>   
Yes, I think so

> Why should we check the dgid?
>
>   
If you want to have multiple connections/qps to same target, but as I 
said above, it's a gray area.

> Doesn't make any sense to me to connect both target ports to the same
> local port. 
What if a target always expose single consistent and unique SRP port 
with tuple <id_ext, ioc_guid>, the ioc_guid part is not derived from any 
of its local HCA's GUID, then you can connect to this target thru 
different HCA ports (different dgid) as different paths to same target.

-vu
> If you do so, the multipath-tools will crash. Note: This
> function is called per local SRP port. Perhaps, a note should be added
> to that function that it only has to be called per local SRP port.
>
> Cheers,
> Sebastian
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQEcBAEBAgAGBQJRuuSCAAoJEH4DRb7WXajZcFcH+gKsSs64Js/CUqMSyPeFPQ7u
> 7jKHvLr2wqHqSMIg5rEeZxZJpE9rL+wi8k5TMAMBrV+Povdwr8tWHgdq7mh5N1xO
> V517YTgdzrwPIFy9e2uktxx4VYpsFGrV8iw3rdAzXRmcYa5U8feXhiD1VZyKjs4p
> 3//wvGAR0po7Pm0WgU9Q+h0arQos8CmeHkpoaNp/nNINXpXlTX21WVvHjwQrMFhC
> Kr8zoCOTd0Sn+WoSs+CT/7Y4oTknukwR5vh6wfKgz2W74YkMKpD658QZozlafyK/
> rwdajV19YYvi8YRTjUXuY5TN0qshYOGDxJDtNFkRGbx+IxIqFkGyyFCp0LPCfto=
> =nlf2
> -----END PGP SIGNATURE-----
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Sebastian Riemer]

On 14.06.2013 19:07, Vu Pham wrote:
[...]
>> For what do you need the same target with multiple pkeys on the same
>> local SRP port?
>>   
> There is no need, it's just a gray area that you can choose to have
> multiple connections to same target using different pkeys (same as dgid)
>> Which other SRP targets exist?
>>   
> 
> Netapp/LSI/Engenio, DDN, TexasMemorySystem Ramsan (IBM), Nimbus, Violin
> Memory, StreamScale
> The last three may be derived from SCST base target.
> 
>> I only know SCST, Solaris COMSTAR and that broken LIO stuff.
>> Does SCST still not support to set the pkey?
>>
>>   
> Yes, I think so
> 
>> Why should we check the dgid?
>>
>>   
> If you want to have multiple connections/qps to same target, but as I
> said above, it's a gray area.
> 
>> Doesn't make any sense to me to connect both target ports to the same
>> local port. 
> What if a target always expose single consistent and unique SRP port
> with tuple <id_ext, ioc_guid>, the ioc_guid part is not derived from any
> of its local HCA's GUID, then you can connect to this target thru
> different HCA ports (different dgid) as different paths to same target.
>
Do you have an example for a target which does it like this or a use
case where this makes sense?

I guess you're proposing here to use a driver global list of target
connections instead of handling this per local SRP port. This would
result in bigger changes which I wouldn't do without a good reason.

>> If you do so, the multipath-tools will crash. Note: This
>> function is called per local SRP port. Perhaps, a note should be added
>> to that function that it only has to be called per local SRP port.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[David Dillow]

On Wed, 2013-06-12 at 15:25 +0200, Bart Van Assche wrote:
> An SRP target is required to maintain a single connection between
> initiator and target. This means that if the 'add_target' attribute
> is used to create a second connection to a target that the first
> connection will be logged out and that the SCSI error handler will
> kick in. The SCSI error handler will cause the SRP initiator to
> reconnect, which will cause I/O over the second connection to fail.
> Avoid such ping-pong behavior by disabling relogins. Note: if
> reconnecting manually is necessary, that is possible by deleting
> and recreating an rport via sysfs.

Will wait for the second version of this as well.

Was an agreement reached on the pkey issue?
I also would rather we mimic the current messages rather than
regurgitate the user's login string back at them.

-- 
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/14] IB/srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

On 06/27/13 23:10, David Dillow wrote:
> On Wed, 2013-06-12 at 15:25 +0200, Bart Van Assche wrote:
>> An SRP target is required to maintain a single connection between
>> initiator and target. This means that if the 'add_target' attribute
>> is used to create a second connection to a target that the first
>> connection will be logged out and that the SCSI error handler will
>> kick in. The SCSI error handler will cause the SRP initiator to
>> reconnect, which will cause I/O over the second connection to fail.
>> Avoid such ping-pong behavior by disabling relogins. Note: if
>> reconnecting manually is necessary, that is possible by deleting
>> and recreating an rport via sysfs.
>
> Will wait for the second version of this as well.
>
> Was an agreement reached on the pkey issue?
> I also would rather we mimic the current messages rather than
> regurgitate the user's login string back at them.

Hello Dave,

My proposal is to leave out the pkey check.

I will update the error message printed when a login gets rejected.

Bart.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html