On Tue, Jun 21, 2016 at 4:45 PM, James Bottomley
<jejb@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> wrote:
>> > > > On Tue, Jun 14, 2016 at 10:03 PM, Greg KH <
>> > > > gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxx>
>> > 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.
From 2848fa899f9c64db4e9f4c204199758410927bab Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx
Reported-by: Sebastian Parschauer <s.parschauer@xxxxxx>
Signed-off-by: James E.J. Bottomley <jejb@xxxxxxxxxxxxxxxxxx>
Tested-by: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxxx>
Signed-off-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
[ jwang: backport from upstream 4.7 to fix scsi resize issue ]
Signed-off-by: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxxx>
---
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
From c181b3a5812f49b7870ec338fc609ce0ab74e67d Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@xxxxxx>
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@xxxxxx>
Reviewed-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
Reviewed-by: Mike Christie <michaelc@xxxxxxxxxxx>
Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
[jwang: backport to 3.12]
Signed-off-by: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxxx>
---
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