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

[James Bottomley <jejb@linux.vnet.ibm.com>]

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