[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 at HansenPartnership.com]
> Sent: Monday, March 26, 2012 4:16 AM
> To: KY Srinivasan
> Cc: gregkh at linuxfoundation.org; linux-kernel at vger.kernel.org;
> devel at linuxdriverproject.org; ohering at suse.com; hch at infradead.org; linux-
> scsi at 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 at linuxfoundation.org; linux-kernel at vger.kernel.org;
> > > devel at linuxdriverproject.org; ohering at suse.com; hch at infradead.org; linux-
> > > scsi at 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 at HansenPartnership.com]
> > > > Sent: Tuesday, March 20, 2012 4:52 AM
> > > > To: KY Srinivasan
> > > > Cc: gregkh at linuxfoundation.org; linux-kernel at vger.kernel.org;
> > > > devel at linuxdriverproject.org; ohering at suse.com; hch at infradead.org;
> linux-
> > > > scsi at 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