[PATCH 0/6] SRP initiator patches for kernel 3.15

[PATCH 0/6] SRP initiator patches for kernel 3.15

[Bart Van Assche]

This patch series includes the following six patches:
0001-scsi_transport_srp-Fix-two-kernel-doc-warnings.patch
0002-IB-srp-Add-more-logging.patch
0003-IB-srp-Fail-SCSI-commands-silently.patch
0004-IB-srp-Avoid-duplicate-connections.patch
0005-IB-srp-Make-writing-into-the-add_target-sysfs-attrib.patch
0006-IB-srp-Avoid-that-writing-into-add_target-hangs-due-.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 1/6] scsi_transport_srp: Fix two kernel-doc warnings

[Bart Van Assche]

This patch fixes the following two kernel-doc warnings:

Warning(drivers/scsi/scsi_transport_srp.c:819): No description found for parameter 'rport'
Warning(include/scsi/scsi_transport_srp.h:75): Excess struct/union/enum/typedef member 'deleted' description in 'srp_rport'

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Reported-by: Masanari Iida <standby24x7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: James Bottomley <jbottomley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/scsi/scsi_transport_srp.c | 1 +
 include/scsi/scsi_transport_srp.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index d47ffc8..13e8983 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -810,6 +810,7 @@ EXPORT_SYMBOL_GPL(srp_remove_host);
 
 /**
  * srp_stop_rport_timers - stop the transport layer recovery timers
+ * @rport: SRP remote port for which to stop the timers.
  *
  * Must be called after srp_remove_host() and scsi_remove_host(). The caller
  * must hold a reference on the rport (rport->dev) and on the SCSI host
diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h
index b11da5c..cdb05dd 100644
--- a/include/scsi/scsi_transport_srp.h
+++ b/include/scsi/scsi_transport_srp.h
@@ -41,7 +41,6 @@ enum srp_rport_state {
  * @mutex:             Protects against concurrent rport reconnect /
  *                     fast_io_fail / dev_loss_tmo activity.
  * @state:             rport state.
- * @deleted:           Whether or not srp_rport_del() has already been invoked.
  * @reconnect_delay:   Reconnect delay in seconds.
  * @failed_reconnects: Number of failed reconnect attempts.
  * @reconnect_work:    Work structure used for scheduling reconnect attempts.
-- 
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 1/6] scsi_transport_srp: Fix two kernel-doc warnings

[Sebastian Riemer]

On 20.02.2014 11:51, Bart Van Assche wrote:
> This patch fixes the following two kernel-doc warnings:
> 
> Warning(drivers/scsi/scsi_transport_srp.c:819): No description found for parameter 'rport'
> Warning(include/scsi/scsi_transport_srp.h:75): Excess struct/union/enum/typedef member 'deleted' description in 'srp_rport'
> 
> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Reported-by: Masanari Iida <standby24x7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> Cc: James Bottomley <jbottomley-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/scsi/scsi_transport_srp.c | 1 +
>  include/scsi/scsi_transport_srp.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
> index d47ffc8..13e8983 100644
> --- a/drivers/scsi/scsi_transport_srp.c
> +++ b/drivers/scsi/scsi_transport_srp.c
> @@ -810,6 +810,7 @@ EXPORT_SYMBOL_GPL(srp_remove_host);
>  
>  /**
>   * srp_stop_rport_timers - stop the transport layer recovery timers
> + * @rport: SRP remote port for which to stop the timers.
>   *
>   * Must be called after srp_remove_host() and scsi_remove_host(). The caller
>   * must hold a reference on the rport (rport->dev) and on the SCSI host
> diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h
> index b11da5c..cdb05dd 100644
> --- a/include/scsi/scsi_transport_srp.h
> +++ b/include/scsi/scsi_transport_srp.h
> @@ -41,7 +41,6 @@ enum srp_rport_state {
>   * @mutex:             Protects against concurrent rport reconnect /
>   *                     fast_io_fail / dev_loss_tmo activity.
>   * @state:             rport state.
> - * @deleted:           Whether or not srp_rport_del() has already been invoked.
>   * @reconnect_delay:   Reconnect delay in seconds.
>   * @failed_reconnects: Number of failed reconnect attempts.
>   * @reconnect_work:    Work structure used for scheduling reconnect attempts.
> 

This is trivial. Thanks!
Acked-by: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
--
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 3/6] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

Do not log SCSI command failures that are the result of a transport
layer failure, a SCSI abort or SCSI reset. This patch is a slightly
modified version of a patch posted by Sebastian Riemer in March 2013
(see also http://thread.gmane.org/gmane.linux.drivers.rdma/15020/).

This patch avoids that a system becomes unresponsive during failover
of a large number of LUNs when kernel messages are logged to a serial
console.

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

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index fba52d2..b3ca4d6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -827,6 +827,7 @@ static void srp_finish_req(struct srp_target_port *target,
 
 	if (scmnd) {
 		srp_free_req(target, req, scmnd, 0);
+		scmnd->request->cmd_flags |= REQ_QUIET;
 		scmnd->result = result;
 		scmnd->scsi_done(scmnd);
 	}
@@ -1517,6 +1518,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 
 	result = srp_chkready(target->rport);
 	if (unlikely(result)) {
+		scmnd->request->cmd_flags |= REQ_QUIET;
 		scmnd->result = result;
 		scmnd->scsi_done(scmnd);
 		goto unlock_rport;
@@ -2011,6 +2013,7 @@ static int srp_abort(struct scsi_cmnd *scmnd)
 	else
 		ret = FAILED;
 	srp_free_req(target, req, scmnd, 0);
+	scmnd->request->cmd_flags |= REQ_QUIET;
 	scmnd->result = DID_ABORT << 16;
 	scmnd->scsi_done(scmnd);
 
-- 
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 3/6] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

On 02/20/14 11:52, Bart Van Assche wrote:
> Do not log SCSI command failures that are the result of a transport
> layer failure, a SCSI abort or SCSI reset. This patch is a slightly
> modified version of a patch posted by Sebastian Riemer in March 2013
> (see also http://thread.gmane.org/gmane.linux.drivers.rdma/15020/).
> 
> This patch avoids that a system becomes unresponsive during failover
> of a large number of LUNs when kernel messages are logged to a serial
> console.
> 
> Signed-off-by: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>

Since this patch based on work of Sebastian, I should have mentioned:

From: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>

--
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 3/6] IB/srp: Fail SCSI commands silently

[David Dillow]

On Thu, 2014-02-20 at 11:52 +0100, Bart Van Assche wrote:
> Do not log SCSI command failures that are the result of a transport
> layer failure, a SCSI abort or SCSI reset. This patch is a slightly
> modified version of a patch posted by Sebastian Riemer in March 2013
> (see also http://thread.gmane.org/gmane.linux.drivers.rdma/15020/).
> 
> This patch avoids that a system becomes unresponsive during failover
> of a large number of LUNs when kernel messages are logged to a serial
> console.

While I can see the utility for slow consoles, this should probably be
done in the SCSI lib, with a separate flag for indicating it is a
failure due to transport issues. That would allow the functionality to
be used by multiple drivers and verbosity configured in one place,
rather than patched in driver-by-driver.

--
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 3/6] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

On 02/21/14 04:55, David Dillow wrote:
> On Thu, 2014-02-20 at 11:52 +0100, Bart Van Assche wrote:
>> Do not log SCSI command failures that are the result of a transport
>> layer failure, a SCSI abort or SCSI reset. This patch is a slightly
>> modified version of a patch posted by Sebastian Riemer in March 2013
>> (see also http://thread.gmane.org/gmane.linux.drivers.rdma/15020/).
>>
>> This patch avoids that a system becomes unresponsive during failover
>> of a large number of LUNs when kernel messages are logged to a serial
>> console.
> 
> While I can see the utility for slow consoles, this should probably be
> done in the SCSI lib, with a separate flag for indicating it is a
> failure due to transport issues. That would allow the functionality to
> be used by multiple drivers and verbosity configured in one place,
> rather than patched in driver-by-driver.

Hello Dave,

If it would be possible to move the code for setting the REQ_QUIET flag
into the SCSI lib that would be great. However, as far as I know only
the LLD knows which requests are in progress. A call to
blk_start_request() makes blk_dequeue_request() remove a request from
the queuelist in struct request_queue. I am not aware of any list in the
block layer nor in the SCSI mid-layer that keeps track of requests that
are in progress ?

Thanks,

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 3/6] IB/srp: Fail SCSI commands silently

[David Dillow]

On Fri, 2014-02-21 at 10:23 +0100, Bart Van Assche wrote:
> On 02/21/14 04:55, David Dillow wrote:
> > While I can see the utility for slow consoles, this should probably be
> > done in the SCSI lib, with a separate flag for indicating it is a
> > failure due to transport issues. That would allow the functionality to
> > be used by multiple drivers and verbosity configured in one place,
> > rather than patched in driver-by-driver.
> 
> Hello Dave,
> 
> If it would be possible to move the code for setting the REQ_QUIET flag
> into the SCSI lib that would be great. However, as far as I know only
> the LLD knows which requests are in progress. A call to
> blk_start_request() makes blk_dequeue_request() remove a request from
> the queuelist in struct request_queue. I am not aware of any list in the
> block layer nor in the SCSI mid-layer that keeps track of requests that
> are in progress ?

I didn't suggest that -- I'm saying add a common functionality to turn
on/off the message printing for commands that failed due to a dead
transport. If it is useful for SRP initiators, it is probably useful for
SAS and iSCSI imitators as well.

At a quick look, it seems you need to add a new value to the
rq_flag_bits enum in include/linux/blk_types.h, __REQ_FAILED_TRANSPORT,
somewhere before __REQ_NR_BITS. And a #define in the style of those
following it. Use that flag instead of REQ_QUIET in the patch we are
discussing, and change the code in scsi_lib.c to check

  ((req->cmd_flags & REQ_FAILED_TRANSPORT) && $user_wants_this_knob) || !(req->cmd_flags & REQ_QUIET)

before calling scsi_print_sense(), with appropriate naming, of course.
This gives you a central place to control it, and allows for consistency
between initiators.

I agree it is much easier to just fix it in SRP, especially given the
glacial rate of change for the SCSI mid-layer, but I really think a
central control and driver-by-driver enablement would be the best
approach. YMMV.

--
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 3/6] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

On 02/22/14 06:41, David Dillow wrote:
> I didn't suggest that -- I'm saying add a common functionality to turn
> on/off the message printing for commands that failed due to a dead
> transport. If it is useful for SRP initiators, it is probably useful for
> SAS and iSCSI imitators as well.
> 
> At a quick look, it seems you need to add a new value to the
> rq_flag_bits enum in include/linux/blk_types.h, __REQ_FAILED_TRANSPORT,
> somewhere before __REQ_NR_BITS. And a #define in the style of those
> following it. Use that flag instead of REQ_QUIET in the patch we are
> discussing, and change the code in scsi_lib.c to check
> 
>   ((req->cmd_flags & REQ_FAILED_TRANSPORT) && $user_wants_this_knob) || !(req->cmd_flags & REQ_QUIET)
> 
> before calling scsi_print_sense(), with appropriate naming, of course.
> This gives you a central place to control it, and allows for consistency
> between initiators.
> 
> I agree it is much easier to just fix it in SRP, especially given the
> glacial rate of change for the SCSI mid-layer, but I really think a
> central control and driver-by-driver enablement would be the best
> approach. YMMV.

Hello Dave,

That makes sense to me. Do you think the (untested) patch below could
be a valid alternative to the above proposal ?

---
 drivers/scsi/scsi_lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d..124ab53 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -857,7 +857,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		 */
 		if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
 			;
-		else if (!(req->cmd_flags & REQ_QUIET))
+		else if (cmd->device->sdev_state != SDEV_TRANSPORT_OFFLINE &&
+			 !(req->cmd_flags & REQ_QUIET))
 			scsi_print_sense("", cmd);
 		result = 0;
 		/* BLOCK_PC may have set error */
-- 
1.8.4.5


Thanks,

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 3/6] IB/srp: Fail SCSI commands silently

[David Dillow]

On Mon, 2014-02-24 at 20:58 +0100, Bart Van Assche wrote:
> On 02/22/14 06:41, David Dillow wrote:
> > I didn't suggest that -- I'm saying add a common functionality to turn
> > on/off the message printing for commands that failed due to a dead
> > transport. If it is useful for SRP initiators, it is probably useful for
> > SAS and iSCSI imitators as well.
> > 
> > At a quick look, it seems you need to add a new value to the
> > rq_flag_bits enum in include/linux/blk_types.h, __REQ_FAILED_TRANSPORT,
> > somewhere before __REQ_NR_BITS. And a #define in the style of those
> > following it. Use that flag instead of REQ_QUIET in the patch we are
> > discussing, and change the code in scsi_lib.c to check
> > 
> >   ((req->cmd_flags & REQ_FAILED_TRANSPORT) && $user_wants_this_knob) || !(req->cmd_flags & REQ_QUIET)
> > 
> > before calling scsi_print_sense(), with appropriate naming, of course.
> > This gives you a central place to control it, and allows for consistency
> > between initiators.
> > 
> > I agree it is much easier to just fix it in SRP, especially given the
> > glacial rate of change for the SCSI mid-layer, but I really think a
> > central control and driver-by-driver enablement would be the best
> > approach. YMMV.
> 
> Hello Dave,
> 
> That makes sense to me. Do you think the (untested) patch below could
> be a valid alternative to the above proposal ?

I'd still like to see a knob to let the user turn it off -- sometimes
you want to see those messages -- but it seems like a viable approach,
especially if it works when you test it :)

I'll leave to others about how important that knob is, though. If no one
else barks, then it should be OK.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7bd7f0d..124ab53 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -857,7 +857,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  		 */
>  		if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
>  			;
> -		else if (!(req->cmd_flags & REQ_QUIET))
> +		else if (cmd->device->sdev_state != SDEV_TRANSPORT_OFFLINE &&
> +			 !(req->cmd_flags & REQ_QUIET))
>  			scsi_print_sense("", cmd);
>  		result = 0;
>  		/* BLOCK_PC may have set error */


--
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 3/6] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

On 02/25/14 05:36, David Dillow wrote:
> On Mon, 2014-02-24 at 20:58 +0100, Bart Van Assche wrote:
>> On 02/22/14 06:41, David Dillow wrote:
>>> I didn't suggest that -- I'm saying add a common functionality to turn
>>> on/off the message printing for commands that failed due to a dead
>>> transport. If it is useful for SRP initiators, it is probably useful for
>>> SAS and iSCSI imitators as well.
>>>
>>> At a quick look, it seems you need to add a new value to the
>>> rq_flag_bits enum in include/linux/blk_types.h, __REQ_FAILED_TRANSPORT,
>>> somewhere before __REQ_NR_BITS. And a #define in the style of those
>>> following it. Use that flag instead of REQ_QUIET in the patch we are
>>> discussing, and change the code in scsi_lib.c to check
>>>
>>>   ((req->cmd_flags & REQ_FAILED_TRANSPORT) && $user_wants_this_knob) || !(req->cmd_flags & REQ_QUIET)
>>>
>>> before calling scsi_print_sense(), with appropriate naming, of course.
>>> This gives you a central place to control it, and allows for consistency
>>> between initiators.
>>>
>>> I agree it is much easier to just fix it in SRP, especially given the
>>> glacial rate of change for the SCSI mid-layer, but I really think a
>>> central control and driver-by-driver enablement would be the best
>>> approach. YMMV.
>>
>> That makes sense to me. Do you think the (untested) patch below could
>> be a valid alternative to the above proposal ?
> 
> I'd still like to see a knob to let the user turn it off -- sometimes
> you want to see those messages -- but it seems like a viable approach,
> especially if it works when you test it :)
> 
> I'll leave to others about how important that knob is, though. If no one
> else barks, then it should be OK.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 7bd7f0d..124ab53 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -857,7 +857,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>  		 */
>>  		if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
>>  			;
>> -		else if (!(req->cmd_flags & REQ_QUIET))
>> +		else if (cmd->device->sdev_state != SDEV_TRANSPORT_OFFLINE &&
>> +			 !(req->cmd_flags & REQ_QUIET))
>>  			scsi_print_sense("", cmd);
>>  		result = 0;
>>  		/* BLOCK_PC may have set error */

Hello Dave,

Testing learned me that the above patch suppresses the messages reported
by the SCSI mid-layer due to a transport failure but not the messages
reported by the block layer (e.g. "end_request: recoverable transport
error, dev sdc, sector 42514" -- see also blk_update_request() in
block/blk-core.c). Do you really think it is essential to introduce a
new flag in the block layer for the purpose of suppressing transport
layer error messages and to add support for that flag in the block core
and in the SCSI mid-layer ? To me it seems a lot simpler to use the
existing REQ_QUIET flag.

Thanks,

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 3/6] IB/srp: Fail SCSI commands silently

[David Dillow]

On Tue, 2014-02-25 at 11:33 +0100, Bart Van Assche wrote:
> Do you really think it is essential to introduce a
> new flag in the block layer for the purpose of suppressing transport
> layer error messages and to add support for that flag in the block
> core and in the SCSI mid-layer ? To me it seems a lot simpler to use
> the existing REQ_QUIET flag.

Yes, I think you want different semantics -- one is the requestor saying
"don't complain about this if it fails", the other is the LLD saying
"this failed due to transport failure, you may or may not want to report
it, under user control". But it doesn't necessarily have to be a new
block flag -- it may be as simple as adding a stanza in
scsi_io_completion() that looks like

if (cmd->device->sdev_state != SDEV_TRANSPORT_OFFLINE &&
    user_want_to_silence_transport_errors)
	req->cmd_flags |= REQ_QUIET;

if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
	;
else if (!(req->cmd_flags & REQ_QUIET))
	scsi_print_sense("", cmd);



--
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 3/6] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

On 02/26/14 07:32, David Dillow wrote:
> On Tue, 2014-02-25 at 11:33 +0100, Bart Van Assche wrote:
>> Do you really think it is essential to introduce a
>> new flag in the block layer for the purpose of suppressing transport
>> layer error messages and to add support for that flag in the block
>> core and in the SCSI mid-layer ? To me it seems a lot simpler to use
>> the existing REQ_QUIET flag.
> 
> Yes, I think you want different semantics -- one is the requestor saying
> "don't complain about this if it fails", the other is the LLD saying
> "this failed due to transport failure, you may or may not want to report
> it, under user control". But it doesn't necessarily have to be a new
> block flag -- it may be as simple as adding a stanza in
> scsi_io_completion() that looks like
> 
> if (cmd->device->sdev_state != SDEV_TRANSPORT_OFFLINE &&
>     user_want_to_silence_transport_errors)
> 	req->cmd_flags |= REQ_QUIET;
> 
> if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
> 	;
> else if (!(req->cmd_flags & REQ_QUIET))
> 	scsi_print_sense("", cmd);

But this approach doesn't suppress the error messages printed by the
block layer due to a transport layer failure. Additionally, the block
layer is neither aware of the SCSI transport layer state nor of the ASC
and ASCQ codes in the sense data. Hence my preference to keep patch 3/6
as it has been posted at the start of this thread.

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 3/6] IB/srp: Fail SCSI commands silently

[David Dillow]

On Wed, 2014-02-26 at 14:16 +0100, Bart Van Assche wrote:
> On 02/26/14 07:32, David Dillow wrote:
> > On Tue, 2014-02-25 at 11:33 +0100, Bart Van Assche wrote:
> >> Do you really think it is essential to introduce a
> >> new flag in the block layer for the purpose of suppressing transport
> >> layer error messages and to add support for that flag in the block
> >> core and in the SCSI mid-layer ? To me it seems a lot simpler to use
> >> the existing REQ_QUIET flag.
> > 
> > Yes, I think you want different semantics -- one is the requestor saying
> > "don't complain about this if it fails", the other is the LLD saying
> > "this failed due to transport failure, you may or may not want to report
> > it, under user control". But it doesn't necessarily have to be a new
> > block flag -- it may be as simple as adding a stanza in
> > scsi_io_completion() that looks like
> > 
> > if (cmd->device->sdev_state != SDEV_TRANSPORT_OFFLINE &&
> >     user_want_to_silence_transport_errors)
> > 	req->cmd_flags |= REQ_QUIET;
> > 
> > if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
> > 	;
> > else if (!(req->cmd_flags & REQ_QUIET))
> > 	scsi_print_sense("", cmd);
> 
> But this approach doesn't suppress the error messages printed by the
> block layer due to a transport layer failure.

I don't see where the block level is printing messages about the failed
requests -- but maybe I'm missing it. I see scsi_done() ->
blk_complete_request() -> blk_done_softirq() -> scsi_softirq_done() ->
scsi_finish_command() -> scsi_io_completion()

I'm not seeing where the errored block requests would be printed before
scsi_io_completion(), which is where we were talking about setting the
REQ_QUIET flag above.

What messages are you talking about -- even better if you can point to
where they come from -- but mainly which ones; I'm happy to track them
down myself. It's been a while since I've done a deep dive on this code,
so my apologies if it should be obvious to me...

> Additionally, the block layer is neither aware of the SCSI transport
> layer state nor of the ASC and ASCQ codes in the sense data.

Of course it isn't, and no one is suggesting it should.

> Hence my preference to keep patch 3/6 as it has been posted at the
> start of this thread.

I agree it is the most expedient approach to solve Sebastian's immediate
problem, but I'd rather the solution be generic to the SCSI mid-layer so
it will be useful for others beyond SRP.


--
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 3/6] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

On 02/27/14 06:48, David Dillow wrote:
> I don't see where the block level is printing messages about the failed
> requests -- but maybe I'm missing it. I see scsi_done() ->
> blk_complete_request() -> blk_done_softirq() -> scsi_softirq_done() ->
> scsi_finish_command() -> scsi_io_completion()
> 
> I'm not seeing where the errored block requests would be printed before
> scsi_io_completion(), which is where we were talking about setting the
> REQ_QUIET flag above.
> 
> What messages are you talking about -- even better if you can point to
> where they come from -- but mainly which ones; I'm happy to track them
> down myself. It's been a while since I've done a deep dive on this code,
> so my apologies if it should be obvious to me...

Hello Dave,

I think the call chain is as follows: scsi_io_completion() ->
scsi_end_request() -> blk_end_request() -> blk_end_bidi_request() ->
blk_update_bidi_request() -> blk_update_request(). That last function
prints an error message for failed requests if REQ_QUIET has not been
set. So this means that it's possible to set the REQ_QUIET flag from the
SCSI mid-layer before the block layer checks that flag.

> I agree it is the most expedient approach to solve Sebastian's immediate
> problem, but I'd rather the solution be generic to the SCSI mid-layer so
> it will be useful for others beyond SRP.

It would be great if this would be solved in a more generic way. But
please keep in mind that this is not just Sebastian's problem - other
users have reported this before.

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 4/6] IB/srp: Avoid duplicate connections

[Bart Van Assche]

The connection uniqueness check is performed before a new connection
is added to the target list. This patch protects both actions by a
mutex such that simultaneous writes from two different threads into the
"add_target" variable do not result in duplicate connections.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 12 +++++++++---
 drivers/infiniband/ulp/srp/ib_srp.h |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b3ca4d6..7d9413e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -2617,6 +2617,8 @@ static ssize_t srp_create_target(struct device *dev,
 	target->tl_retry_count	= 7;
 	target->queue_size	= SRP_DEFAULT_QUEUE_SIZE;
 
+	mutex_lock(&host->add_target_mutex);
+
 	ret = srp_parse_options(buf, target);
 	if (ret)
 		goto err;
@@ -2683,7 +2685,11 @@ static ssize_t srp_create_target(struct device *dev,
 		     be64_to_cpu(target->service_id),
 		     target->path.sgid.raw, target->path.dgid.raw);
 
-	return count;
+	ret = count;
+
+out:
+	mutex_unlock(&host->add_target_mutex);
+	return ret;
 
 err_disconnect:
 	srp_disconnect_target(target);
@@ -2699,8 +2705,7 @@ err_free_mem:
 
 err:
 	scsi_host_put(target_host);
-
-	return ret;
+	goto out;
 }
 
 static DEVICE_ATTR(add_target, S_IWUSR, NULL, srp_create_target);
@@ -2736,6 +2741,7 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
 	INIT_LIST_HEAD(&host->target_list);
 	spin_lock_init(&host->target_lock);
 	init_completion(&host->released);
+	mutex_init(&host->add_target_mutex);
 	host->srp_dev = device;
 	host->port = port;
 
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 5756810..aad27b7 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -105,6 +105,7 @@ struct srp_host {
 	spinlock_t		target_lock;
 	struct completion	released;
 	struct list_head	list;
+	struct mutex		add_target_mutex;
 };
 
 struct srp_request {
-- 
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 5/6] IB/srp: Make writing into the "add_target" sysfs attribute interruptible

[Bart Van Assche]

Avoid that stopping srp_daemon takes unusually long due to a cable
pull by making writing into the "add_target" sysfs attribute
interruptible.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 7d9413e..0d934b7 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -411,6 +411,8 @@ static void srp_path_rec_completion(int status,
 
 static int srp_lookup_path(struct srp_target_port *target)
 {
+	int ret;
+
 	target->path.numb_path = 1;
 
 	init_completion(&target->done);
@@ -431,7 +433,9 @@ static int srp_lookup_path(struct srp_target_port *target)
 	if (target->path_query_id < 0)
 		return target->path_query_id;
 
-	wait_for_completion(&target->done);
+	ret = wait_for_completion_interruptible(&target->done);
+	if (ret < 0)
+		return ret;
 
 	if (target->status < 0)
 		shost_printk(KERN_WARNING, target->scsi_host,
@@ -710,7 +714,9 @@ static int srp_connect_target(struct srp_target_port *target)
 		ret = srp_send_req(target);
 		if (ret)
 			return ret;
-		wait_for_completion(&target->done);
+		ret = wait_for_completion_interruptible(&target->done);
+		if (ret < 0)
+			return ret;
 
 		/*
 		 * The CM event handling code will set status to
-- 
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 6/6] IB/srp: Avoid that writing into "add_target" hangs due to a cable pull

[Bart Van Assche]

If a cable is pulled while srp_connect_target() is in progress
that can result in that function never to return. That makes the
process, e.g. srp_daemon, that invoked this function unkillable.
Avoid this by letting srp_connect_target() finish if the event
IB_CM_TIMEWAIT_EXIT is received. This patch fixes a hang with the
following call trace:
 [<ffffffff814eae85>] schedule_timeout+0x215/0x2e0
 [<ffffffff814eab03>] wait_for_common+0x123/0x180
 [<ffffffff814eac1d>] wait_for_completion+0x1d/0x20
 [<ffffffffa03b398c>] srp_connect_target+0x1dc/0x410 [ib_srp]
 [<ffffffffa03b5809>] srp_create_target+0xba9/0xe70 [ib_srp]
 [<ffffffff8133e590>] dev_attr_store+0x20/0x30
 [<ffffffff811eb8f5>] sysfs_write_file+0xe5/0x170
 [<ffffffff811767c8>] vfs_write+0xb8/0x1a0
 [<ffffffff811770c1>] sys_write+0x51/0x90
 [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 0d934b7..dcdaf49 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1873,6 +1873,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 	case IB_CM_TIMEWAIT_EXIT:
 		shost_printk(KERN_ERR, target->scsi_host,
 			     PFX "connection closed\n");
+		comp = 1;
 
 		target->status = 0;
 		break;
-- 
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 2/6] IB/srp: Add more logging

[Bart Van Assche]

Log sgid and dgid when reporting that a login has been rejected or when
a host has been added. This makes it easy to figure out which initiator
and target ports these messages apply to.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Riemer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Cc: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 529b6bc..fba52d2 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1804,8 +1804,10 @@ static void srp_cm_rej_handler(struct ib_cm_id *cm_id,
 				shost_printk(KERN_WARNING, shost,
 					     PFX "SRP_LOGIN_REJ: requested max_it_iu_len too large\n");
 			else
-				shost_printk(KERN_WARNING, shost,
-					    PFX "SRP LOGIN REJECTED, reason 0x%08x\n", reason);
+				shost_printk(KERN_WARNING, shost, PFX
+					     "SRP LOGIN from %pI6 to %pI6 REJECTED, reason 0x%08x\n",
+					     target->path.sgid.raw,
+					     target->orig_dgid, reason);
 		} else
 			shost_printk(KERN_WARNING, shost,
 				     "  REJ reason: IB_CM_REJ_CONSUMER_DEFINED,"
@@ -2651,15 +2653,6 @@ static ssize_t srp_create_target(struct device *dev,
 
 	ib_query_gid(ibdev, host->port, 0, &target->path.sgid);
 
-	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);
-
 	ret = srp_create_target_ib(target);
 	if (ret)
 		goto err_free_mem;
@@ -2679,6 +2672,14 @@ static ssize_t srp_create_target(struct device *dev,
 	if (ret)
 		goto err_disconnect;
 
+	shost_printk(KERN_DEBUG, target->scsi_host, PFX
+		     "new target: id_ext %016llx ioc_guid %016llx pkey %04x service_id %016llx sgid %pI6 dgid %pI6\n",
+		     be64_to_cpu(target->id_ext),
+		     be64_to_cpu(target->ioc_guid),
+		     be16_to_cpu(target->path.pkey),
+		     be64_to_cpu(target->service_id),
+		     target->path.sgid.raw, target->path.dgid.raw);
+
 	return count;
 
 err_disconnect:
-- 
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 0/6] SRP initiator patches for kernel 3.15

[Vasiliy Tolstov]

2014-02-20 14:50 GMT+04:00 Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>:
> This patch series includes the following six patches:
> 0001-scsi_transport_srp-Fix-two-kernel-doc-warnings.patch
> 0002-IB-srp-Add-more-logging.patch
> 0003-IB-srp-Fail-SCSI-commands-silently.patch
> 0004-IB-srp-Avoid-duplicate-connections.patch
> 0005-IB-srp-Make-writing-into-the-add_target-sysfs-attrib.patch
> 0006-IB-srp-Avoid-that-writing-into-add_target-hangs-due-.patch


Is that possible to get some of this into stable 3.10.x?

-- 
Vasiliy Tolstov,
e-mail: v.tolstov-+9FY0jupvH6HXe+LvDLADg@public.gmane.org
jabber: vase-+9FY0jupvH6HXe+LvDLADg@public.gmane.org
--
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 0/6] SRP initiator patches for kernel 3.15

[Bart Van Assche]

On 02/21/14 04:36, Vasiliy Tolstov wrote:
> 2014-02-20 14:50 GMT+04:00 Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>:
>> This patch series includes the following six patches:
>> 0001-scsi_transport_srp-Fix-two-kernel-doc-warnings.patch
>> 0002-IB-srp-Add-more-logging.patch
>> 0003-IB-srp-Fail-SCSI-commands-silently.patch
>> 0004-IB-srp-Avoid-duplicate-connections.patch
>> 0005-IB-srp-Make-writing-into-the-add_target-sysfs-attrib.patch
>> 0006-IB-srp-Avoid-that-writing-into-add_target-hangs-due-.patch
> 
> Is that possible to get some of this into stable 3.10.x?

What I understood from Documentation/stable_kernel_rules.txt is that
patches that fix a bug are acceptable for the stable kernel series but
functionality changes not. So I think patches 4, 5 and 6 would be good
candidates for the stable kernel series. Roland, do you want me to
resend this series with "stable" tags added where appropriate or are you
fine with adding this tag yourself to the last three patches ?

Thanks,

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