[PATCHv2] sd: Don't treat succeeded SYNC as error

Commit Message

Jinpu Wang May 10, 2016, 2:48 p.m. UTC

On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <[e-mail]> wrote:
> On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <[e-mail]> wrote:
>> On Mon, May 2, 2016 at 3:44 PM, James Bottomley <[e-mail]> wrote:
>>> On Mon, 2016-05-02 at 12:05 +0200, Hannes Reinecke wrote:
>>>> On 04/29/2016 02:49 PM, Jinpu Wang wrote:
>>>> > Hi, all
>>>> >
>>>> > We hit IO error on fsync, it turns out was because sd treat
>>>> > succeeded
>>>> > SYNC as error. From what I checked in SBC spec there is no
>>>> > indication
>>>> > we should fail IO in this case, so we create this patch.
>>>> >
>>>> >
>>>> > Best Regards,
>>>> >
>>>> > Jack Wang
>>>> >
>>>> > v2:
>>>> > No change on patch itself, only resend in body as suggested by
>>>> > Bart,
>>>> > still keep the attachment in case mail client break the format.
>>>> >
>>>> > From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00
>>>> > 2001
>>>> > From: Jack Wang <[e-mail]>
>>>> > Date: Mon, 25 Apr 2016 12:05:22 +0200
>>>> > Subject: [PATCH] sd: Don't treat succeeded SYNC as error
>>>> >
>>>> > We hit IO error in our production on multipath devices during
>>>> > resize
>>>> > device on target side, the problem turns out sd driver passes up as
>>>> > IO
>>>> > error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
>>>> > Capacity data has changed, even storage side sync the data
>>>> > properly.
>>>> >
>>>> > In order to fix this check in sd_done, report success if condition
>>>> > matches.
>>>> >
>>>> > Sebastian Parschauer report/analyze the bug here:
>>>> > https://sourceforge.net/p/scst/mailman/message/34953416/
>>>> >
>>>> > Signed-off-by: Sebastian Parschauer <[e-mail]>
>>>> > Signed-off-by: Jack Wang <[e-mail]>
>>>> > ---
>>>> >  drivers/scsi/sd.c | 13 +++++++++++++
>>>> >  1 file changed, 13 insertions(+)
>>>> >
>>>> Well.
>>>> Is there anything which guarantees us that 'capacity data has
>>>> changed' will be the only sense code which we'll be seeing as a
>>>> response to SYNCHRONIZE CACHE?
>>>> I sincerely doubt so.
>>>> So why don't you fall back to the default action (ie retry the
>>>> command) whenever you hit an UNIT ATTENTION?
>>>> This way we would cove any resulting sense code, _and_ would get rid
>>>> of the rather ugly special case here.
>>>
>>> Actually, why are we getting here at all?  should we be eating this
>>> unit attention once we've reported it in scsi_check_sense()?
>>>
>>> I also don't quite understand why the normal retry mechanism in
>>> scsi_io_completion() (called after drv->done()) isn't handling this.
>>>  We set retries on a flush command and we give sd_sync_cache three
>>> goes.  Any one of those should also cause the CC/UA to be ignored.
>>>
>>> James
>>>
>>>
>>
>> Sorry for delay, I agree safer to retry this command.
>> I checked the code path, in scsi_io_completion, we call
>> __scsi_error_from_host_byte for FLUSH request,
>> and we set error to EIO by default, somehow the code report error
>> directly to user space without retry.
>> [  647.206270] sd 1:0:0:0: [sdb] tag#0 Send: scmd 0xffff8800b6558480
>> [  647.206422] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35
>> 00 00 00 00 00 00 00 00 00
>> [  647.209748] sd 1:0:0:0: Capacity data has changed
>> [  647.209896] sd 1:0:0:0: [sdb] tag#0 Done: SUCCESS Result:
>> hostbyte=DID_OK driverbyte=DRIVER_OK
>> [  647.210157] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35
>> 00 00 00 00 00 00 00 00 00
>> [  647.210419] sd 1:0:0:0: [sdb] tag#0 Sense Key : Unit Attention [current]
>> [  647.210567] sd 1:0:0:0: [sdb] tag#0 Add. Sense: Capacity data has changed
>> [  647.210741] sd 1:0:0:0: [sdb] tag#0 scsi host busy 1 failed 0
>> [  647.210888] sd 1:0:0:0: Notifying upper driver of completion (result 8000002)
>> [  647.211035] sd 1:0:0:0: [sdb] tag#0 sd_done: completed 0 of 0 bytes
>> [  647.211182] sd 1:0:0:0: [sdb] tag#0 0 sectors total, 0 bytes done, error -5
>> [  647.211330] blk_update_request: I/O error, dev sdb, sector 0
>>
>> Will figure out why retry doesn't work.
>>
>> Thanks James and Hannes for all your input.
>>
>> Regards,
>> Jack
>
> Hi James, Hannes and all,
>
> I find out it's code below which report error directly back to user
> space without any retry.
>  913         /*
>  914          * If we finished all bytes in the request we are done now.
>  915          */
>  916         if (!scsi_end_request(req, error, good_bytes, 0))
>  917                 return;
>
> But not sure, what's the best way to fix the behavior to let it retry,
>  maybe add condition with sense key && asc && ascq direct go to
> requeue before line 913?
>
> Thanks
> Jack


Hi James , Hannes and all,

I created a patch below, I did basic test on my test machines. Please
share your comments!

Thanks,
Jack
From 72ab860811e14e37db81fb409abf0fa7e7fe32cb Mon Sep 17 00:00:00 2001
From: Jack Wang <[e-mail]>
Date: Tue, 10 May 2016 10:10:59 +0200
Subject: [PATCH] scsi: requeue command on capacity data has changed

We hit IO error in our production on multipath devices during resize
device on target side, the problem turns out scsi driver passes up as IO
error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
Capacity data has changed, even storage side sync the data properly.

To fix this, when condition met, we simply requeue the command.

Reported-by: Sebastian Parschauer <[e-mail]>
Signed-off-by: Jack Wang <[e-mail]>
---
 drivers/scsi/scsi_lib.c | 5 +++++
 1 file changed, 5 insertions(+)

  */

Patch

From 72ab860811e14e37db81fb409abf0fa7e7fe32cb Mon Sep 17 00:00:00 2001
From: Jack Wang <[e-mail]>
Date: Tue, 10 May 2016 10:10:59 +0200
Subject: [PATCH] scsi: requeue command on capacity data has changed

We hit IO error in our production on multipath devices during resize
device on target side, the problem turns out scsi driver passes up as IO
error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate
Capacity data has changed, even storage side sync the data properly.

To fix this, when condition met, we simply requeue the command.

Reported-by: Sebastian Parschauer <[e-mail]>
Signed-off-by: Jack Wang <[e-mail]>
---
 drivers/scsi/scsi_lib.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..b00310f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -910,6 +910,11 @@  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		error = 0;
 	}
 
+	if (sense_valid && (sshdr.sense_key == UNIT_ATTENTION)) {
+		if ((sshdr.asc == 0x2a && sshdr.ascq == 0x09))
+			goto requeue;
+	}
+
 	/*
 	 * If we finished all bytes in the request we are done now.
 	 */
-- 
1.9.1