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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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