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: Thursday, March 29, 2012 4:02 AM
> 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 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
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—