SCSI fix (a621bac3)missing in stable

SCSI fix (a621bac3)missing in stable

[Jinpu Wang]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

Hi all,

We found the SCSI fix is missing in any stable only in linux 4.7-rc2
[PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
 commands

commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.

So we backport & tested on linux-3.12 (as we're using this kernel).
Could you review if we did it right?


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

[-- Attachment #2: 0001-scsi_lib-correctly-retry-failed-zero-length-REQ_TYPE.patch --]
[-- Type: text/x-patch, Size: 2681 bytes --]

From 0cd7e60c45205b0b7347160a01943cb6e29515d5 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Fri, 13 May 2016 12:04:06 -0700
Subject: [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
 commands

commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.

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.

Cc: stable@vger.kernel.org
Reported-by: Sebastian Parschauer <s.parschauer@gmx.de>
Signed-off-by: James E.J. Bottomley <jejb@linux.vnet.ibm.com>
Tested-by: Jack Wang <jinpu.wang@profitbricks.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ ywang: backport from upstream 4.7 to fix scsi resize issue ]
Signed-off-by: Michael Wang <yun.wang@profitbricks.com>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/scsi/scsi_lib.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fd206c9..ad0f826 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -880,9 +880,30 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	 * are leftovers and there is some kind of error
 	 * (result != 0), retry the rest.
 	 */
-	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
+	if (!(blk_rq_bytes(req) == 0 && error) &&
+	    scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
 
+	/*
+	 * Failed zero length commands won't do scsi_end_request()
+	 * which check no retrys and requeue cases, do them here.
+	 *
+	 * For other cases scsi_end_request() will take care and
+	 * we won't reach here.
+	 */
+	if (error && scsi_noretry_cmd(cmd)) {
+		blk_end_request_all(req, error);
+		__scsi_release_buffers(cmd, 0);
+		scsi_next_command(cmd);
+		return;
+	}
+
+	if (result == 0) {
+		scsi_release_buffers(cmd);
+		scsi_requeue_command(q, cmd);
+		return;
+	}
+
 	error = __scsi_error_from_host_byte(cmd, result);
 
 	if (host_byte(result) == DID_RESET) {
-- 
1.9.1

Re: SCSI fix (a621bac3)missing in stable

[Greg KH]

On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
> Hi all,
> 
> We found the SCSI fix is missing in any stable only in linux 4.7-rc2
> [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
>  commands
> 
> commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.

That is because it only hit a public release (4.7-rc2) on Sunday, please
give us a chance to backport it to the other stable trees, it's in the
queue.

> So we backport & tested on linux-3.12 (as we're using this kernel).
> Could you review if we did it right?

Did you have to change anything?

thanks,

greg k-h

Re: SCSI fix (a621bac3)missing in stable

[Jinpu Wang]

On Wed, Jun 8, 2016 at 7:11 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
>> Hi all,
>>
>> We found the SCSI fix is missing in any stable only in linux 4.7-rc2
>> [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
>>  commands
>>
>> commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
>
> That is because it only hit a public release (4.7-rc2) on Sunday, please
> give us a chance to backport it to the other stable trees, it's in the
> queue.
>
>> So we backport & tested on linux-3.12 (as we're using this kernel).
>> Could you review if we did it right?
>
> Did you have to change anything?
>
> thanks,
>
> greg k-h

Hi Greg,

Thanks for stepping in. we have to adjust the context, because
scsi_lib changed quite a lot, due to SCSI-MQ support.
scsi_end_request was removed in bc85dc500, later add back another
version of scsi_end_request in f6d47e74f.

We will wait for response:-)


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

Re: SCSI fix (a621bac3)missing in stable

[Greg KH]

On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
> Hi all,
> 
> We found the SCSI fix is missing in any stable only in linux 4.7-rc2
> [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
>  commands
> 
> commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
> 
> So we backport & tested on linux-3.12 (as we're using this kernel).
> Could you review if we did it right?

This "backport" includes a bunch of checks and comments that are not in
the upstream kernel, so I don't really want to include it in the
3.14-stable queue unless you get the scsi maintainers to ack it.

thanks,

greg k-h

Re: SCSI fix (a621bac3)missing in stable

[Jinpu Wang]

On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
>> Hi all,
>>
>> We found the SCSI fix is missing in any stable only in linux 4.7-rc2
>> [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
>>  commands
>>
>> commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
>>
>> So we backport & tested on linux-3.12 (as we're using this kernel).
>> Could you review if we did it right?
>
> This "backport" includes a bunch of checks and comments that are not in
> the upstream kernel, so I don't really want to include it in the
> 3.14-stable queue unless you get the scsi maintainers to ack it.
>
> thanks,
>
> greg k-h

Thanks Greg!

Hi James, hi Martin

Could you give your Ack on the patch?

-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH

Re: SCSI fix (a621bac3)missing in stable

[Jinpu Wang]

On Wed, Jun 15, 2016 at 10:04 AM, Jinpu Wang
<jinpu.wang@profitbricks.com> wrote:
> On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
>>> Hi all,
>>>
>>> We found the SCSI fix is missing in any stable only in linux 4.7-rc2
>>> [PATCH] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
>>>  commands
>>>
>>> commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
>>>
>>> So we backport & tested on linux-3.12 (as we're using this kernel).
>>> Could you review if we did it right?
>>
>> This "backport" includes a bunch of checks and comments that are not in
>> the upstream kernel, so I don't really want to include it in the
>> 3.14-stable queue unless you get the scsi maintainers to ack it.
>>
>> thanks,
>>
>> greg k-h
>
> Thanks Greg!
>
> Hi James, hi Martin
>
> Could you give your Ack on the patch?
>
> --

Ping?
-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Re: SCSI fix (a621bac3)missing in stable

[James Bottomley]

On Fri, 2016-06-17 at 17:29 +0200, Jinpu Wang wrote:
> On Wed, Jun 15, 2016 at 10:04 AM, Jinpu Wang
> <jinpu.wang@profitbricks.com> wrote:
> > On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <
> > gregkh@linuxfoundation.org> wrote:
> > > On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
> > > > Hi all,
> > > > 
> > > > We found the SCSI fix is missing in any stable only in linux
> > > > 4.7-rc2
> > > > [PATCH] scsi_lib: correctly retry failed zero length
> > > > REQ_TYPE_FS
> > > >  commands
> > > > 
> > > > commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
> > > > 
> > > > So we backport & tested on linux-3.12 (as we're using this
> > > > kernel).
> > > > Could you review if we did it right?
> > > 
> > > This "backport" includes a bunch of checks and comments that are
> > > not in
> > > the upstream kernel, so I don't really want to include it in the
> > > 3.14-stable queue unless you get the scsi maintainers to ack it.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Thanks Greg!
> > 
> > Hi James, hi Martin
> > 
> > Could you give your Ack on the patch?
> > 
> > --
> 
> Ping?

How can I review this?  What you're proposing certainly isn't a
backport.  What you're actually trying to do is to port around this
commit:

ommit bc85dc500f9df9b2eec15077e5046672c46adeaa
Author: Christoph Hellwig <hch@lst.de>
Date:   Thu May 1 16:51:03 2014 +0200

    scsi: remove scsi_end_request

Have you tried simply backporting that to your tree and then applying
to work around?

James

Re: SCSI fix (a621bac3)missing in stable

[Jinpu Wang]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

On Fri, Jun 17, 2016 at 5:51 PM, James Bottomley
<jejb@linux.vnet.ibm.com> wrote:
> On Fri, 2016-06-17 at 17:29 +0200, Jinpu Wang wrote:
>> On Wed, Jun 15, 2016 at 10:04 AM, Jinpu Wang
>> <jinpu.wang@profitbricks.com> wrote:
>> > On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <
>> > gregkh@linuxfoundation.org> wrote:
>> > > On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
>> > > > Hi all,
>> > > >
>> > > > We found the SCSI fix is missing in any stable only in linux
>> > > > 4.7-rc2
>> > > > [PATCH] scsi_lib: correctly retry failed zero length
>> > > > REQ_TYPE_FS
>> > > >  commands
>> > > >
>> > > > commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
>> > > >
>> > > > So we backport & tested on linux-3.12 (as we're using this
>> > > > kernel).
>> > > > Could you review if we did it right?
>> > >
>> > > This "backport" includes a bunch of checks and comments that are
>> > > not in
>> > > the upstream kernel, so I don't really want to include it in the
>> > > 3.14-stable queue unless you get the scsi maintainers to ack it.
>> > >
>> > > thanks,
>> > >
>> > > greg k-h
>> >
>> > Thanks Greg!
>> >
>> > Hi James, hi Martin
>> >
>> > Could you give your Ack on the patch?
>> >
>> > --
>>
>> Ping?
>
> How can I review this?  What you're proposing certainly isn't a
> backport.  What you're actually trying to do is to port around this
> commit:
>
> ommit bc85dc500f9df9b2eec15077e5046672c46adeaa
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Thu May 1 16:51:03 2014 +0200
>
>     scsi: remove scsi_end_request
>
> Have you tried simply backporting that to your tree and then applying
> to work around?
>
> James
>

Thanks James for suggestions.
I did backport bc85dc500, and backport a621bac3, it passed my local tests.

Could you review patches attached?


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

[-- Attachment #2: 0001-scsi-remove-scsi_end_request.patch --]
[-- Type: text/x-patch, Size: 6641 bytes --]

From 3faa2678e59664f9184498e0faa787e24534f935 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 1 May 2014 16:51:03 +0200
Subject: [PATCH 1/2] scsi: remove scsi_end_request

commit bc85dc500f9df9b2eec15077e5046672c46adeaa upstream.

By folding scsi_end_request into its only caller we can significantly clean
up the completion logic.  We can use simple goto labels now to only have
a single place to finish or requeue command there instead of the previous
convoluted logic.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org>
Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
Reviewed-by: Hannes Reinecke <hare@suse.de>
[jwang: backport to 3.12]
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/scsi/scsi_lib.c | 113 +++++++++++++-----------------------------------
 1 file changed, 31 insertions(+), 82 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fd206c9..d3374cd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -541,66 +541,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 
 static void __scsi_release_buffers(struct scsi_cmnd *, int);
 
-/*
- * Function:    scsi_end_request()
- *
- * Purpose:     Post-processing of completed commands (usually invoked at end
- *		of upper level post-processing and scsi_io_completion).
- *
- * Arguments:   cmd	 - command that is complete.
- *              error    - 0 if I/O indicates success, < 0 for I/O error.
- *              bytes    - number of bytes of completed I/O
- *		requeue  - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns:     cmd if requeue required, NULL otherwise.
- *
- * Notes:       This is called for block device requests in order to
- *              mark some number of sectors as complete.
- * 
- *		We are guaranteeing that the request queue will be goosed
- *		at some point during this call.
- * Notes:	If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
-					  int bytes, int requeue)
-{
-	struct request_queue *q = cmd->device->request_queue;
-	struct request *req = cmd->request;
-
-	/*
-	 * If there are blocks left over at the end, set up the command
-	 * to queue the remainder of them.
-	 */
-	if (blk_end_request(req, error, bytes)) {
-		/* kill remainder if no retrys */
-		if (error && scsi_noretry_cmd(cmd))
-			blk_end_request_all(req, error);
-		else {
-			if (requeue) {
-				/*
-				 * Bleah.  Leftovers again.  Stick the
-				 * leftovers in the front of the
-				 * queue, and goose the queue again.
-				 */
-				scsi_release_buffers(cmd);
-				scsi_requeue_command(q, cmd);
-				cmd = NULL;
-			}
-			return cmd;
-		}
-	}
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	__scsi_release_buffers(cmd, 0);
-	scsi_next_command(cmd);
-	return NULL;
-}
-
 static inline unsigned int scsi_sgtable_index(unsigned short nents)
 {
 	unsigned int index;
@@ -752,16 +692,9 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
  *
  * Returns:     Nothing
  *
- * Notes:       This function is matched in terms of capabilities to
- *              the function that created the scatter-gather list.
- *              In other words, if there are no bounce buffers
- *              (the normal case for most drivers), we don't need
- *              the logic to deal with cleaning up afterwards.
- *
- *		We must call scsi_end_request().  This will finish off
- *		the specified number of sectors.  If we are done, the
- *		command block will be released and the queue function
- *		will be goosed.  If we are not done then we have to
+ * Notes:       We will finish off the specified number of sectors.  If we
+ *		are done, the command block will be released and the queue
+ *		function will be goosed.  If we are not done then we have to
  *		figure out what to do next:
  *
  *		a) We can call scsi_requeue_command().  The request
@@ -770,7 +703,7 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
  *		   be used if we made forward progress, or if we want
  *		   to switch from READ(10) to READ(6) for example.
  *
- *		b) We can call scsi_queue_insert().  The request will
+ *		b) We can call __scsi_queue_insert().  The request will
  *		   be put back on the queue and retried using the same
  *		   command as before, possibly after a delay.
  *
@@ -876,12 +809,25 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	/*
-	 * A number of bytes were successfully read.  If there
-	 * are leftovers and there is some kind of error
-	 * (result != 0), retry the rest.
+	 * If we finished all bytes in the request we are done now.
 	 */
-	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
-		return;
+	if (!blk_end_request(req, error, good_bytes))
+		goto next_command;
+
+	/*
+	 * Kill remainder if no retrys.
+	 */
+	if (error && scsi_noretry_cmd(cmd)) {
+		blk_end_request_all(req, error);
+		goto next_command;
+	}
+
+	/*
+	 * If there had been no error, but we have leftover bytes in the
+	 * requeues just queue the command up again.
+	 */
+	if (result == 0)
+		goto requeue;
 
 	error = __scsi_error_from_host_byte(cmd, result);
 
@@ -1003,7 +949,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	switch (action) {
 	case ACTION_FAIL:
 		/* Give up and fail the remainder of the request */
-		scsi_release_buffers(cmd);
 		if (!(req->cmd_flags & REQ_QUIET) && __ratelimit(&rs)) {
 			if (description)
 				scmd_printk(KERN_INFO, cmd, "%s\n",
@@ -1013,12 +958,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				scsi_print_sense("", cmd);
 			scsi_print_command(cmd);
 		}
-		if (blk_end_request_err(req, error))
-			scsi_requeue_command(q, cmd);
-		else
-			scsi_next_command(cmd);
-		break;
+		if (!blk_end_request_err(req, error))
+			goto next_command;
+		/*FALLTHRU*/
 	case ACTION_REPREP:
+	requeue:
 		/* Unprep the request and put it back at the head of the queue.
 		 * A new command will be prepared and issued.
 		 */
@@ -1034,6 +978,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0);
 		break;
 	}
+	return;
+
+next_command:
+	__scsi_release_buffers(cmd, 0);
+	scsi_next_command(cmd);
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
-- 
1.9.1


[-- Attachment #3: 0002-scsi_lib-correctly-retry-failed-zero-length-REQ_TYPE.patch --]
[-- Type: text/x-patch, Size: 2171 bytes --]

From 6c259143c1701fd3e20359a5182e3c0480aff404 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Fri, 13 May 2016 12:04:06 -0700
Subject: [PATCH 2/2] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
 commands

commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.

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.

Cc: stable@vger.kernel.org
Reported-by: Sebastian Parschauer <s.parschauer@gmx.de>
Signed-off-by: James E.J. Bottomley <jejb@linux.vnet.ibm.com>
Tested-by: Jack Wang <jinpu.wang@profitbricks.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ jwang: backport from upstream 4.7 to fix scsi resize issue ]
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/scsi/scsi_lib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d3374cd..4184468 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -809,9 +809,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 (!blk_end_request(req, error, good_bytes))
+	if (!(blk_rq_bytes(req) == 0 && error) &&
+	    !blk_end_request(req, error, good_bytes))
 		goto next_command;
 
 	/*
-- 
1.9.1

Re: SCSI fix (a621bac3)missing in stable

[James Bottomley]

On Mon, 2016-06-20 at 12:23 +0200, Jinpu Wang wrote:
> On Fri, Jun 17, 2016 at 5:51 PM, James Bottomley
> <jejb@linux.vnet.ibm.com> wrote:
> > On Fri, 2016-06-17 at 17:29 +0200, Jinpu Wang wrote:
> > > On Wed, Jun 15, 2016 at 10:04 AM, Jinpu Wang
> > > <jinpu.wang@profitbricks.com> wrote:
> > > > On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <
> > > > gregkh@linuxfoundation.org> wrote:
> > > > > On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > We found the SCSI fix is missing in any stable only in
> > > > > > linux
> > > > > > 4.7-rc2
> > > > > > [PATCH] scsi_lib: correctly retry failed zero length
> > > > > > REQ_TYPE_FS
> > > > > >  commands
> > > > > > 
> > > > > > commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
> > > > > > 
> > > > > > So we backport & tested on linux-3.12 (as we're using this
> > > > > > kernel).
> > > > > > Could you review if we did it right?
> > > > > 
> > > > > This "backport" includes a bunch of checks and comments that
> > > > > are
> > > > > not in
> > > > > the upstream kernel, so I don't really want to include it in
> > > > > the
> > > > > 3.14-stable queue unless you get the scsi maintainers to ack
> > > > > it.
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h
> > > > 
> > > > Thanks Greg!
> > > > 
> > > > Hi James, hi Martin
> > > > 
> > > > Could you give your Ack on the patch?
> > > > 
> > > > --
> > > 
> > > Ping?
> > 
> > How can I review this?  What you're proposing certainly isn't a
> > backport.  What you're actually trying to do is to port around this
> > commit:
> > 
> > ommit bc85dc500f9df9b2eec15077e5046672c46adeaa
> > Author: Christoph Hellwig <hch@lst.de>
> > Date:   Thu May 1 16:51:03 2014 +0200
> > 
> >     scsi: remove scsi_end_request
> > 
> > Have you tried simply backporting that to your tree and then
> > applying
> > to work around?
> > 
> > James
> > 
> 
> Thanks James for suggestions.
> I did backport bc85dc500, and backport a621bac3, it passed my local
> tests.
> 
> Could you review patches attached?

I'm afraid 3.12 is way too old for me to remember the current state of
play.  However, if those two patches backport unmodified and you've
tested it, then you're good to go.

James

Re: SCSI fix (a621bac3)missing in stable

[Jinpu Wang]

[-- Attachment #1: Type: text/plain, Size: 2803 bytes --]

On Tue, Jun 21, 2016 at 4:45 PM, James Bottomley
<jejb@linux.vnet.ibm.com> wrote:
> On Mon, 2016-06-20 at 12:23 +0200, Jinpu Wang wrote:
>> On Fri, Jun 17, 2016 at 5:51 PM, James Bottomley
>> <jejb@linux.vnet.ibm.com> wrote:
>> > On Fri, 2016-06-17 at 17:29 +0200, Jinpu Wang wrote:
>> > > On Wed, Jun 15, 2016 at 10:04 AM, Jinpu Wang
>> > > <jinpu.wang@profitbricks.com> wrote:
>> > > > On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <
>> > > > gregkh@linuxfoundation.org> wrote:
>> > > > > On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
>> > > > > > Hi all,
>> > > > > >
>> > > > > > We found the SCSI fix is missing in any stable only in
>> > > > > > linux
>> > > > > > 4.7-rc2
>> > > > > > [PATCH] scsi_lib: correctly retry failed zero length
>> > > > > > REQ_TYPE_FS
>> > > > > >  commands
>> > > > > >
>> > > > > > commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
>> > > > > >
>> > > > > > So we backport & tested on linux-3.12 (as we're using this
>> > > > > > kernel).
>> > > > > > Could you review if we did it right?
>> > > > >
>> > > > > This "backport" includes a bunch of checks and comments that
>> > > > > are
>> > > > > not in
>> > > > > the upstream kernel, so I don't really want to include it in
>> > > > > the
>> > > > > 3.14-stable queue unless you get the scsi maintainers to ack
>> > > > > it.
>> > > > >
>> > > > > thanks,
>> > > > >
>> > > > > greg k-h
>> > > >
>> > > > Thanks Greg!
>> > > >
>> > > > Hi James, hi Martin
>> > > >
>> > > > Could you give your Ack on the patch?
>> > > >
>> > > > --
>> > >
>> > > Ping?
>> >
>> > How can I review this?  What you're proposing certainly isn't a
>> > backport.  What you're actually trying to do is to port around this
>> > commit:
>> >
>> > ommit bc85dc500f9df9b2eec15077e5046672c46adeaa
>> > Author: Christoph Hellwig <hch@lst.de>
>> > Date:   Thu May 1 16:51:03 2014 +0200
>> >
>> >     scsi: remove scsi_end_request
>> >
>> > Have you tried simply backporting that to your tree and then
>> > applying
>> > to work around?
>> >
>> > James
>> >
>>
>> Thanks James for suggestions.
>> I did backport bc85dc500, and backport a621bac3, it passed my local
>> tests.
>>
>> Could you review patches attached?
>
> I'm afraid 3.12 is way too old for me to remember the current state of
> play.  However, if those two patches backport unmodified and you've
> tested it, then you're good to go.
>
> James
>
>
Thanks James!

Hi Greg,
Could pick up this 2 patches for 3.14? I tried locally, it can be
applied cleanly on top of 3.14.72

And Jiri,
For 3.12, it can be applied cleanly on top of 3.12.61?


-- 
Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

[-- Attachment #2: 0002-scsi_lib-correctly-retry-failed-zero-length-REQ_TYPE.patch --]
[-- Type: text/x-patch, Size: 2171 bytes --]

From 2848fa899f9c64db4e9f4c204199758410927bab Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Fri, 13 May 2016 12:04:06 -0700
Subject: [PATCH 2/2] scsi_lib: correctly retry failed zero length REQ_TYPE_FS
 commands

commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.

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.

Cc: stable@vger.kernel.org
Reported-by: Sebastian Parschauer <s.parschauer@gmx.de>
Signed-off-by: James E.J. Bottomley <jejb@linux.vnet.ibm.com>
Tested-by: Jack Wang <jinpu.wang@profitbricks.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ jwang: backport from upstream 4.7 to fix scsi resize issue ]
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/scsi/scsi_lib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4b78933..aeff397 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -806,9 +806,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 (!blk_end_request(req, error, good_bytes))
+	if (!(blk_rq_bytes(req) == 0 && error) &&
+	    !blk_end_request(req, error, good_bytes))
 		goto next_command;
 
 	/*
-- 
1.9.1


[-- Attachment #3: 0001-scsi-remove-scsi_end_request.patch --]
[-- Type: text/x-patch, Size: 6621 bytes --]

From c181b3a5812f49b7870ec338fc609ce0ab74e67d Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 1 May 2014 16:51:03 +0200
Subject: [PATCH 1/2] scsi: remove scsi_end_request

commit bc85dc500f9df9b2eec15077e5046672c46adeaa upstream.

By folding scsi_end_request into its only caller we can significantly clean
up the completion logic.  We can use simple goto labels now to only have
a single place to finish or requeue command there instead of the previous
convoluted logic.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org>
Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
Reviewed-by: Hannes Reinecke <hare@suse.de>
[jwang: backport to 3.12]
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/scsi/scsi_lib.c | 113 +++++++++++++-----------------------------------
 1 file changed, 31 insertions(+), 82 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2b01c88..4b78933 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -540,66 +540,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 
 static void __scsi_release_buffers(struct scsi_cmnd *, int);
 
-/*
- * Function:    scsi_end_request()
- *
- * Purpose:     Post-processing of completed commands (usually invoked at end
- *		of upper level post-processing and scsi_io_completion).
- *
- * Arguments:   cmd	 - command that is complete.
- *              error    - 0 if I/O indicates success, < 0 for I/O error.
- *              bytes    - number of bytes of completed I/O
- *		requeue  - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns:     cmd if requeue required, NULL otherwise.
- *
- * Notes:       This is called for block device requests in order to
- *              mark some number of sectors as complete.
- * 
- *		We are guaranteeing that the request queue will be goosed
- *		at some point during this call.
- * Notes:	If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
-					  int bytes, int requeue)
-{
-	struct request_queue *q = cmd->device->request_queue;
-	struct request *req = cmd->request;
-
-	/*
-	 * If there are blocks left over at the end, set up the command
-	 * to queue the remainder of them.
-	 */
-	if (blk_end_request(req, error, bytes)) {
-		/* kill remainder if no retrys */
-		if (error && scsi_noretry_cmd(cmd))
-			blk_end_request_all(req, error);
-		else {
-			if (requeue) {
-				/*
-				 * Bleah.  Leftovers again.  Stick the
-				 * leftovers in the front of the
-				 * queue, and goose the queue again.
-				 */
-				scsi_release_buffers(cmd);
-				scsi_requeue_command(q, cmd);
-				cmd = NULL;
-			}
-			return cmd;
-		}
-	}
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	__scsi_release_buffers(cmd, 0);
-	scsi_next_command(cmd);
-	return NULL;
-}
-
 static inline unsigned int scsi_sgtable_index(unsigned short nents)
 {
 	unsigned int index;
@@ -751,16 +691,9 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
  *
  * Returns:     Nothing
  *
- * Notes:       This function is matched in terms of capabilities to
- *              the function that created the scatter-gather list.
- *              In other words, if there are no bounce buffers
- *              (the normal case for most drivers), we don't need
- *              the logic to deal with cleaning up afterwards.
- *
- *		We must call scsi_end_request().  This will finish off
- *		the specified number of sectors.  If we are done, the
- *		command block will be released and the queue function
- *		will be goosed.  If we are not done then we have to
+ * Notes:       We will finish off the specified number of sectors.  If we
+ *		are done, the command block will be released and the queue
+ *		function will be goosed.  If we are not done then we have to
  *		figure out what to do next:
  *
  *		a) We can call scsi_requeue_command().  The request
@@ -769,7 +702,7 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
  *		   be used if we made forward progress, or if we want
  *		   to switch from READ(10) to READ(6) for example.
  *
- *		b) We can call scsi_queue_insert().  The request will
+ *		b) We can call __scsi_queue_insert().  The request will
  *		   be put back on the queue and retried using the same
  *		   command as before, possibly after a delay.
  *
@@ -873,12 +806,25 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	/*
-	 * A number of bytes were successfully read.  If there
-	 * are leftovers and there is some kind of error
-	 * (result != 0), retry the rest.
+	 * If we finished all bytes in the request we are done now.
 	 */
-	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
-		return;
+	if (!blk_end_request(req, error, good_bytes))
+		goto next_command;
+
+	/*
+	 * Kill remainder if no retrys.
+	 */
+	if (error && scsi_noretry_cmd(cmd)) {
+		blk_end_request_all(req, error);
+		goto next_command;
+	}
+
+	/*
+	 * If there had been no error, but we have leftover bytes in the
+	 * requeues just queue the command up again.
+	 */
+	if (result == 0)
+		goto requeue;
 
 	error = __scsi_error_from_host_byte(cmd, result);
 
@@ -1000,7 +946,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	switch (action) {
 	case ACTION_FAIL:
 		/* Give up and fail the remainder of the request */
-		scsi_release_buffers(cmd);
 		if (!(req->cmd_flags & REQ_QUIET)) {
 			if (description)
 				scmd_printk(KERN_INFO, cmd, "%s\n",
@@ -1010,12 +955,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				scsi_print_sense("", cmd);
 			scsi_print_command(cmd);
 		}
-		if (blk_end_request_err(req, error))
-			scsi_requeue_command(q, cmd);
-		else
-			scsi_next_command(cmd);
-		break;
+		if (!blk_end_request_err(req, error))
+			goto next_command;
+		/*FALLTHRU*/
 	case ACTION_REPREP:
+	requeue:
 		/* Unprep the request and put it back at the head of the queue.
 		 * A new command will be prepared and issued.
 		 */
@@ -1031,6 +975,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0);
 		break;
 	}
+	return;
+
+next_command:
+	__scsi_release_buffers(cmd, 0);
+	scsi_next_command(cmd);
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
-- 
1.9.1

Re: SCSI fix (a621bac3)missing in stable

[Jinpu Wang]

On Tue, Jun 21, 2016 at 5:31 PM, Jinpu Wang <jinpu.wang@profitbricks.com> wrote:
> On Tue, Jun 21, 2016 at 4:45 PM, James Bottomley
> <jejb@linux.vnet.ibm.com> wrote:
>> On Mon, 2016-06-20 at 12:23 +0200, Jinpu Wang wrote:
>>> On Fri, Jun 17, 2016 at 5:51 PM, James Bottomley
>>> <jejb@linux.vnet.ibm.com> wrote:
>>> > On Fri, 2016-06-17 at 17:29 +0200, Jinpu Wang wrote:
>>> > > On Wed, Jun 15, 2016 at 10:04 AM, Jinpu Wang
>>> > > <jinpu.wang@profitbricks.com> wrote:
>>> > > > On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <
>>> > > > gregkh@linuxfoundation.org> wrote:
>>> > > > > On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
>>> > > > > > Hi all,
>>> > > > > >
>>> > > > > > We found the SCSI fix is missing in any stable only in
>>> > > > > > linux
>>> > > > > > 4.7-rc2
>>> > > > > > [PATCH] scsi_lib: correctly retry failed zero length
>>> > > > > > REQ_TYPE_FS
>>> > > > > >  commands
>>> > > > > >
>>> > > > > > commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
>>> > > > > >
>>> > > > > > So we backport & tested on linux-3.12 (as we're using this
>>> > > > > > kernel).
>>> > > > > > Could you review if we did it right?
>>> > > > >
>>> > > > > This "backport" includes a bunch of checks and comments that
>>> > > > > are
>>> > > > > not in
>>> > > > > the upstream kernel, so I don't really want to include it in
>>> > > > > the
>>> > > > > 3.14-stable queue unless you get the scsi maintainers to ack
>>> > > > > it.
>>> > > > >
>>> > > > > thanks,
>>> > > > >
>>> > > > > greg k-h
>>> > > >
>>> > > > Thanks Greg!
>>> > > >
>>> > > > Hi James, hi Martin
>>> > > >
>>> > > > Could you give your Ack on the patch?
>>> > > >
>>> > > > --
>>> > >
>>> > > Ping?
>>> >
>>> > How can I review this?  What you're proposing certainly isn't a
>>> > backport.  What you're actually trying to do is to port around this
>>> > commit:
>>> >
>>> > ommit bc85dc500f9df9b2eec15077e5046672c46adeaa
>>> > Author: Christoph Hellwig <hch@lst.de>
>>> > Date:   Thu May 1 16:51:03 2014 +0200
>>> >
>>> >     scsi: remove scsi_end_request
>>> >
>>> > Have you tried simply backporting that to your tree and then
>>> > applying
>>> > to work around?
>>> >
>>> > James
>>> >
>>>
>>> Thanks James for suggestions.
>>> I did backport bc85dc500, and backport a621bac3, it passed my local
>>> tests.
>>>
>>> Could you review patches attached?
>>
>> I'm afraid 3.12 is way too old for me to remember the current state of
>> play.  However, if those two patches backport unmodified and you've
>> tested it, then you're good to go.
>>
>> James
>>
>>
> Thanks James!
>
> Hi Greg,
> Could pick up this 2 patches for 3.14? I tried locally, it can be
> applied cleanly on top of 3.14.72
>
> And Jiri,
> For 3.12, it can be applied cleanly on top of 3.12.61?

Hi Greg & Jiri,

A soft ping?

Cheers
Jack

Re: SCSI fix (a621bac3)missing in stable

[Greg KH]

On Wed, Jun 29, 2016 at 02:36:08PM +0200, Jinpu Wang wrote:
> On Tue, Jun 21, 2016 at 5:31 PM, Jinpu Wang <jinpu.wang@profitbricks.com> wrote:
> > On Tue, Jun 21, 2016 at 4:45 PM, James Bottomley
> > <jejb@linux.vnet.ibm.com> wrote:
> >> On Mon, 2016-06-20 at 12:23 +0200, Jinpu Wang wrote:
> >>> On Fri, Jun 17, 2016 at 5:51 PM, James Bottomley
> >>> <jejb@linux.vnet.ibm.com> wrote:
> >>> > On Fri, 2016-06-17 at 17:29 +0200, Jinpu Wang wrote:
> >>> > > On Wed, Jun 15, 2016 at 10:04 AM, Jinpu Wang
> >>> > > <jinpu.wang@profitbricks.com> wrote:
> >>> > > > On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <
> >>> > > > gregkh@linuxfoundation.org> wrote:
> >>> > > > > On Wed, Jun 08, 2016 at 06:47:40PM +0200, Jinpu Wang wrote:
> >>> > > > > > Hi all,
> >>> > > > > >
> >>> > > > > > We found the SCSI fix is missing in any stable only in
> >>> > > > > > linux
> >>> > > > > > 4.7-rc2
> >>> > > > > > [PATCH] scsi_lib: correctly retry failed zero length
> >>> > > > > > REQ_TYPE_FS
> >>> > > > > >  commands
> >>> > > > > >
> >>> > > > > > commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.
> >>> > > > > >
> >>> > > > > > So we backport & tested on linux-3.12 (as we're using this
> >>> > > > > > kernel).
> >>> > > > > > Could you review if we did it right?
> >>> > > > >
> >>> > > > > This "backport" includes a bunch of checks and comments that
> >>> > > > > are
> >>> > > > > not in
> >>> > > > > the upstream kernel, so I don't really want to include it in
> >>> > > > > the
> >>> > > > > 3.14-stable queue unless you get the scsi maintainers to ack
> >>> > > > > it.
> >>> > > > >
> >>> > > > > thanks,
> >>> > > > >
> >>> > > > > greg k-h
> >>> > > >
> >>> > > > Thanks Greg!
> >>> > > >
> >>> > > > Hi James, hi Martin
> >>> > > >
> >>> > > > Could you give your Ack on the patch?
> >>> > > >
> >>> > > > --
> >>> > >
> >>> > > Ping?
> >>> >
> >>> > How can I review this?  What you're proposing certainly isn't a
> >>> > backport.  What you're actually trying to do is to port around this
> >>> > commit:
> >>> >
> >>> > ommit bc85dc500f9df9b2eec15077e5046672c46adeaa
> >>> > Author: Christoph Hellwig <hch@lst.de>
> >>> > Date:   Thu May 1 16:51:03 2014 +0200
> >>> >
> >>> >     scsi: remove scsi_end_request
> >>> >
> >>> > Have you tried simply backporting that to your tree and then
> >>> > applying
> >>> > to work around?
> >>> >
> >>> > James
> >>> >
> >>>
> >>> Thanks James for suggestions.
> >>> I did backport bc85dc500, and backport a621bac3, it passed my local
> >>> tests.
> >>>
> >>> Could you review patches attached?
> >>
> >> I'm afraid 3.12 is way too old for me to remember the current state of
> >> play.  However, if those two patches backport unmodified and you've
> >> tested it, then you're good to go.
> >>
> >> James
> >>
> >>
> > Thanks James!
> >
> > Hi Greg,
> > Could pick up this 2 patches for 3.14? I tried locally, it can be
> > applied cleanly on top of 3.14.72
> >
> > And Jiri,
> > For 3.12, it can be applied cleanly on top of 3.12.61?
> 
> Hi Greg & Jiri,
> 
> A soft ping?

Please be patient, this is behind 150+ other patches at the moment...

greg k-h

Re: SCSI fix (a621bac3)missing in stable

[Jiri Slaby]

On 06/21/2016, 05:31 PM, Jinpu Wang wrote:
> And Jiri,
> For 3.12, it can be applied cleanly on top of 3.12.61?

Now applied, thanks!

-- 
js
suse labs