scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

PATCH Applied James Bottomley 4 participants 5 messages May 13, 2016 – May 20, 2016 SCSI

Share & tools

James Bottomley

author maintainer

2016-05-13 19:04 UTC #0 lore raw

PATCH 1/1 scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

When SCSI was written, all commands coming from the filesystem (REQ_TYPE_FS commands) had data. This meant that our signal for needing to complete the command was the number of bytes completed being equal to the number of bytes in the request. Unfortunately, with the advent of flush barriers, we can now get zero length REQ_TYPE_FS commands, which confuse this logic because they satisfy the condition every time. This means they never get retried even for retryable conditions, like UNIT ATTENTION because we complete them early assuming they're done. Fix this by special casing the early completion condition to recognise zero length commands with errors and let them drop through to the retry code.

Reported-by Sebastian Parschauer <[email protected]>

Signed-off-by James E.J. Bottomley <[email protected]>

---

drivers/scsi/scsi_lib.c +5 -2

index 8106515..f704d02 100644
@@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	/*
-	 * If we finished all bytes in the request we are done now.
+	 * special case: failed zero length commands always need to
+	 * drop down into the retry code. Otherwise, if we finished
+	 * all bytes in the request we are done now.
 	 */
-	if (!scsi_end_request(req, error, good_bytes, 0))
+	if (!(blk_rq_bytes(req) == 0 && error) &&
+	    !scsi_end_request(req, error, good_bytes, 0))
 		return;
 
 	/*

Johannes Thumshirn

2016-05-17 09:28 UTC #1 lore raw

Quoted 24 lines · James Bottomley

When SCSI was written, all commands coming from the filesystem (REQ_TYPE_FS commands) had data. This meant that our signal for needing to complete the command was the number of bytes completed being equal to the number of bytes in the request. Unfortunately, with the advent of flush barriers, we can now get zero length REQ_TYPE_FS commands, which confuse this logic because they satisfy the condition every time. This means they never get retried even for retryable conditions, like UNIT ATTENTION because we complete them early assuming they're done. Fix this by special casing the early completion condition to recognise zero length commands with errors and let them drop through to the retry code.
Reported-by Sebastian Parschauer <[email protected]>
Signed-off-by James E.J. Bottomley <[email protected]>
---
drivers/scsi/scsi_lib.c +0 -0
index 8106515..f704d02 100644
@@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 

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

Quoted 10 lines

drivers/scsi/scsi_lib.c +5 -2

 	/*
-	 * If we finished all bytes in the request we are done now.
+	 * special case: failed zero length commands always need to
+	 * drop down into the retry code. Otherwise, if we finished
+	 * all bytes in the request we are done now.
 	 */
-	if (!scsi_end_request(req, error, good_bytes, 0))
+	if (!(blk_rq_bytes(req) == 0 && error) &&
+	    !scsi_end_request(req, error, good_bytes, 0))
 		return;

Naive question, why aren't we checking for blk_rq_bytes(req) == 0 && error in scsi_end_request()?

Quoted 6 lines

drivers/scsi/scsi_lib.c +0 -1
 	/*
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 10 lines

Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jinpu Wang

2016-05-17 09:35 UTC #2 lore raw

Quoted 25 lines · Johannes Thumshirn

Quoted 24 lines · James Bottomley
When SCSI was written, all commands coming from the filesystem (REQ_TYPE_FS commands) had data. This meant that our signal for needing to complete the command was the number of bytes completed being equal to the number of bytes in the request. Unfortunately, with the advent of flush barriers, we can now get zero length REQ_TYPE_FS commands, which confuse this logic because they satisfy the condition every time. This means they never get retried even for retryable conditions, like UNIT ATTENTION because we complete them early assuming they're done. Fix this by special casing the early completion condition to recognise zero length commands with errors and let them drop through to the retry code.
Reported-by Sebastian Parschauer <[email protected]>
Signed-off-by James E.J. Bottomley <[email protected]>
---
drivers/scsi/scsi_lib.c +0 -0
index 8106515..f704d02 100644
@@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
      }
 

Quoted 15 lines

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

Quoted 10 lines

drivers/scsi/scsi_lib.c +5 -2

      /*
-      * If we finished all bytes in the request we are done now.
+      * special case: failed zero length commands always need to
+      * drop down into the retry code. Otherwise, if we finished
+      * all bytes in the request we are done now.
       */
-     if (!scsi_end_request(req, error, good_bytes, 0))
+     if (!(blk_rq_bytes(req) == 0 && error) &&
+         !scsi_end_request(req, error, good_bytes, 0))
              return;

Naive question, why aren't we checking for blk_rq_bytes(req) == 0 && error in scsi_end_request()?

Hi Johannes,

Hannes also suggested this way, but that lead to BUG see:
http://www.spinics.net/lists/linux-scsi/msg96908.html

Signature · 11 lines

Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jinpu Wang

Tested-by on v1 1/1

2016-05-17 09:30 UTC #3 lore raw

In reply to James Bottomley

Quoted 26 lines · James Bottomley

When SCSI was written, all commands coming from the filesystem (REQ_TYPE_FS commands) had data. This meant that our signal for needing to complete the command was the number of bytes completed being equal to the number of bytes in the request. Unfortunately, with the advent of flush barriers, we can now get zero length REQ_TYPE_FS commands, which confuse this logic because they satisfy the condition every time. This means they never get retried even for retryable conditions, like UNIT ATTENTION because we complete them early assuming they're done. Fix this by special casing the early completion condition to recognise zero length commands with errors and let them drop through to the retry code.
Reported-by Sebastian Parschauer <[email protected]>
Signed-off-by James E.J. Bottomley <[email protected]>
---
drivers/scsi/scsi_lib.c +0 -1
index 8106515..f704d02 100644
@@ -911,9 +911,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
        }
 
        /*
-        * If we finished all bytes in the request we are done now.

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

Quoted 10 lines

drivers/scsi/scsi_lib.c +5 -1

+        * special case: failed zero length commands always need to
+        * drop down into the retry code. Otherwise, if we finished
+        * all bytes in the request we are done now.
         */
-       if (!scsi_end_request(req, error, good_bytes, 0))
+       if (!(blk_rq_bytes(req) == 0 && error) &&
+           !scsi_end_request(req, error, good_bytes, 0))
                return;
 
        /*

Hi James,

Thanks for your new patch. I tested on my environment, works fine. You can add:

Tested-by Jack Wang <[email protected]>

Signature · 11 lines

Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen

Applied

2016-05-20 02:07 UTC #4 lore raw

In reply to James Bottomley

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"James" == James Bottomley <[email protected]> writes:

James> When SCSI was written, all commands coming from the filesystem James> (REQ_TYPE_FS commands) had data. This meant that our signal for James> needing to complete the command was the number of bytes completed James> being equal to the number of bytes in the request. James> Unfortunately, with the advent of flush barriers, we can now get James> zero length REQ_TYPE_FS commands, which confuse this logic James> because they satisfy the condition every time. This means they James> never get retried even for retryable conditions, like UNIT James> ATTENTION because we complete them early assuming they're done. James> Fix this by special casing the early completion condition to James> recognise zero length commands with errors and let them drop James> through to the retry code.

Applied to 4.7/scsi-fixes.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering