[PATCH RESEND 0000/0002] drivers: scsi: storvsc

[PATCH RESEND 0000/0002] drivers: scsi: storvsc

[K. Y. Srinivasan]

The current Windows hosts only handle a subset of scsi commands
sent from the guest and for commands that are not supported, they are
filtered on the host side a generic failure is returned to the guest
as SRB status. The returned error code does not permit the guest to
figure out if there was an error or if the command was not supported.

Based on the input I got from the community, I have convinced the windows
developers to return an error code that allows the guest to distinguish between
unsupported command and true failures. However, it is not clear when this fix will
be shipped.

This patch set addresses this issue by filtering the ATA_16 command on the guest
(note that currently the host is filtering this command as it is not supported).
I also have a patch here the correctly handles SRB_STATUS_INVALID_REQUEST
error returns - note that the current windows hosts don't return this today.

This is a resend of the patches sent earlier based on suggestions from
Jeff Garzik <jgpobox@gmail.com> and Douglas Gilbert <dgilbert@interlog.com>. 


Regards,

K. Y

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

[K. Y. Srinivasan]

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@gmail.com> and
Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
here.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 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
 
 /*
  * This is the end of Protocol specific defines.
@@ -779,6 +780,17 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
 	}
 
 	/*
+	 * If the host returns with an invalid request, set
+	 * the scsi command result correctly.
+	 */
+	if (vm_srb->srb_status == SRB_STATUS_INVALID_REQUEST) {
+		scmnd->result = ((DRIVER_SENSE << 24) |
+				SAM_STAT_CHECK_CONDITION);
+		scsi_build_sense_buffer(0, scmnd->sense_buffer,
+					ILLEGAL_REQUEST, 0x20, 0);
+	}
+
+	/*
 	 * If there is an error; offline the device since all
 	 * error recovery strategies would have already been
 	 * deployed on the host side.
-- 
1.7.4.1

[PATCH RESEND 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[K. Y. Srinivasan]

The current Windows hosts don't handle the ATA_16 command and furthermore
return a generic error code after filtering the command on the host side.
For now filter the command on the guest side until the host is modified to
properly handle unsupported commands.

I would like to thank Jeff Garzik <jgpobox@gmail.com> and
Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
here.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/scsi/storvsc_drv.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 018c363..66e9bdd 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1233,9 +1233,21 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
 	/*
 	 * smartd sends this command and the host does not handle
 	 * this. So, don't send it.
+	 * The current Windows hosts implement a subset of scsi commands
+	 * and for the commands that are not implemented, the host filters
+	 * them and returns a generic failure SRB status. I have been in
+	 * discussions with the Windows team to return the appropriate SRB
+	 * status code for unsupported scsi commands and while they have
+	 * agreed to implement this, it is not clear when this change will be
+	 * available. Consequently, filtering the command before submitting it
+	 * to the host is a resonable interim solution.
 	 */
 	case SET_WINDOW:
-		scmnd->result = ILLEGAL_REQUEST << 16;
+	case ATA_16:
+		scmnd->result = ((DRIVER_SENSE << 24) |
+				SAM_STAT_CHECK_CONDITION);
+		scsi_build_sense_buffer(0, scmnd->sense_buffer,
+					ILLEGAL_REQUEST, 0x20, 0);
 		allowed = false;
 		break;
 	default:
-- 
1.7.4.1

Re: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly when SRB status is INVALID

[James Bottomley]

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@gmail.com> and
> Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
> here.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  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

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

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, March 19, 2012 12:13 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> scsi@vger.kernel.org
> 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@gmail.com> and
> > Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
> > here.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> >  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. 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. 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.

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.

Regards,

K. Y

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

[James Bottomley]

On Mon, 2012-03-19 at 16:50 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> > Sent: Monday, March 19, 2012 12:13 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> > scsi@vger.kernel.org
> > 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@gmail.com> and
> > > Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
> > > here.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > >  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?

> 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?

James

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

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, March 19, 2012 6:40 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> scsi@vger.kernel.org
> 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@HansenPartnership.com]
> > > Sent: Monday, March 19, 2012 12:13 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> > > scsi@vger.kernel.org
> > > 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@gmail.com> and
> > > > Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct
> approach
> > > > here.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > ---
> > > >  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

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

[James Bottomley]

On Mon, 2012-03-19 at 22:52 +0000, KY Srinivasan wrote:
> > >  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.

Sure, but they're not shipped, right ... it's like the test builds we do
for large companies like IBM and HP to try out certain things before
deciding they don't work.

> > > 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.

OK, so this may be the root cause of the problem.  DID_TARGET_FAILURE
returns FAILED from scsi_decide_disposition().  This wakes up the error
handler to retry the command and, since the command is never going to
work, this ends up offlining the device.  The same thing will happen for
every command with no recovery.

The question now is, what else returns SRB_STATUS_ERROR?  If it's always
for stuff that's unretryable, then the DID_ error is wrong and you
should be returning DID_PASSTHROUGH with an error code and the problem
will be solved.  If we can get SRB_STATUS_ERROR on retryable commands,
then you discriminate at the point of failure, not at the point of input
and return DID_TARGET_FAILURE for the ones that should be retried and
DID_PASSTHROUGH + error for the ones that shouldn't.  This will ensure
the driver is completely backwards compatible and that it will work
if/when windows properly handles the commands.

James

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

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Tuesday, March 20, 2012 4:52 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> scsi@vger.kernel.org
> 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 22:52 +0000, KY Srinivasan wrote:
> > > >  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.
> 
> Sure, but they're not shipped, right ... it's like the test builds we do
> for large companies like IBM and HP to try out certain things before
> deciding they don't work.

They are close to shipping and it is very difficult to get any changes in
presently. Furthermore, this is only on windows8; none of the prior 
versions of windows servers of interest support this. I am starting an effort to
get this change into prior windows servers. Once again, it is not clear when
these changes will be pushed out.

  
> 
> > > > 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.
> 
> OK, so this may be the root cause of the problem.  DID_TARGET_FAILURE
> returns FAILED from scsi_decide_disposition().  This wakes up the error
> handler to retry the command and, since the command is never going to
> work, this ends up offlining the device.  The same thing will happen for
> every command with no recovery.
> 
> The question now is, what else returns SRB_STATUS_ERROR?  If it's always
> for stuff that's unretryable, then the DID_ error is wrong and you
> should be returning DID_PASSTHROUGH with an error code and the problem
> will be solved.  If we can get SRB_STATUS_ERROR on retryable commands,
> then you discriminate at the point of failure, not at the point of input
> and return DID_TARGET_FAILURE for the ones that should be retried and
> DID_PASSTHROUGH + error for the ones that shouldn't.  This will ensure
> the driver is completely backwards compatible and that it will work
> if/when windows properly handles the commands.

James, unfortunately based on the current SRB codes I get back from the
host, I don't know which commands that failed ought to be retried and which
ones should not be; I simply get a single SRB error code for cases where the 
host filtered the unsupported commands as well as the case where the host 
supported the command and something failed in the command execution.
If there is something I can try in this driver to fix this problem, I am more than
happy to try it. If it involves getting changes into  the host (win8, win2k8 etc.),
I am willing to start a conversation with the relevant teams, but I cannot 
obviously determine when such changes will ship. However, I do need
solution for the problem now.

I appreciate your taking the time to help me gravitate towards the 
correct solution here. Given my constraints, let me know what is the
best way forward here.

Regards,

K. Y   

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

[KY Srinivasan]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Tuesday, March 20, 2012 10:42 AM
> To: 'James Bottomley'
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> scsi@vger.kernel.org
> Subject: RE: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly
> when SRB status is INVALID
> 
> 
> 
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> > Sent: Tuesday, March 20, 2012 4:52 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> > scsi@vger.kernel.org
> > 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 22:52 +0000, KY Srinivasan wrote:
> > > > >  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.
> >
> > Sure, but they're not shipped, right ... it's like the test builds we do
> > for large companies like IBM and HP to try out certain things before
> > deciding they don't work.
> 
> They are close to shipping and it is very difficult to get any changes in
> presently. Furthermore, this is only on windows8; none of the prior
> versions of windows servers of interest support this. I am starting an effort to
> get this change into prior windows servers. Once again, it is not clear when
> these changes will be pushed out.
> 
> 
> >
> > > > > 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.
> >
> > OK, so this may be the root cause of the problem.  DID_TARGET_FAILURE
> > returns FAILED from scsi_decide_disposition().  This wakes up the error
> > handler to retry the command and, since the command is never going to
> > work, this ends up offlining the device.  The same thing will happen for
> > every command with no recovery.
> >
> > The question now is, what else returns SRB_STATUS_ERROR?  If it's always
> > for stuff that's unretryable, then the DID_ error is wrong and you
> > should be returning DID_PASSTHROUGH with an error code and the problem
> > will be solved.  If we can get SRB_STATUS_ERROR on retryable commands,
> > then you discriminate at the point of failure, not at the point of input
> > and return DID_TARGET_FAILURE for the ones that should be retried and
> > DID_PASSTHROUGH + error for the ones that shouldn't.  This will ensure
> > the driver is completely backwards compatible and that it will work
> > if/when windows properly handles the commands.
> 
> James, unfortunately based on the current SRB codes I get back from the
> host, I don't know which commands that failed ought to be retried and which
> ones should not be; I simply get a single SRB error code for cases where the
> host filtered the unsupported commands as well as the case where the host
> supported the command and something failed in the command execution.
> If there is something I can try in this driver to fix this problem, I am more than
> happy to try it. If it involves getting changes into  the host (win8, win2k8 etc.),
> I am willing to start a conversation with the relevant teams, but I cannot
> obviously determine when such changes will ship. However, I do need
> solution for the problem now.
> 
> I appreciate your taking the time to help me gravitate towards the
> correct solution here. Given my constraints, let me know what is the
> best way forward here.

Ping.

Regards,

K. Y

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

[James Bottomley]

On Fri, 2012-03-23 at 15:50 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: KY Srinivasan
> > Sent: Tuesday, March 20, 2012 10:42 AM
> > To: 'James Bottomley'
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> > scsi@vger.kernel.org
> > Subject: RE: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly
> > when SRB status is INVALID
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> > > Sent: Tuesday, March 20, 2012 4:52 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> > > scsi@vger.kernel.org
> > > 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 22:52 +0000, KY Srinivasan wrote:
> > > > > >  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.
> > >
> > > Sure, but they're not shipped, right ... it's like the test builds we do
> > > for large companies like IBM and HP to try out certain things before
> > > deciding they don't work.
> > 
> > They are close to shipping and it is very difficult to get any changes in
> > presently. Furthermore, this is only on windows8; none of the prior
> > versions of windows servers of interest support this. I am starting an effort to
> > get this change into prior windows servers. Once again, it is not clear when
> > these changes will be pushed out.
> > 
> > 
> > >
> > > > > > 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.
> > >
> > > OK, so this may be the root cause of the problem.  DID_TARGET_FAILURE
> > > returns FAILED from scsi_decide_disposition().  This wakes up the error
> > > handler to retry the command and, since the command is never going to
> > > work, this ends up offlining the device.  The same thing will happen for
> > > every command with no recovery.
> > >
> > > The question now is, what else returns SRB_STATUS_ERROR?  If it's always
> > > for stuff that's unretryable, then the DID_ error is wrong and you
> > > should be returning DID_PASSTHROUGH with an error code and the problem
> > > will be solved.  If we can get SRB_STATUS_ERROR on retryable commands,
> > > then you discriminate at the point of failure, not at the point of input
> > > and return DID_TARGET_FAILURE for the ones that should be retried and
> > > DID_PASSTHROUGH + error for the ones that shouldn't.  This will ensure
> > > the driver is completely backwards compatible and that it will work
> > > if/when windows properly handles the commands.
> > 
> > James, unfortunately based on the current SRB codes I get back from the
> > host, I don't know which commands that failed ought to be retried and which
> > ones should not be; I simply get a single SRB error code for cases where the
> > host filtered the unsupported commands as well as the case where the host
> > supported the command and something failed in the command execution.
> > If there is something I can try in this driver to fix this problem, I am more than
> > happy to try it. If it involves getting changes into  the host (win8, win2k8 etc.),
> > I am willing to start a conversation with the relevant teams, but I cannot
> > obviously determine when such changes will ship. However, I do need
> > solution for the problem now.
> > 
> > I appreciate your taking the time to help me gravitate towards the
> > correct solution here. Given my constraints, let me know what is the
> > best way forward here.
> 
> Ping.

On what? What don't you understand about the above?

The failure path needs to look like the following metacode

case SRB_whatever

if (retryable command)
	return DID_TARGET_FAILURE
else
	setup sense and error
	return DID_PASSTHROUGH

James

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

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, March 26, 2012 4:16 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> scsi@vger.kernel.org
> Subject: RE: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly
> when SRB status is INVALID
> 
> On Fri, 2012-03-23 at 15:50 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: KY Srinivasan
> > > Sent: Tuesday, March 20, 2012 10:42 AM
> > > To: 'James Bottomley'
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> > > scsi@vger.kernel.org
> > > Subject: RE: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result
> correctly
> > > when SRB status is INVALID
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: James Bottomley
> [mailto:James.Bottomley@HansenPartnership.com]
> > > > Sent: Tuesday, March 20, 2012 4:52 AM
> > > > To: KY Srinivasan
> > > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org;
> linux-
> > > > scsi@vger.kernel.org
> > > > 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 22:52 +0000, KY Srinivasan wrote:
> > > > > > >  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.
> > > >
> > > > Sure, but they're not shipped, right ... it's like the test builds we do
> > > > for large companies like IBM and HP to try out certain things before
> > > > deciding they don't work.
> > >
> > > They are close to shipping and it is very difficult to get any changes in
> > > presently. Furthermore, this is only on windows8; none of the prior
> > > versions of windows servers of interest support this. I am starting an effort to
> > > get this change into prior windows servers. Once again, it is not clear when
> > > these changes will be pushed out.
> > >
> > >
> > > >
> > > > > > > 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.
> > > >
> > > > OK, so this may be the root cause of the problem.  DID_TARGET_FAILURE
> > > > returns FAILED from scsi_decide_disposition().  This wakes up the error
> > > > handler to retry the command and, since the command is never going to
> > > > work, this ends up offlining the device.  The same thing will happen for
> > > > every command with no recovery.
> > > >
> > > > The question now is, what else returns SRB_STATUS_ERROR?  If it's always
> > > > for stuff that's unretryable, then the DID_ error is wrong and you
> > > > should be returning DID_PASSTHROUGH with an error code and the
> problem
> > > > will be solved.  If we can get SRB_STATUS_ERROR on retryable commands,
> > > > then you discriminate at the point of failure, not at the point of input
> > > > and return DID_TARGET_FAILURE for the ones that should be retried and
> > > > DID_PASSTHROUGH + error for the ones that shouldn't.  This will ensure
> > > > the driver is completely backwards compatible and that it will work
> > > > if/when windows properly handles the commands.
> > >
> > > James, unfortunately based on the current SRB codes I get back from the
> > > host, I don't know which commands that failed ought to be retried and which
> > > ones should not be; I simply get a single SRB error code for cases where the
> > > host filtered the unsupported commands as well as the case where the host
> > > supported the command and something failed in the command execution.
> > > If there is something I can try in this driver to fix this problem, I am more than
> > > happy to try it. If it involves getting changes into  the host (win8, win2k8 etc.),
> > > I am willing to start a conversation with the relevant teams, but I cannot
> > > obviously determine when such changes will ship. However, I do need
> > > solution for the problem now.
> > >
> > > I appreciate your taking the time to help me gravitate towards the
> > > correct solution here. Given my constraints, let me know what is the
> > > best way forward here.
> >
> > Ping.
> 
> On what? What don't you understand about the above?
> 
> The failure path needs to look like the following metacode
> 
> case SRB_whatever
> 
> if (retryable command)
> 	return DID_TARGET_FAILURE
> else
> 	setup sense and error
> 	return DID_PASSTHROUGH
> 

Thanks James and I am sorry for taking so much of your time. I suspect the check for
retryable commands is to be based on the sense data that might be available. Assuming 
this is what you had in mind, as I look the  scsi_error.c, scsi_check_sense() appears to
do what I would need here. Would you be willing to take patch that exports this function.
Or, is there a better way to identify commands that would be re-tried.

Regards,

K. Y

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

[James Bottomley]

On Tue, 2012-03-27 at 15:32 +0000, KY Srinivasan wrote:
> > > > James, unfortunately based on the current SRB codes I get back from the
> > > > host, I don't know which commands that failed ought to be retried and which
> > > > ones should not be; I simply get a single SRB error code for cases where the
> > > > host filtered the unsupported commands as well as the case where the host
> > > > supported the command and something failed in the command execution.
> > > > If there is something I can try in this driver to fix this problem, I am more than
> > > > happy to try it. If it involves getting changes into  the host (win8, win2k8 etc.),
> > > > I am willing to start a conversation with the relevant teams, but I cannot
> > > > obviously determine when such changes will ship. However, I do need
> > > > solution for the problem now.
> > > >
> > > > I appreciate your taking the time to help me gravitate towards the
> > > > correct solution here. Given my constraints, let me know what is the
> > > > best way forward here.
> > >
> > > Ping.
> > 
> > On what? What don't you understand about the above?
> > 
> > The failure path needs to look like the following metacode
> > 
> > case SRB_whatever
> > 
> > if (retryable command)
> > 	return DID_TARGET_FAILURE
> > else
> > 	setup sense and error
> > 	return DID_PASSTHROUGH
> > 
> 
> Thanks James and I am sorry for taking so much of your time. I suspect the check for
> retryable commands is to be based on the sense data that might be available. Assuming 
> this is what you had in mind, as I look the  scsi_error.c, scsi_check_sense() appears to
> do what I would need here. Would you be willing to take patch that exports this function.
> Or, is there a better way to identify commands that would be re-tried.

Erm, no, I was thinking of something much more simple.  Right at the
moment if (retryable_command) is if (scmnd->cmnd[0] != ATA_16)

Although I'm sure there are other commands that are not retryable ... I
don't really know since I'm not sure what else your hypervisor will
choke on.  It might be simpler to have a list of known good retryable
commands (READ_ WRITE_ GET_CAPACITY_ INQUIRY etc. i.e. the commands sd
normally uses) and assume anything outside that range isn't retryable
but someone who knows what's going on in the hypervisor needs to decide
this.

The point is that you don't pre-filter.  You send the command and then
try and work out from the generic error and the type of command you sent
what happened.  That way, if the hypervisor ever works properly (or
simply passes commands through to real devices) it will "just work"
rather than failing because of the pre-filter.

We should only use the pre-filter if the effects of the command are
fatal (like we have to a lot in USB because the firmware crashes and
hangs).

James

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

[KY Srinivasan]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Thursday, March 29, 2012 4:02 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> scsi@vger.kernel.org
> Subject: RE: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly
> when SRB status is INVALID
> 
> On Tue, 2012-03-27 at 15:32 +0000, KY Srinivasan wrote:
> > > > > James, unfortunately based on the current SRB codes I get back from the
> > > > > host, I don't know which commands that failed ought to be retried and
> which
> > > > > ones should not be; I simply get a single SRB error code for cases where
> the
> > > > > host filtered the unsupported commands as well as the case where the
> host
> > > > > supported the command and something failed in the command
> execution.
> > > > > If there is something I can try in this driver to fix this problem, I am more
> than
> > > > > happy to try it. If it involves getting changes into  the host (win8, win2k8
> etc.),
> > > > > I am willing to start a conversation with the relevant teams, but I cannot
> > > > > obviously determine when such changes will ship. However, I do need
> > > > > solution for the problem now.
> > > > >
> > > > > I appreciate your taking the time to help me gravitate towards the
> > > > > correct solution here. Given my constraints, let me know what is the
> > > > > best way forward here.
> > > >
> > > > Ping.
> > >
> > > On what? What don't you understand about the above?
> > >
> > > The failure path needs to look like the following metacode
> > >
> > > case SRB_whatever
> > >
> > > if (retryable command)
> > > 	return DID_TARGET_FAILURE
> > > else
> > > 	setup sense and error
> > > 	return DID_PASSTHROUGH
> > >
> >
> > Thanks James and I am sorry for taking so much of your time. I suspect the
> check for
> > retryable commands is to be based on the sense data that might be available.
> Assuming
> > this is what you had in mind, as I look the  scsi_error.c, scsi_check_sense()
> appears to
> > do what I would need here. Would you be willing to take patch that exports this
> function.
> > Or, is there a better way to identify commands that would be re-tried.
> 
> Erm, no, I was thinking of something much more simple.  Right at the
> moment if (retryable_command) is if (scmnd->cmnd[0] != ATA_16)
> 
> Although I'm sure there are other commands that are not retryable ... I
> don't really know since I'm not sure what else your hypervisor will
> choke on.  It might be simpler to have a list of known good retryable
> commands (READ_ WRITE_ GET_CAPACITY_ INQUIRY etc. i.e. the commands sd
> normally uses) and assume anything outside that range isn't retryable
> but someone who knows what's going on in the hypervisor needs to decide
> this.
> 
> The point is that you don't pre-filter.  You send the command and then
> try and work out from the generic error and the type of command you sent
> what happened.  That way, if the hypervisor ever works properly (or
> simply passes commands through to real devices) it will "just work"
> rather than failing because of the pre-filter.
> 
> We should only use the pre-filter if the effects of the command are
> fatal (like we have to a lot in USB because the firmware crashes and
> hangs).

Thanks for the clarification. I will have the patch out shortly.

K. Y

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

[James Bottomley]

On Mon, 2012-03-19 at 16:50 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> > Sent: Monday, March 19, 2012 12:13 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; hch@infradead.org; linux-
> > scsi@vger.kernel.org
> > 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@gmail.com> and
> > > Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
> > > here.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > >  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?

> 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?

James

[PATCH RESEND 0000/0002] drivers: scsi: storvsc

[K. Y. Srinivasan]

The current Windows hosts only handle a subset of scsi commands
sent from the guest and for commands that are not supported, they are
filtered on the host side a generic failure is returned to the guest
as SRB status. The returned error code does not permit the guest to
figure out if there was an error or if the command was not supported.

Based on the input I got from the community, I have convinced the windows
developers to return an error code that allows the guest to distinguish between
unsupported command and true failures. However, it is not clear when this fix will
be shipped.

This patch set addresses this issue by filtering the ATA_16 command on the guest
(note that currently the host is filtering this command as it is not supported).
I also have a patch here the correctly handles SRB_STATUS_INVALID_REQUEST
error returns - note that the current windows hosts don't return this today.

This is a resend of the patches sent earlier based on suggestions from
Jeff Garzik <jgpobox@gmail.com> and Douglas Gilbert <dgilbert@interlog.com>. 


Regards,

K. Y

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

[K. Y. Srinivasan]

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@gmail.com> and
Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
here.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 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..d5448e8 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
 
 /*
  * This is the end of Protocol specific defines.
@@ -779,6 +780,17 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
 	}
 
 	/*
+	 * If the host returns with an invalid request, set
+	 * the scsi command result correctly.
+	 */
+	if (vm_srb->srb_status == SRB_STATUS_INVALID_REQUEST) {
+		scmnd->result = ((DRIVER_SENSE << 24) |
+                                 SAM_STAT_CHECK_CONDITION);
+		scsi_build_sense_buffer(0, scmnd->sense_buffer,
+					ILLEGAL_REQUEST, 0x20, 0);
+	}
+
+	/*
 	 * If there is an error; offline the device since all
 	 * error recovery strategies would have already been
 	 * deployed on the host side.
-- 
1.7.4.1

[PATCH RESEND 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[K. Y. Srinivasan]

The current Windows hosts don't handle the ATA_16 command and furthermore
return a generic error code after filtering the command on the host side.
For now filter the command on the guest side until the host is modified to
properly handle unsupported commands.

I would like to thank Jeff Garzik <jgpobox@gmail.com> and
Douglas Gilbert <dgilbert@interlog.com> for suggesting the correct approach
here.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/scsi/storvsc_drv.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index d5448e8..e9e37dd 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1233,9 +1233,21 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
 	/*
 	 * smartd sends this command and the host does not handle
 	 * this. So, don't send it.
+	 * The current Windows hosts implement a subset of scsi commands
+	 * and for the commands that are not implemented, the host filters
+	 * them and returns a generic failure SRB status. I have been in
+	 * discussions with the Windows team to return the appropriate SRB
+	 * status code for unsupported scsi commands and while they have
+	 * agreed to implement this, it is not clear when this change will be
+	 * available. Consequently, filtering the command before submitting it
+	 * to the host is a resonable interim solution.
 	 */
 	case SET_WINDOW:
-		scmnd->result = ILLEGAL_REQUEST << 16;
+	case ATA_16:
+		scmnd->result = ((DRIVER_SENSE << 24) |
+                                 SAM_STAT_CHECK_CONDITION);
+		scsi_build_sense_buffer(0, scmnd->sense_buffer,
+					ILLEGAL_REQUEST, 0x20, 0);
 		allowed = false;
 		break;
 	default:
-- 
1.7.4.1