RE: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi resultcorrectly when SRB status is INVALID

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx]
> Sent: Monday, March 19, 2012 6:40 PM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; ohering@xxxxxxxx; hch@xxxxxxxxxxxxx; linux-
> scsi@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly
> when SRB status is INVALID
> 
> On Mon, 2012-03-19 at 16:50 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx]
> > > Sent: Monday, March 19, 2012 12:13 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > devel@xxxxxxxxxxxxxxxxxxxxxx; ohering@xxxxxxxx; hch@xxxxxxxxxxxxx; linux-
> > > scsi@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result
> correctly
> > > when SRB status is INVALID
> > >
> > > On Sun, 2012-03-18 at 17:12 -0700, K. Y. Srinivasan wrote:
> > > > Currently Windows hosts only support a subset of scsi commands and for
> > > commands
> > > > that are not supported, the host returns a generic SRB failure status.
> > > > However, they have agreed to change the return value to indicate that
> > > > the command is not supported. In preparation for that, handle the
> > > > SRB_STATUS_INVALID_REQUEST return value correctly.
> > > >
> > > > I would like to thank Jeff Garzik <jgpobox@xxxxxxxxx> and
> > > > Douglas Gilbert <dgilbert@xxxxxxxxxxxx> for suggesting the correct
> approach
> > > > here.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > > > Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/scsi/storvsc_drv.c |   12 ++++++++++++
> > > >  1 files changed, 12 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > > index 44c7a48..018c363 100644
> > > > --- a/drivers/scsi/storvsc_drv.c
> > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > @@ -202,6 +202,7 @@ enum storvsc_request_type {
> > > >  #define SRB_STATUS_INVALID_LUN	0x20
> > > >  #define SRB_STATUS_SUCCESS	0x01
> > > >  #define SRB_STATUS_ERROR	0x04
> > > > +#define SRB_STATUS_INVALID_REQUEST 0x06
> > >
> > > I don't really think this is the correct approach.  We already have a
> > > SCSI error return for this, which you're now translating in the driver
> > > and hypervisor.  Rather than have a special byte return of
> > > SRB_STATUS_INVALID_REQUEST, why not have the hypervisor do the right
> > > thing and fill in the ILLEGAL_REQUEST sense return.  That way you don't
> > > need a special error code and you don't need to construct the sense
> > > buffer in the driver.  Now HyperV will be correctly set up for both pass
> > > through and emulated responses.  It's surely not much work and you
> > > already process sense data correctly in storvsc_command_completion(), so
> > > you wouldn't need any patches to the driver for this approach.
> >
> > James, the issue here is that currently shipping Windows hosts don't even do
> > what I am handling here.
> 
> Right, I understand that.
> 
> >  Based on the input I got from you and Christoph,
> > I convinced the windows team to at least return the SRB status that indicates
> > an illegal request. I will suggest to them that perhaps they should also set the
> > correct sense code and so I would not need this patch.
> 
> Not also; instead of.  There's no need for an extra SRB status.  Just
> return the standard check condition sense data.
> 
> >  However, keep in mind
> > that there is no current ETA on when Windows will ship with these changes -
> Windows 8
> > may ship with code where they would return an invalid SRB status, but they are
> not
> > setting the sense code, hence this patch. When the Window host does the
> "right thing"
> > I will clean this up, but I don't know when that will be.
> 
> I thought you just said you'd only just asked them if they could
> implemented it, in which case no version of windows currently ships with
> this, correct?

There are some private builds of windows 8 floating around with this change, where
they are returning ILLEGAL_REQUEST SRB status  without any sense data.

> 
> > More importantly, the second patch  in this series where I filter out
> > the ATA_16 command
> > on the guest is really important for us. Without that patch on a range
> > on windows hosts
> > including the current beta version of windows8 where the host is
> > returning a generic
> > error in response to ATA_16 command, we cannot boot many Linux
> > distros. If you
> > prefer, I can drop the first patch and re-submit the second patch for
> > consideration now.
> 
> I'm not sure about that either.  You presumably translate
> SRB_STATUS_ERROR into DID_TARGET_FAILURE.  That should cause the
> termination of the command with prejudice in exactly the same way as an
> ILLEGAL_REQUEST sense code would (minus the useful error information),
> so what's causing the boot failure?

You are right, currently without a proper SRB code, I do a DID_TARGET_FAILURE and
this results in the device being offlined and if the device happens to be the root device,
we obviously cannot boot. I have seen this problem with sles11 sp2 on a win8 box.

Regards,

K. Y
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i