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

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

[Jinpu Wang]

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

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 <jinpu.wang@profitbricks.com>
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 <s.parschauer@gmx.de>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/scsi/sd.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..e9bfe01 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1833,6 +1833,19 @@ static int sd_done(struct scsi_cmnd *SCpnt)
  }
  }
  break;
+ case UNIT_ATTENTION:
+ /* Capacity data has changed */
+ if (sshdr.asc == 0x2a && sshdr.ascq == 0x09) {
+ switch (op) {
+ /* don't treat succeeded fsync() as error */
+ case SYNCHRONIZE_CACHE:
+ case SYNCHRONIZE_CACHE_16:
+ if (good_bytes == scsi_bufflen(SCpnt))
+ SCpnt->result = 0;
+ break;
+ }
+ }
+ break;
  default:
  break;
  }
-- 
1.9.1

[-- Attachment #2: 0001-sd-Don-t-treat-succeeded-SYNC-as-error.patch --]
[-- Type: text/x-patch, Size: 1453 bytes --]

From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang@profitbricks.com>
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 <s.parschauer@gmx.de>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/scsi/sd.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5a5457a..e9bfe01 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1833,6 +1833,19 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 			}
 		}
 		break;
+	case UNIT_ATTENTION:
+		/* Capacity data has changed */
+		if (sshdr.asc == 0x2a && sshdr.ascq == 0x09) {
+			switch (op) {
+			/* don't treat succeeded fsync() as error */
+			case SYNCHRONIZE_CACHE:
+			case SYNCHRONIZE_CACHE_16:
+				if (good_bytes == scsi_bufflen(SCpnt))
+					SCpnt->result = 0;
+					break;
+			}
+		}
+		break;
 	default:
 		break;
 	}
-- 
1.9.1

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

[Hannes Reinecke]

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 <jinpu.wang@profitbricks.com>
> 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 <s.parschauer@gmx.de>
> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> ---
>  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.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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: [PATCHv2]sd: Don't treat succeeded SYNC as error

[James Bottomley]

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 <jinpu.wang@profitbricks.com>
> > 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 <s.parschauer@gmx.de>
> > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> > ---
> >  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

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

[James Bottomley]

On Mon, 2016-05-02 at 06:44 -0700, James Bottomley 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 <jinpu.wang@profitbricks.com>
> > > 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 <s.parschauer@gmx.de>
> > > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> > > ---
> > >  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.

Actually, there's another problem with this patch: you're clearing the
error and indicating success, meaning you never retry.  CC/UA for
notifications is usually signalled in the device before acting on the
command, so the chances are your SYNC request was never executed and if
you never retry we'll be operating in a cache unstaged situation where
we're assuming data will be on the medium.

James

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

[Jinpu Wang]

On Mon, May 2, 2016 at 3:44 PM, James Bottomley <jejb@linux.vnet.ibm.com> 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 <jinpu.wang@profitbricks.com>
>> > 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 <s.parschauer@gmx.de>
>> > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>> > ---
>> >  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

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

[Jinpu Wang]

On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <jinpu.wang@profitbricks.com> wrote:
> On Mon, May 2, 2016 at 3:44 PM, James Bottomley <jejb@linux.vnet.ibm.com> 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 <jinpu.wang@profitbricks.com>
>>> > 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 <s.parschauer@gmx.de>
>>> > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>>> > ---
>>> >  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

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

[Jinpu Wang]

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

On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <jinpu.wang@profitbricks.com> wrote:
> On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <jinpu.wang@profitbricks.com> wrote:
>> On Mon, May 2, 2016 at 3:44 PM, James Bottomley <jejb@linux.vnet.ibm.com> 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 <jinpu.wang@profitbricks.com>
>>>> > 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 <s.parschauer@gmx.de>
>>>> > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>>>> > ---
>>>> >  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 <jinpu.wang@profitbricks.com>
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 <s.parschauer@gmx.de>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 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

[-- Attachment #2: 0001-scsi-requeue-command-on-capacity-data-has-changed.patch --]
[-- Type: text/x-patch, Size: 1234 bytes --]

From 72ab860811e14e37db81fb409abf0fa7e7fe32cb Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang@profitbricks.com>
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 <s.parschauer@gmx.de>
Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 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

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

[James Bottomley]

On Tue, 2016-05-10 at 16:48 +0200, Jinpu Wang wrote:
> On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <
> jinpu.wang@profitbricks.com> wrote:
> > On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <
> > jinpu.wang@profitbricks.com> wrote:
> > > On Mon, May 2, 2016 at 3:44 PM, James Bottomley <
> > > jejb@linux.vnet.ibm.com> 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 <jinpu.wang@profitbricks.com>
> > > > > > 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 <s.parschauer@gmx.de>
> > > > > > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> > > > > > ---
> > > > > >  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 <jinpu.wang@profitbricks.com>
> 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 <s.parschauer@gmx.de>
> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> ---
>  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;
> + }
> +

Actually, I think this is symptomatic of a much bigger problem.  Now
that the FS can send zero length non BLOCK_PC request, we're not
treating failure correctly.  blk_update_request() will always finish
them becuase they have no bytes outstanding (not having any in the
first case).  So I think we need a special exception for all zero
length commands which complete with a failure to allow them to retry
(if required).

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8106515..5a97866 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 (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result != 0) &&
+	    !scsi_end_request(req, error, good_bytes, 0))
 		return;
 
 	/*

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

[Jinpu Wang]

On Tue, May 10, 2016 at 5:08 PM, James Bottomley
<jejb@linux.vnet.ibm.com> wrote:
> On Tue, 2016-05-10 at 16:48 +0200, Jinpu Wang wrote:
>> On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <
>> jinpu.wang@profitbricks.com> wrote:
>> > On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <
>> > jinpu.wang@profitbricks.com> wrote:
>> > > On Mon, May 2, 2016 at 3:44 PM, James Bottomley <
>> > > jejb@linux.vnet.ibm.com> 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 <jinpu.wang@profitbricks.com>
>> > > > > > 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 <s.parschauer@gmx.de>
>> > > > > > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>> > > > > > ---
>> > > > > >  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 <jinpu.wang@profitbricks.com>
>> 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 <s.parschauer@gmx.de>
>> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>> ---
>>  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;
>> + }
>> +
>
> Actually, I think this is symptomatic of a much bigger problem.  Now
> that the FS can send zero length non BLOCK_PC request, we're not
> treating failure correctly.  blk_update_request() will always finish
> them becuase they have no bytes outstanding (not having any in the
> first case).  So I think we need a special exception for all zero
> length commands which complete with a failure to allow them to retry
> (if required).

Which request do you mean, I only find FLUSH?

I will test your patch.

Thanks,
Jack

>
> James
>
> ---
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..5a97866 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 (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result != 0) &&
> +           !scsi_end_request(req, error, good_bytes, 0))
>                 return;
>
>         /*
>



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

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

ProfitBricks GmbH
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 5770083-42
Fax: +49 30 5770085-98
Email: jinpu.wang@profitbricks.com
URL: http://www.profitbricks.de

Sitz der Gesellschaft: Berlin.
Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
Geschäftsführer: Andreas Gauger, Achim Weiss.
--
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: [PATCHv2]sd: Don't treat succeeded SYNC as error

[James Bottomley]

On Tue, 2016-05-10 at 17:46 +0200, Jinpu Wang wrote:
> On Tue, May 10, 2016 at 5:08 PM, James Bottomley
> <jejb@linux.vnet.ibm.com> wrote:
[...]
> > Actually, I think this is symptomatic of a much bigger problem. 
> >  Now that the FS can send zero length non BLOCK_PC request, we're 
> > not treating failure correctly.  blk_update_request() will always
> > finish them becuase they have no bytes outstanding (not having any 
> > in the first case).  So I think we need a special exception for all 
> > zero length commands which complete with a failure to allow them to
> > retry (if required).
> 
> Which request do you mean, I only find FLUSH?

Flush is the only one currently, but zero length fs requests didn't
exist when this bit of SCSI was coded, which is why all the code judges
request completion on have we completed all the bytes (which is
obviously true if you had no bytes to send in the first place).

> I will test your patch.

Thanks.

James

> Thanks,
> Jack
> 
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 8106515..5a97866 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 (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result
> > != 0) &&
> > +           !scsi_end_request(req, error, good_bytes, 0))
> >                 return;
> > 
> >         /*
> > 
> 
> 
> 

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

[Hannes Reinecke]

On 05/10/2016 05:08 PM, James Bottomley wrote:
> On Tue, 2016-05-10 at 16:48 +0200, Jinpu Wang wrote:
>> On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <
>> jinpu.wang@profitbricks.com> wrote:
>>> On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <
>>> jinpu.wang@profitbricks.com> wrote:
>>>> On Mon, May 2, 2016 at 3:44 PM, James Bottomley <
>>>> jejb@linux.vnet.ibm.com> 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 <jinpu.wang@profitbricks.com>
>>>>>>> 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 <s.parschauer@gmx.de>
>>>>>>> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>>>>>>> ---
>>>>>>>  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 <jinpu.wang@profitbricks.com>
>> 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 <s.parschauer@gmx.de>
>> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>> ---
>>  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;
>> + }
>> +
> 
> Actually, I think this is symptomatic of a much bigger problem.  Now
> that the FS can send zero length non BLOCK_PC request, we're not
> treating failure correctly.  blk_update_request() will always finish
> them becuase they have no bytes outstanding (not having any in the
> first case).  So I think we need a special exception for all zero
> length commands which complete with a failure to allow them to retry
> (if required).
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8106515..5a97866 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 (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result != 0) &&
> +	    !scsi_end_request(req, error, good_bytes, 0))
>  		return;
>  
>  	/*
> 
My, this is ugly.
Plus most of this _should_ have been handled by these lines just above:
} else if (blk_rq_bytes(req) == 0 && result && !sense_deferred) {
	/*
	 * Certain non BLOCK_PC requests are commands that don't
	 * actually transfer anything (FLUSH), so cannot use
	 * good_bytes != blk_rq_bytes(req) as the signal for an error.
	 * This sets the error explicitly for the problem case.
	 */
	error = __scsi_error_from_host_byte(cmd, result);
}

Wouldn't this patch fix it as well?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7cb66b0..68c0e74 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -693,7 +693,7 @@ static bool scsi_end_request(struct request
*req, int error,
        struct scsi_device *sdev = cmd->device;
        struct request_queue *q = sdev->request_queue;

-       if (blk_update_request(req, error, bytes))
+       if (bytes && blk_update_request(req, error, bytes))
                return true;

        /* Bidi request must be completed as a whole */

Which actually looks like a valid patch anyway ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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: [PATCHv2]sd: Don't treat succeeded SYNC as error

[James Bottomley]

On Wed, 2016-05-11 at 08:21 +0200, Hannes Reinecke wrote:
> On 05/10/2016 05:08 PM, James Bottomley wrote:
> > On Tue, 2016-05-10 at 16:48 +0200, Jinpu Wang wrote:
> > > On Mon, May 9, 2016 at 6:41 PM, Jinpu Wang <
> > > jinpu.wang@profitbricks.com> wrote:
> > > > On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang <
> > > > jinpu.wang@profitbricks.com> wrote:
> > > > > On Mon, May 2, 2016 at 3:44 PM, James Bottomley <
> > > > > jejb@linux.vnet.ibm.com> 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 <jinpu.wang@profitbricks.com>
> > > > > > > > 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 <
> > > > > > > > s.parschauer@gmx.de>
> > > > > > > > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> > > > > > > > ---
> > > > > > > >  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 <jinpu.wang@profitbricks.com>
> > > 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 <s.parschauer@gmx.de>
> > > Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> > > ---
> > >  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;
> > > + }
> > > +
> > 
> > Actually, I think this is symptomatic of a much bigger problem. 
> >  Now
> > that the FS can send zero length non BLOCK_PC request, we're not
> > treating failure correctly.  blk_update_request() will always
> > finish
> > them becuase they have no bytes outstanding (not having any in the
> > first case).  So I think we need a special exception for all zero
> > length commands which complete with a failure to allow them to
> > retry
> > (if required).
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 8106515..5a97866 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 (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result
> > != 0) &&
> > +	    !scsi_end_request(req, error, good_bytes, 0))
> >  		return;
> >  
> >  	/*
> > 
> My, this is ugly.
> Plus most of this _should_ have been handled by these lines just
> above:
> } else if (blk_rq_bytes(req) == 0 && result && !sense_deferred) {
> 	/*
> 	 * Certain non BLOCK_PC requests are commands that don't
> 	 * actually transfer anything (FLUSH), so cannot use
> 	 * good_bytes != blk_rq_bytes(req) as the signal for an error.
> 	 * This sets the error explicitly for the problem case.
> 	 */
> 	error = __scsi_error_from_host_byte(cmd, result);
> }

No, that's making sure that a flush error is detected.  The problem is
that we don't retry the command on a retryable error even if we have
retries remaining.

> Wouldn't this patch fix it as well?

Perhaps we validate the theory of the problem first before we start
quibbling about the best way to fix it ...

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7cb66b0..68c0e74 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -693,7 +693,7 @@ static bool scsi_end_request(struct request
> *req, int error,
>         struct scsi_device *sdev = cmd->device;
>         struct request_queue *q = sdev->request_queue;
> 
> -       if (blk_update_request(req, error, bytes))
> +       if (bytes && blk_update_request(req, error, bytes))
>                 return true;

Um, I think you mean 

if (bytes == 0 || blk_update_request())

James

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

[Jinpu Wang]

On Tue, May 10, 2016 at 6:00 PM, James Bottomley
<jejb@linux.vnet.ibm.com> wrote:
> On Tue, 2016-05-10 at 17:46 +0200, Jinpu Wang wrote:
>> On Tue, May 10, 2016 at 5:08 PM, James Bottomley
>> <jejb@linux.vnet.ibm.com> wrote:
> [...]
>> > Actually, I think this is symptomatic of a much bigger problem.
>> >  Now that the FS can send zero length non BLOCK_PC request, we're
>> > not treating failure correctly.  blk_update_request() will always
>> > finish them becuase they have no bytes outstanding (not having any
>> > in the first case).  So I think we need a special exception for all
>> > zero length commands which complete with a failure to allow them to
>> > retry (if required).
>>
>> Which request do you mean, I only find FLUSH?
>
> Flush is the only one currently, but zero length fs requests didn't
> exist when this bit of SCSI was coded, which is why all the code judges
> request completion on have we completed all the bytes (which is
> obviously true if you had no bytes to send in the first place).
>
>> I will test your patch.
>
> Thanks.
>
> James
>
>> Thanks,
>> Jack
>>
>> >
>> > James
>> >
>> > ---
>> >
>> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> > index 8106515..5a97866 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 (!(good_bytes == 0 && blk_rq_bytes(req) == 0 && result
>> > != 0) &&
>> > +           !scsi_end_request(req, error, good_bytes, 0))
>> >                 return;
>> >
>> >         /*
>> >
>>
>>
>>
>

Hi James,

Your patch also fix the error for me. I'm also thinking a patch like this. :)
Could you send out a formal patch, you can add my Tested-by.

Regards,
Jack

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

[James Bottomley]

On Tue, 2016-05-10 at 23:43 -0700, James Bottomley wrote:
> On Wed, 2016-05-11 at 08:21 +0200, Hannes Reinecke wrote:
[..]
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 7cb66b0..68c0e74 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -693,7 +693,7 @@ static bool scsi_end_request(struct request
> > *req, int error,
> >         struct scsi_device *sdev = cmd->device;
> >         struct request_queue *q = sdev->request_queue;
> > 
> > -       if (blk_update_request(req, error, bytes))
> > +       if (bytes && blk_update_request(req, error, bytes))
> >                 return true;
> 
> Um, I think you mean 
> 
> if (bytes == 0 || blk_update_request())

Actually, even this would be wrong.  We expect scsi_end_request called
with blk_rq_bytes() to complete the request and return false.  If you
do the above, it won't and we'll trigger a BUG lower down (or retry
forever).

James

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

[Jinpu Wang]

On Wed, May 11, 2016 at 5:05 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Tue, 2016-05-10 at 23:43 -0700, James Bottomley wrote:
>> On Wed, 2016-05-11 at 08:21 +0200, Hannes Reinecke wrote:
> [..]
>> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> > index 7cb66b0..68c0e74 100644
>> > --- a/drivers/scsi/scsi_lib.c
>> > +++ b/drivers/scsi/scsi_lib.c
>> > @@ -693,7 +693,7 @@ static bool scsi_end_request(struct request
>> > *req, int error,
>> >         struct scsi_device *sdev = cmd->device;
>> >         struct request_queue *q = sdev->request_queue;
>> >
>> > -       if (blk_update_request(req, error, bytes))
>> > +       if (bytes && blk_update_request(req, error, bytes))
>> >                 return true;
>>
>> Um, I think you mean
>>
>> if (bytes == 0 || blk_update_request())
>
> Actually, even this would be wrong.  We expect scsi_end_request called
> with blk_rq_bytes() to complete the request and return false.  If you
> do the above, it won't and we'll trigger a BUG lower down (or retry
> forever).
>
> James
>
>
You're right, I tried a similar patch:
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1f832c8..d10dabd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -692,7 +692,7 @@ static bool scsi_end_request(struct request *req, int error,
        struct scsi_device *sdev = cmd->device;
        struct request_queue *q = sdev->request_queue;

-       if (blk_update_request(req, error, bytes)) {
+       if ((bytes == 0 && blk_rq_bytes(req) == 0 && error) ||
blk_update_request(req, error, bytes)) {

It lead to BUG below:
[  590.180455] ------------[ cut here ]------------
[  590.180600] kernel BUG at drivers/scsi/scsi_lib.c:931!
[  590.180745] invalid opcode: 0000 [#1]
[  590.180962] Modules linked in: ib_srp scsi_transport_srp rdma_ucm
ib_ucm rdma_cm iw_cm ib_ipoib ib_cm ib_uverbs ib_umad loop mlx4_ib
ib_sa ib_mad ib_core ib_addr kvm_amd kvm acpi_cpufreq mlx4_core
irqbypass tpm_infineon tpm_tis xhci_pci psmouse xhci_hcd
crct10dif_pclmul crc32_pclmul tpm edac_mce_amd crc32c_intel k10temp
processor button fam15h_power edac_core serio_raw evdev i2c_piix4
md_mod sg sd_mod r8169 mlx_compat ahci libahci libata scsi_mod
[  590.183546] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.5-1-pserver #1
[  590.183693] Hardware name: To be filled by O.E.M. To be filled by
O.E.M./M5A97 R2.0, BIOS 2501 04/07/2014
[  590.183974] task: ffffffff81e0e540 ti: ffffffff81e00000 task.ti:
ffffffff81e00000
[  590.184225] RIP: 0010:[<ffffffffa000b248>]  [<ffffffffa000b248>]
scsi_io_completion+0x658/0x660 [scsi_mod]
[  590.184519] RSP: 0018:ffff88023ec03e20  EFLAGS: 00010202
[  590.184664] RAX: 0000000000000001 RBX: 0000000008000002 RCX: 0000000000000006
[  590.184811] RDX: 0000000000000007 RSI: 0000000000000246 RDI: ffff88023ec0ccf0
[  590.184957] RBP: ffff88023ec03e70 R08: ffff8800b5e033d8 R09: 0000000000000000
[  590.185133] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000001
[  590.185280] R13: ffff880234665590 R14: ffff8800b9e26780 R15: 00000000fffffffb
[  590.185427] FS:  00007fed4a8db700(0000) GS:ffff88023ec00000(0000)
knlGS:0000000000000000
[  590.185677] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  590.185822] CR2: 00007fbc1e037000 CR3: 000000022b6cb000 CR4: 00000000000406f0
[  590.185969] Stack:
[  590.186110]  0000004080304680 00000bb800000000 ffff8800b8418810
0000000000000005
[  590.186531]  0000000000290670 ffff8800b9e26780 0000000000000000
ffff880229fb7c28
[  590.186924]  ffff8802294ea000 0000000000000004 ffff88023ec03ea0
ffffffffa0002037
[  590.187316] Call Trace:
[  590.187459]  <IRQ>
[  590.187540]  [<ffffffffa0002037>] scsi_finish_command+0xd7/0x130 [scsi_mod]
[  590.187829]  [<ffffffffa000a570>] scsi_queue_insert+0x140/0x750 [scsi_mod]