SRP initiator patches for kernel 3.15

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 <[email protected]>

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 <[email protected]>

Reported-by Masanari Iida <[email protected]>

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

Cc James Bottomley <[email protected]>

Cc Roland Dreier <[email protected]>

---
 drivers/scsi/scsi_transport_srp.c | 1 +
 include/scsi/scsi_transport_srp.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/scsi_transport_srp.c +1 -0

index d47ffc8..13e8983 100644
@@ -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

include/scsi/scsi_transport_srp.h +0 -0

index b11da5c..cdb05dd 100644

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

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.

Quoted 8 lines · 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.

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.

2014-02-20 14:50 GMT+04:00 Bart Van Assche <[email protected]>:

Quoted 7 lines

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?

Quoted 5 lines · David Dillow

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.

Quoted 13 lines · David Dillow

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)

Quoted 8 lines

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

index 7bd7f0d..124ab53 100644

Quoted 18 lines · Bart Van Assche

Quoted 17 lines · David Dillow

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 5 lines

drivers/scsi/scsi_lib.c +0 -0

index 7bd7f0d..124ab53 100644

drivers/scsi/scsi_lib.c void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) · line 864

Quoted 9 lines

drivers/scsi/scsi_lib.c +2 -1

@@ -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 */

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.

Quoted 5 lines · Bart Van Assche

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

Quoted 18 lines · David Dillow

Quoted 5 lines · Bart Van Assche

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;

Quoted 4 lines

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.

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

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.