[PATCH v2 0/4] SRP initiator patches for kernel 3.17

[PATCH v2 0/4] SRP initiator patches for kernel 3.17

[Bart Van Assche]

This patch series consists of four patches of which three are bug fixes
and one is a performance optimization.

Changes compared with v1:
- Left out the fifth patch since there is no agreement about that
  patch.
- Added "unlikely(...)" around the tests for overflow / underflow flags
  in the SRP_CMD response as proposed by Dave Dillow.

The patches included in this series are:

0001-scsi_transport_srp-Fix-fast_io_fail_tmo-dev_loss_tmo.patch
0002-IB-srp-Fix-deadlock-between-host-removal-and-multipa.patch
0003-IB-srp-Fix-residual-handling.patch
0004-IB-srp-Use-P_Key-cache-for-P_Key-lookups.patch
--
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 v2 1/4] scsi_transport_srp: Fix fast_io_fail_tmo=dev_loss_tmo=off behavior

[Bart Van Assche]

If scsi_remove_host() is called while an rport is in the blocked
state then scsi_remove_host() will only finish if the rport is
unblocked from inside a timer function. Make sure that an rport
only enters the blocked state if a timer will be started that
will unblock it. This avoids that unloading the ib_srp kernel
module after having disconnected the initiator from the target
system results in a deadlock if both the fast_io_fail_tmo and
dev_loss_tmo parameters have been set to "off".

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>
Cc: James Bottomley <jbottomley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/scsi/scsi_transport_srp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 13e8983..a0c5bfd 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -473,7 +473,8 @@ static void __srp_start_tl_fail_timers(struct srp_rport *rport)
 	if (delay > 0)
 		queue_delayed_work(system_long_wq, &rport->reconnect_work,
 				   1UL * delay * HZ);
-	if (srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0) {
+	if ((fast_io_fail_tmo >= 0 || dev_loss_tmo >= 0) &&
+	    srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0) {
 		pr_debug("%s new state: %d\n", dev_name(&shost->shost_gendev),
 			 rport->state);
 		scsi_target_block(&shost->shost_gendev);
-- 
1.8.4.5

--
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 v2 2/4] IB/srp: Fix deadlock between host removal and multipathd

[Bart Van Assche]

If scsi_remove_host() is invoked after a SCSI device has been
blocked, if the fast_io_fail_tmo or dev_loss_tmo work gets
scheduled on the workqueue executing srp_remove_work() and if an
I/O request is scheduled after the SCSI device had been blocked
by e.g. multipathd then the following deadlock can occur:

kworker/6:1     D ffff880831f3c460     0   195      2 0x00000000
Call Trace:
 [<ffffffff814aafd9>] schedule+0x29/0x70
 [<ffffffff814aa0ef>] schedule_timeout+0x10f/0x2a0
 [<ffffffff8105af6f>] msleep+0x2f/0x40
 [<ffffffff8123b0ae>] __blk_drain_queue+0x4e/0x180
 [<ffffffff8123d2d5>] blk_cleanup_queue+0x225/0x230
 [<ffffffffa0010732>] __scsi_remove_device+0x62/0xe0 [scsi_mod]
 [<ffffffffa000ed2f>] scsi_forget_host+0x6f/0x80 [scsi_mod]
 [<ffffffffa0002eba>] scsi_remove_host+0x7a/0x130 [scsi_mod]
 [<ffffffffa07cf5c5>] srp_remove_work+0x95/0x180 [ib_srp]
 [<ffffffff8106d7aa>] process_one_work+0x1ea/0x6c0
 [<ffffffff8106dd9b>] worker_thread+0x11b/0x3a0
 [<ffffffff810758bd>] kthread+0xed/0x110
 [<ffffffff814b972c>] ret_from_fork+0x7c/0xb0
multipathd      D ffff880096acc460     0  5340      1 0x00000000
Call Trace:
 [<ffffffff814aafd9>] schedule+0x29/0x70
 [<ffffffff814aa0ef>] schedule_timeout+0x10f/0x2a0
 [<ffffffff814ab79b>] io_schedule_timeout+0x9b/0xf0
 [<ffffffff814abe1c>] wait_for_completion_io_timeout+0xdc/0x110
 [<ffffffff81244b9b>] blk_execute_rq+0x9b/0x100
 [<ffffffff8124f665>] sg_io+0x1a5/0x450
 [<ffffffff8124fd21>] scsi_cmd_ioctl+0x2a1/0x430
 [<ffffffff8124fef2>] scsi_cmd_blk_ioctl+0x42/0x50
 [<ffffffffa00ec97e>] sd_ioctl+0xbe/0x140 [sd_mod]
 [<ffffffff8124bd04>] blkdev_ioctl+0x234/0x840
 [<ffffffff811cb491>] block_ioctl+0x41/0x50
 [<ffffffff811a0df0>] do_vfs_ioctl+0x300/0x520
 [<ffffffff811a1051>] SyS_ioctl+0x41/0x80
 [<ffffffff814b9962>] tracesys+0xd0/0xd5

Fix this by scheduling removal work on another workqueue than the
transport layer timers.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 38 +++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8420bdf..8bcdde8 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -130,6 +130,7 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr);
 static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event);
 
 static struct scsi_transport_template *ib_srp_transport_template;
+static struct workqueue_struct *srp_remove_wq;
 
 static struct ib_client srp_client = {
 	.name   = "srp",
@@ -731,7 +732,7 @@ static bool srp_queue_remove_work(struct srp_target_port *target)
 	spin_unlock_irq(&target->lock);
 
 	if (changed)
-		queue_work(system_long_wq, &target->remove_work);
+		queue_work(srp_remove_wq, &target->remove_work);
 
 	return changed;
 }
@@ -3261,9 +3262,10 @@ static void srp_remove_one(struct ib_device *device)
 		spin_unlock(&host->target_lock);
 
 		/*
-		 * Wait for target port removal tasks.
+		 * Wait for tl_err and target port removal tasks.
 		 */
 		flush_workqueue(system_long_wq);
+		flush_workqueue(srp_remove_wq);
 
 		kfree(host);
 	}
@@ -3313,16 +3315,22 @@ static int __init srp_init_module(void)
 		indirect_sg_entries = cmd_sg_entries;
 	}
 
+	srp_remove_wq = create_workqueue("srp_remove");
+	if (IS_ERR(srp_remove_wq)) {
+		ret = PTR_ERR(srp_remove_wq);
+		goto out;
+	}
+
+	ret = -ENOMEM;
 	ib_srp_transport_template =
 		srp_attach_transport(&ib_srp_transport_functions);
 	if (!ib_srp_transport_template)
-		return -ENOMEM;
+		goto destroy_wq;
 
 	ret = class_register(&srp_class);
 	if (ret) {
 		pr_err("couldn't register class infiniband_srp\n");
-		srp_release_transport(ib_srp_transport_template);
-		return ret;
+		goto release_tr;
 	}
 
 	ib_sa_register_client(&srp_sa_client);
@@ -3330,13 +3338,22 @@ static int __init srp_init_module(void)
 	ret = ib_register_client(&srp_client);
 	if (ret) {
 		pr_err("couldn't register IB client\n");
-		srp_release_transport(ib_srp_transport_template);
-		ib_sa_unregister_client(&srp_sa_client);
-		class_unregister(&srp_class);
-		return ret;
+		goto unreg_sa;
 	}
 
-	return 0;
+out:
+	return ret;
+
+unreg_sa:
+	ib_sa_unregister_client(&srp_sa_client);
+	class_unregister(&srp_class);
+
+release_tr:
+	srp_release_transport(ib_srp_transport_template);
+
+destroy_wq:
+	destroy_workqueue(srp_remove_wq);
+	goto out;
 }
 
 static void __exit srp_cleanup_module(void)
@@ -3345,6 +3362,7 @@ static void __exit srp_cleanup_module(void)
 	ib_sa_unregister_client(&srp_sa_client);
 	class_unregister(&srp_class);
 	srp_release_transport(ib_srp_transport_template);
+	destroy_workqueue(srp_remove_wq);
 }
 
 module_init(srp_init_module);
-- 
1.8.4.5

--
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 v2 3/4] IB/srp: Fix residual handling

[Bart Van Assche]

>From Documentation/scsi/scsi_mid_low_api.txt: "resid - an LLD should
set this signed integer to the requested transfer length (i.e.
'request_bufflen') less the number of bytes that are actually
transferred." This means that resid > 0 in case of an underrun and
also that resid < 0 in case of an overrun. Modify the SRP initiator
code such that it matches this requirement.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8bcdde8..6894822 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1644,10 +1644,14 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 				     SCSI_SENSE_BUFFERSIZE));
 		}
 
-		if (rsp->flags & (SRP_RSP_FLAG_DOOVER | SRP_RSP_FLAG_DOUNDER))
-			scsi_set_resid(scmnd, be32_to_cpu(rsp->data_out_res_cnt));
-		else if (rsp->flags & (SRP_RSP_FLAG_DIOVER | SRP_RSP_FLAG_DIUNDER))
+		if (unlikely(rsp->flags & SRP_RSP_FLAG_DIUNDER))
 			scsi_set_resid(scmnd, be32_to_cpu(rsp->data_in_res_cnt));
+		else if (unlikely(rsp->flags & SRP_RSP_FLAG_DIOVER))
+			scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_in_res_cnt));
+		else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOUNDER))
+			scsi_set_resid(scmnd, be32_to_cpu(rsp->data_out_res_cnt));
+		else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOOVER))
+			scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_out_res_cnt));
 
 		srp_free_req(target, req, scmnd,
 			     be32_to_cpu(rsp->req_lim_delta));
-- 
1.8.4.5

--
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 v2 3/4] IB/srp: Fix residual handling

[David Dillow]

On Wed, 2014-07-09 at 15:57 +0200, Bart Van Assche wrote:
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1644,10 +1644,14 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
>  				     SCSI_SENSE_BUFFERSIZE));
>  		}
 
Above this block, there is a line 
	if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {

My thought was that you could wrap the code starting at that block and
ending after the additional scsi_set_resid() calls you added with an
outer test like so:

	if (unlikely(rsp->flags & ~SRP_RSP_FLAG_RSPVALID)) {
		if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {
			...

instead of adding unlikely() to each test of flags. Once inside the
block -- which should be rare -- we're on the slow path anyway.

Your call.

> -		if (rsp->flags & (SRP_RSP_FLAG_DOOVER | SRP_RSP_FLAG_DOUNDER))
> -			scsi_set_resid(scmnd, be32_to_cpu(rsp->data_out_res_cnt));
> -		else if (rsp->flags & (SRP_RSP_FLAG_DIOVER | SRP_RSP_FLAG_DIUNDER))
> +		if (unlikely(rsp->flags & SRP_RSP_FLAG_DIUNDER))
>  			scsi_set_resid(scmnd, be32_to_cpu(rsp->data_in_res_cnt));
> +		else if (unlikely(rsp->flags & SRP_RSP_FLAG_DIOVER))
> +			scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_in_res_cnt));
> +		else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOUNDER))
> +			scsi_set_resid(scmnd, be32_to_cpu(rsp->data_out_res_cnt));
> +		else if (unlikely(rsp->flags & SRP_RSP_FLAG_DOOVER))
> +			scsi_set_resid(scmnd, -be32_to_cpu(rsp->data_out_res_cnt));
>  
>  		srp_free_req(target, req, scmnd,
>  			     be32_to_cpu(rsp->req_lim_delta));


--
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 v2 4/4] IB/srp: Use P_Key cache for P_Key lookups

[Bart Van Assche]

This change slightly reduces the time needed to log in.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 6894822..41a5230 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -40,6 +40,7 @@
 #include <linux/parser.h>
 #include <linux/random.h>
 #include <linux/jiffies.h>
+#include <rdma/ib_cache.h>
 
 #include <linux/atomic.h>
 
@@ -260,10 +261,10 @@ static int srp_init_qp(struct srp_target_port *target,
 	if (!attr)
 		return -ENOMEM;
 
-	ret = ib_find_pkey(target->srp_host->srp_dev->dev,
-			   target->srp_host->port,
-			   be16_to_cpu(target->path.pkey),
-			   &attr->pkey_index);
+	ret = ib_find_cached_pkey(target->srp_host->srp_dev->dev,
+				  target->srp_host->port,
+				  be16_to_cpu(target->path.pkey),
+				  &attr->pkey_index);
 	if (ret)
 		goto out;
 
-- 
1.8.4.5

--
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