Re: SCSI regression in 4.11

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 2 Mar 2017 14:25:14 +0100
Hannes Reinecke <hare@suse.de> wrote:

> On 03/02/2017 02:40 AM, Stephen Hemminger wrote:
> > On Thu, 2 Mar 2017 01:56:15 +0100
> > Christoph Hellwig <hch@lst.de> wrote:
> >  
> >> On Thu, Mar 02, 2017 at 01:01:35AM +0100, Christoph Hellwig wrote:  
> >>> On Wed, Mar 01, 2017 at 07:54:12AM -0800, Stephen Hemminger wrote:  
> >>>>> 	http://git.infradead.org/users/hch/block.git/commitdiff/148cff67b401e2229c076c0ea418712654be77e4  
> >>>>
> >>>> It appears that is already in the code I am testing in linux-next...  
> >>>
> >>> It's in -next now, but it wasn't at the time you reported the bug.
> >>>
> >>> And it would sortof explain the bug if the INQUIRY data is correct
> >>> in the scatterlist, but we ignore it, given that scsi_probe_lun
> >>> ignores the result based on sense data.
> >>>
> >>> Can you check what happens with the horrible hack below:  
> >>
> >> Strike that - we're checking result later, so this can't be the case.
> >>
> >> Now the other interesting thing is the memset in __scsi_exectute,
> >> which looks very suspicious.  Try the following please:
> >>
> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> >> index 3e32dc954c3c..22f4fb550561 100644
> >> --- a/drivers/scsi/scsi_lib.c
> >> +++ b/drivers/scsi/scsi_lib.c
> >> @@ -253,7 +253,8 @@ static int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
> >>  	 * and prevent security leaks by zeroing out the excess data.
> >>  	 */
> >>  	if (unlikely(rq->resid_len > 0 && rq->resid_len <= bufflen))
> >> -		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
> >> +//		memset(buffer + (bufflen - rq->resid_len), 0, rq->resid_len);
> >> +		printk_ratelimited("%s: got resid %d\n", __func__, rq->resid_len);
> >>
> >>  	if (resid)
> >>  		*resid = rq->resid_len;  
> >
> >
> > Still fails but does print resid on some of the later INQUIRY commands (not the initial one).
> >  
> Can you test what happens if you blank out the storvsc_drv workaround:
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 585e54f..c36f42d 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1060,13 +1060,13 @@ static void storvsc_on_io_completion(struct 
> storvsc_device *stor_device,
>           * We do this so we can distinguish truly fatal failues
>           * (srb status == 0x4) and off-line the device in that case.
>           */
> -
> +#if 0
>          if ((stor_pkt->vm_srb.cdb[0] == INQUIRY) ||
>             (stor_pkt->vm_srb.cdb[0] == MODE_SENSE)) {
>                  vstor_packet->vm_srb.scsi_status = 0;
>                  vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
>          }
> -
> +#endif
> 
>          /* Copy over the status...etc */
>          stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;
> 
> It might thappen that we're fail to interpret the 'Device not present' 
> status correctly (which will happen for non-connected DVDs) causing the 
> SCSI stack to make incorrect decisions later on.
> 
> Cheers,
> 
> Hannes

Makes no difference, that was one of the first things I tried (and just tried again).