[PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[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 <s.parschauer@gmx.de>
Signed-off-by: James E.J. Bottomley <jejb@linux.vnet.ibm.com>

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..f704d02 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -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;
 
 	/*

Re: [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[Johannes Thumshirn]

On Fri, May 13, 2016 at 12:04:06PM -0700, James Bottomley wrote:
> 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 <s.parschauer@gmx.de>
> Signed-off-by: James E.J. Bottomley <jejb@linux.vnet.ibm.com>
> 
> ---
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..f704d02 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -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;


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

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

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[Jinpu Wang]

On Tue, May 17, 2016 at 11:28 AM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On Fri, May 13, 2016 at 12:04:06PM -0700, James Bottomley wrote:
>> 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 <s.parschauer@gmx.de>
>> Signed-off-by: James E.J. Bottomley <jejb@linux.vnet.ibm.com>
>>
>> ---
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 8106515..f704d02 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -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;
>
>
> 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


-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[Jinpu Wang]

On Fri, May 13, 2016 at 9:04 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> 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 <s.parschauer@gmx.de>
> Signed-off-by: James E.J. Bottomley <jejb@linux.vnet.ibm.com>
>
> ---
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..f704d02 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -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;
>
>         /*

Hi James,

Thanks for your new patch. I tested on my environment, works fine.
You can add:
Tested-by: Jack Wang <jinpu.wang@profitbricks.com>
-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> 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.

-- 
Martin K. Petersen	Oracle Linux Engineering