[PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16

[KY Srinivasan]

> -----Original Message-----
> From: hch at infradead.org [mailto:hch at infradead.org]
> Sent: Thursday, July 10, 2014 11:32 PM
> To: James Bottomley
> Cc: KY Srinivasan; linux-kernel at vger.kernel.org; mkp at mkp.net;
> hch at infradead.org; devel at linuxdriverproject.org; apw at canonical.com;
> stable at vger.kernel.org; linux-scsi at vger.kernel.org; ohering at suse.com;
> jasowang at redhat.com
> Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> 
> On Wed, Jul 09, 2014 at 10:27:24PM +0000, James Bottomley wrote:
> > If we fix it at source, why would there be any need to filter?  That's
> > the reason the no_write_same flag was introduced.  If we can find and
> > fix the bug, it can go back into the stable trees as a bug fix, hence
> > nothing should ever emit write_same(10 or 16) and additional driver
> > code is redundant (and counter productive, since if this ever breaks
> > again you're our best canary).
> >
> > This looks like it might be the problem but Martin should confirm (I
> > think the problem comes to us from the RC16 code which unconditionally
> > sets WS16).
> 
> I think the problem is a differnet one.  If we have the logical provisioning
> EVPD it configures what method to use, but if we don't have one we simply
> check for a max unmap blocks field, and if that's not present use WRITE
> SAME.
> 
> The patch checks the no_write_same flag before doing that, for which we
> also have to do the write_same setup before the discard setup in
> sd_revalidate_disk.
> 
> Ky: does hyperv support UNMAP?  If so any idea why it doesn't set the
> maximum unmap block count field in the EVPD?

Windows hosts do support UNMAP and set the field in the EVPD. However, since the host
advertises SPC-2 compliance, Linux does not even query the VPD page.
 
> 
> If we want to enable UNMAP in this case I'd prefer a blacklist entry than
> trying UNMAP despite the device not advertising it.
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ba756b1..fbccfd2 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2614,9 +2614,10 @@ static void sd_read_block_limits(struct scsi_disk
> *sdkp)
> 
>  			if (sdkp->max_unmap_blocks)
>  				sd_config_discard(sdkp, SD_LBP_UNMAP);
> -			else
> +			else if (!sdkp->device->no_write_same)
>  				sd_config_discard(sdkp, SD_LBP_WS16);
> -
> +			else
> +				sd_config_discard(sdkp, SD_LBP_DISABLE);
>  		} else {	/* LBP VPD page tells us what to use */
> 
>  			if (sdkp->lbpu && sdkp->max_unmap_blocks) @@ -
> 2766,6 +2767,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	 */
>  	if (sdkp->media_present) {
>  		sd_read_capacity(sdkp, buffer);
> +		sd_read_write_same(sdkp, buffer);
> 
>  		if (sd_try_extended_inquiry(sdp)) {
>  			sd_read_block_provisioning(sdkp);
> @@ -2776,7 +2778,6 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  		sd_read_write_protect_flag(sdkp, buffer);
>  		sd_read_cache_type(sdkp, buffer);
>  		sd_read_app_tag_own(sdkp, buffer);
> -		sd_read_write_same(sdkp, buffer);
>  	}
> 
>  	sdkp->first_scan = 0;