[PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[K. Y. Srinivasan]

Add blist flags to permit the reading of the VPD pages even when
the target may claim SPC-2 compliance. MSFT targets currently
claim SPC-2 compliance while they implement post SPC-2 features.
With this patch we can correctly handle WRITE_SAME_16 issues.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/scsi/storvsc_drv.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 29d0329..15ba695 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -327,6 +327,8 @@ MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
  */
 static int storvsc_timeout = 180;
 
+static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
+
 #define STORVSC_MAX_IO_REQUESTS				200
 
 static void storvsc_on_channel_callback(void *context);
@@ -1449,6 +1451,14 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
 
 	sdevice->no_write_same = 1;
 
+	/*
+	 * Add blist flags to permit the reading of the VPD pages even when
+	 * the target may claim SPC-2 compliance. MSFT targets currently
+	 * claim SPC-2 compliance while they implement post SPC-2 features.
+	 * With this patch we can correctly handle WRITE_SAME_16 issues.
+	 */
+	sdevice->sdev_bflags |= msft_blist_flags;
+
 	return 0;
 }
 
-- 
1.7.4.1

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Sitsofe Wheeler]

On Mon, Jul 21, 2014 at 04:06:01PM -0700, K. Y. Srinivasan wrote:
> Add blist flags to permit the reading of the VPD pages even when
> the target may claim SPC-2 compliance. MSFT targets currently
> claim SPC-2 compliance while they implement post SPC-2 features.
> With this patch we can correctly handle WRITE_SAME_16 issues.

OK I've just seen this as I was about to post a similar patch to get
discard going on Hyper-V. Will your patches handle Hyper-V pass through devices
that support discard? The SSD I have here reports the following in the Linux
VM:

# sg_vpd -p lbpv  /dev/sdc
Logical block provisioning VPD page (SBC):
  Unmap command supported (LBPU): 1
  Write same (16) with unmap bit supported (LBWS): 0
  Write same (10) with unmap bit supported (LBWS10): 0
  Logical block provisioning read zeros (LBPRZ): 0
  Anchored LBAs supported (ANC_SUP): 1
  Threshold exponent: 0
  Descriptor present (DP): 0
  Provisioning type: 0

# sg_readcap -l /dev/sdc
Read Capacity results:
   Protection: prot_en=0, p_type=0, p_i_exponent=0
   Logical block provisioning: lbpme=0, lbprz=0
   Last logical block address=234441647 (0xdf94baf), Number of logical blocks=234441648
   Logical block length=512 bytes
   Logical blocks per physical block exponent=0
   Lowest aligned logical block address=0
Hence:
   Device size: 120034123776 bytes, 114473.5 MiB, 120.03 GB

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Christoph Hellwig]

On Wed, Jul 23, 2014 at 11:04:48AM +0100, Sitsofe Wheeler wrote:
> OK I've just seen this as I was about to post a similar patch to get
> discard going on Hyper-V. Will your patches handle Hyper-V pass through devices
> that support discard? The SSD I have here reports the following in the Linux
> VM:

I didn't know hyperv also supports pass through, but it should work fine.
The only thing I'm worried about is real SCSI-2 devices or crappy USB
devices that don't support EVPD pages and might cause problems.  But
let's ignore that problem for now..

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Sitsofe Wheeler]

On Wed, Jul 23, 2014 at 04:51:28AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 23, 2014 at 11:04:48AM +0100, Sitsofe Wheeler wrote:
> > OK I've just seen this as I was about to post a similar patch to get
> > discard going on Hyper-V. Will your patches handle Hyper-V pass through devices
> > that support discard? The SSD I have here reports the following in the Linux
> > VM:
> 
> I didn't know hyperv also supports pass through, but it should work fine.
> The only thing I'm worried about is real SCSI-2 devices or crappy USB
> devices that don't support EVPD pages and might cause problems.  But
> let's ignore that problem for now..

That's good to know (I was worried the device would not be detected as
supporting discard because it doesn't report lbpme and doesn't declare a
conformance version (see below)). Is there a tree with all these updates
in or do they need to be pieced together from this email thread?

Additionally is it worth quirking sd_try_rc16_first on when
BLIST_TRY_VPD_PAGES is present?

# sg_inq  /dev/sdc
standard INQUIRY:
  PQual=0  Device_type=0  RMB=0  version=0x00  [no conformance claimed]
  [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
  SCCS=0  ACC=0  TPGS=0  3PC=0  Protect=0  [BQue=0]
  EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
  [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
  [SPI: Clocking=0x0  QAS=0  IUS=0]
    length=60 (0x3c)   Peripheral device type: disk
 Vendor identification: ADATA   
 Product identification: SSD S510 120GB
 Product revision level: 5.2.
 Unit serial number: 03205115500300002076

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Christoph Hellwig]

On Wed, Jul 23, 2014 at 01:54:43PM +0100, Sitsofe Wheeler wrote:
> That's good to know (I was worried the device would not be detected as
> supporting discard because it doesn't report lbpme and doesn't declare a
> conformance version (see below)).

Ok, that makes things worse - you might be able to force it through
sysfs, though.

> Is there a tree with all these updates
> in or do they need to be pieced together from this email thread?

I'm picking this up for my scsi-queue tree.  Still need a review
for the patch we're replying to before that one can go in, though.

> Additionally is it worth quirking sd_try_rc16_first on when
> BLIST_TRY_VPD_PAGES is present?

Sounds reasonable to me.

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Sitsofe Wheeler]

On Wed, Jul 23, 2014 at 07:10:14AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 23, 2014 at 01:54:43PM +0100, Sitsofe Wheeler wrote:
> > That's good to know (I was worried the device would not be detected as
> > supporting discard because it doesn't report lbpme and doesn't declare a
> > conformance version (see below)).
> 
> Ok, that makes things worse - you might be able to force it through
> sysfs, though.

I've got a hack that seems to be working for me - see below.

> > Is there a tree with all these updates
> > in or do they need to be pieced together from this email thread?
> 
> I'm picking this up for my scsi-queue tree.  Still need a review
> for the patch we're replying to before that one can go in, though.

OK.

> > Additionally is it worth quirking sd_try_rc16_first on when
> > BLIST_TRY_VPD_PAGES is present?
> 
> Sounds reasonable to me.

OK did you want me to post a set of changes to your patch? I've
basically changed the patches from bypassing the SCSI level check (to
allow scanning of VPD pages) to also try READ_CAPACITY(16) first and
bypass lbpme checks. Now it's more of a case of BLIST_TRY_LBP...

-- 
Sitsofe | http://sucs.org/~sits/

RE: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[KY Srinivasan]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Wednesday, July 23, 2014 7:10 AM
> To: Sitsofe Wheeler
> Cc: KY Srinivasan; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; ohering@suse.com;
> apw@canonical.com; jasowang@redhat.com; jbottomley@parallels.com;
> linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags
> 
> On Wed, Jul 23, 2014 at 01:54:43PM +0100, Sitsofe Wheeler wrote:
> > That's good to know (I was worried the device would not be detected as
> > supporting discard because it doesn't report lbpme and doesn't declare
> > a conformance version (see below)).
> 
> Ok, that makes things worse - you might be able to force it through sysfs,
> though.
> 
> > Is there a tree with all these updates in or do they need to be pieced
> > together from this email thread?
> 
> I'm picking this up for my scsi-queue tree.  Still need a review for the patch
> we're replying to before that one can go in, though.

Hannes, could you please review this patch. Christoph, other than the review of this patch,
Is there something you want done before these can be committed.

Regards,

K. Y
> 
> > Additionally is it worth quirking sd_try_rc16_first on when
> > BLIST_TRY_VPD_PAGES is present?
> 
> Sounds reasonable to me.

RE: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[KY Srinivasan]

> -----Original Message-----
> From: Sitsofe Wheeler [mailto:sitsofe@gmail.com]
> Sent: Wednesday, July 23, 2014 3:05 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> jasowang@redhat.com; jbottomley@parallels.com; hch@infradead.org;
> linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags
> 
> On Mon, Jul 21, 2014 at 04:06:01PM -0700, K. Y. Srinivasan wrote:
> > Add blist flags to permit the reading of the VPD pages even when the
> > target may claim SPC-2 compliance. MSFT targets currently claim SPC-2
> > compliance while they implement post SPC-2 features.
> > With this patch we can correctly handle WRITE_SAME_16 issues.
> 
> OK I've just seen this as I was about to post a similar patch to get discard
> going on Hyper-V. Will your patches handle Hyper-V pass through devices
> that support discard? The SSD I have here reports the following in the Linux

It should. Hyper-V does support pass through devices.

K. Y
> VM:
> 
> # sg_vpd -p lbpv  /dev/sdc
> Logical block provisioning VPD page (SBC):
>   Unmap command supported (LBPU): 1
>   Write same (16) with unmap bit supported (LBWS): 0
>   Write same (10) with unmap bit supported (LBWS10): 0
>   Logical block provisioning read zeros (LBPRZ): 0
>   Anchored LBAs supported (ANC_SUP): 1
>   Threshold exponent: 0
>   Descriptor present (DP): 0
>   Provisioning type: 0
> 
> # sg_readcap -l /dev/sdc
> Read Capacity results:
>    Protection: prot_en=0, p_type=0, p_i_exponent=0
>    Logical block provisioning: lbpme=0, lbprz=0
>    Last logical block address=234441647 (0xdf94baf), Number of logical
> blocks=234441648
>    Logical block length=512 bytes
>    Logical blocks per physical block exponent=0
>    Lowest aligned logical block address=0
> Hence:
>    Device size: 120034123776 bytes, 114473.5 MiB, 120.03 GB
> 
> --
> Sitsofe | http://sucs.org/~sits/

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Sitsofe Wheeler]

On Mon, Jul 21, 2014 at 04:06:01PM -0700, K. Y. Srinivasan wrote:
> Add blist flags to permit the reading of the VPD pages even when
> the target may claim SPC-2 compliance. MSFT targets currently
> claim SPC-2 compliance while they implement post SPC-2 features.
> With this patch we can correctly handle WRITE_SAME_16 issues.
> 
>  static void storvsc_on_channel_callback(void *context);
> @@ -1449,6 +1451,14 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
>  
>  	sdevice->no_write_same = 1;
>  
> +	/*
> +	 * Add blist flags to permit the reading of the VPD pages even when
> +	 * the target may claim SPC-2 compliance. MSFT targets currently
> +	 * claim SPC-2 compliance while they implement post SPC-2 features.
> +	 * With this patch we can correctly handle WRITE_SAME_16 issues.
> +	 */
> +	sdevice->sdev_bflags |= msft_blist_flags;
> +

I'm not sure this alone will work - won't sdev_bflags/bflags have
already been built at this point? If the setting of this has to stay in
this function why don't you (also) set sdevice->try_vpd_pages in a
similar way to no_write_same?

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Christoph Hellwig]

On Wed, Jul 23, 2014 at 03:10:28PM +0100, Sitsofe Wheeler wrote:
> I'm not sure this alone will work - won't sdev_bflags/bflags have
> already been built at this point?

They've been built up, but we can still or new values into it.  It looks
fine to me from review, but if you can test it on an actualy hypverv
setup that would be valueable feedback.

> If the setting of this has to stay in
> this function why don't you (also) set sdevice->try_vpd_pages in a
> similar way to no_write_same?

We could probably do that.  I don't actually like this whole split
between the blacklist flags, and the bitfields.  I'll see if we can
just get rid of those bitfields an always just use the blacklist
flags directly, now that we have precedence of setting them in drivers
and not just the table keyed by the inquiry data.  But that's a longer
term project, this late in the 3.17 cycle I'd just like to merge
something that gets discards on hyperv to work.

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Sitsofe Wheeler]

[-- Attachment #1: Type: text/plain, Size: 6973 bytes --]

On Wed, Jul 23, 2014 at 07:15:58AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 23, 2014 at 03:10:28PM +0100, Sitsofe Wheeler wrote:
> > I'm not sure this alone will work - won't sdev_bflags/bflags have
> > already been built at this point?
> 
> They've been built up, but we can still or new values into it.  It looks
> fine to me from review, but if you can test it on an actualy hypverv
> setup that would be valueable feedback.

The previous patches didn't work for me with a Windows 2012 R2 host with a
3.16.0-rc6.x86_64-00076-g2f7d2ec-dirty guest. After applying
https://patchwork.kernel.org/patch/4541201 (which needed a small fixup) and
https://patchwork.kernel.org/patch/4598601 and the attached debugging output
patch here's the result I got:

hv_vmbus: registering driver hv_storvsc
scsi0 : storvsc_host_t
scsi 0:0:0:0: scsi_get_device_flags_keyed: key: 3
scsi 0:0:0:0: scsi_get_device_flags_keyed: Post SCSI_DEVINFO_GLOBAL
scsi 0:0:0:0: scsi_get_device_flags_keyed: No sdev_bflags
scsi 0:0:0:0: sdev->scsi_level: 5
scsi 0:0:0:0: Direct-Access     Msft     Virtual Disk     1.0  PQ: 0 ANSI: 4
scsi 0:0:0:0: scsi_add_lun: Have BLIST_TRY_VPD_PAGES? No
scsi 0:0:0:0: storvsc_device_configure: Added BLIST_TRY_VPD_PAGES
scsi1 : storvsc_host_t
sd 0:0:0:0: Attached scsi generic sg0 type 0
scsi 1:0:0:0: scsi_get_device_flags_keyed: key: 3
scsi 1:0:0:0: scsi_get_device_flags_keyed: Post SCSI_DEVINFO_GLOBAL
scsi 1:0:0:0: scsi_get_device_flags_keyed: No sdev_bflags
scsi 1:0:0:0: sdev->scsi_level: 5
scsi 1:0:0:0: Direct-Access     Msft     Virtual Disk     1.0  PQ: 0 ANSI: 4
scsi 1:0:0:0: scsi_add_lun: Have BLIST_TRY_VPD_PAGES? No
scsi 1:0:0:0: storvsc_device_configure: Added BLIST_TRY_VPD_PAGES
scsi 1:0:0:1: scsi_get_device_flags_keyed: key: 5
scsi 1:0:0:1: scsi_get_device_flags_keyed: Post SCSI_DEVINFO_GLOBAL
scsi 1:0:0:1: scsi_get_device_flags_keyed: No sdev_bflags
scsi 1:0:0:1: scsi_get_device_flags_keyed: key: 5
scsi 1:0:0:1: scsi_get_device_flags_keyed: Post SCSI_DEVINFO_GLOBAL
scsi 1:0:0:1: scsi_get_device_flags_keyed: No sdev_bflags
scsi 1:0:0:1: sdev->scsi_level: 0
scsi 1:0:0:1: Direct-Access     ADATA    SSD S510 120GB   5.2. PQ: 0 ANSI: 0
scsi 1:0:0:1: scsi_add_lun: Have BLIST_TRY_VPD_PAGES? No
scsi 1:0:0:1: storvsc_device_configure: Added BLIST_TRY_VPD_PAGES
scsi 1:0:0:3: scsi_get_device_flags_keyed: key: 0
scsi 1:0:0:3: scsi_get_device_flags_keyed: Post SCSI_DEVINFO_GLOBAL
scsi 1:0:0:3: scsi_get_device_flags_keyed: No sdev_bflags
scsi 1:0:0:3: sdev->scsi_level: 5
scsi 1:0:0:3: Direct-Access     Msft     Virtual Disk     1.0  PQ: 0 ANSI: 4
scsi 1:0:0:3: scsi_add_lun: Have BLIST_TRY_VPD_PAGES? No
scsi 1:0:0:3: storvsc_device_configure: Added BLIST_TRY_VPD_PAGES
sd 1:0:0:0: sd_try_rc16_first: sdp->scsi_level: 5
sd 1:0:0:0: [sdb] 2097152 512-byte logical blocks: (1.07 GB/1.00 GiB)
sd 1:0:0:0: [sdb] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:0: [sdb] sd_revalidate_disk: Skipped extended inquiries
sd 1:0:0:0: [sdb] Write Protect is off
sd 1:0:0:0: [sdb] Mode Sense: 0f 00 00 00
sd 1:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 0:0:0:0: sd_try_rc16_first: sdp->scsi_level: 5
ata_piix 0000:00:07.1: version 2.13
sd 1:0:0:0: sd_try_rc16_first: sdp->scsi_level: 5
sd 1:0:0:0: [sdb] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:0: [sdb] sd_revalidate_disk: Skipped extended inquiries
 sdb: unknown partition table
ata_piix 0000:00:07.1: Hyper-V Virtual Machine detected, ATA device ignore set
sd 1:0:0:0: sd_try_rc16_first: sdp->scsi_level: 5
sd 1:0:0:0: [sdb] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:0: [sdb] sd_revalidate_disk: Skipped extended inquiries
sd 1:0:0:0: [sdb] Attached SCSI disk
sd 1:0:0:0: Attached scsi generic sg1 type 0
sd 0:0:0:0: [sda] 8388608 512-byte logical blocks: (4.29 GB/4.00 GiB)
scsi2 : ata_piix
scsi3 : ata_piix
ata1: PATA max UDMA/33 cmd 0x1f0 ctl 0x3f6 bmdma 0xffa0 irq 14
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xffa8 irq 15
sd 1:0:0:1: Attached scsi generic sg2 type 0
libphy: Fixed MDIO Bus: probed
hv_vmbus: registering driver hv_netvsc
sd 1:0:0:3: Attached scsi generic sg3 type 0
hv_netvsc: hv_netvsc channel opened successfully
sd 0:0:0:0: [sda] sd_revalidate_disk: Extended inquiry check...
sd 0:0:0:0: [sda] sd_revalidate_disk: Skipped extended inquiries
sd 1:0:0:1: sd_try_rc16_first: sdp->scsi_level: 0
sd 1:0:0:3: sd_try_rc16_first: sdp->scsi_level: 5
sd 1:0:0:1: [sdc] 234441648 512-byte logical blocks: (120 GB/111 GiB)
sd 1:0:0:1: [sdc] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:1: [sdc] sd_revalidate_disk: Skipped extended inquiries
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 0f 00 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 0:0:0:0: sd_try_rc16_first: sdp->scsi_level: 5
sd 0:0:0:0: [sda] sd_revalidate_disk: Extended inquiry check...
sd 0:0:0:0: [sda] sd_revalidate_disk: Skipped extended inquiries
 sda: sda1
sd 0:0:0:0: sd_try_rc16_first: sdp->scsi_level: 5
sd 0:0:0:0: [sda] sd_revalidate_disk: Extended inquiry check...
sd 0:0:0:0: [sda] sd_revalidate_disk: Skipped extended inquiries
sd 0:0:0:0: [sda] Attached SCSI disk
sd 1:0:0:1: [sdc] Write Protect is off
sd 1:0:0:3: [sdd] 199229440 512-byte logical blocks: (102 GB/95.0 GiB)
sd 1:0:0:3: [sdd] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:3: [sdd] sd_revalidate_disk: Skipped extended inquiries
sd 1:0:0:3: [sdd] Write Protect is off
sd 1:0:0:3: [sdd] Mode Sense: 0f 00 00 00
sd 1:0:0:3: [sdd] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 1:0:0:1: [sdc] Mode Sense: 0f 00 00 00
sd 1:0:0:3: sd_try_rc16_first: sdp->scsi_level: 5
sd 1:0:0:1: [sdc] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 1:0:0:3: [sdd] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:3: [sdd] sd_revalidate_disk: Skipped extended inquiries
 sdd: unknown partition table
sd 1:0:0:3: sd_try_rc16_first: sdp->scsi_level: 5
sd 1:0:0:3: [sdd] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:3: [sdd] sd_revalidate_disk: Skipped extended inquiries
sd 1:0:0:3: [sdd] Attached SCSI disk
sd 1:0:0:1: sd_try_rc16_first: sdp->scsi_level: 0
sd 1:0:0:1: [sdc] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:1: [sdc] sd_revalidate_disk: Skipped extended inquiries
 sdc: sdc1
sd 1:0:0:1: sd_try_rc16_first: sdp->scsi_level: 0
sd 1:0:0:1: [sdc] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:1: [sdc] sd_revalidate_disk: Skipped extended inquiries
ata1.00: host indicates ignore ATA devices, ignored
sd 1:0:0:1: [sdc] Attached SCSI disk
tsc: Refined TSC clocksource calibration: 3200.069 MHz

sda is a 4 GByte VHDX attached to Hyper-V's IDE interface.
sdb is a 1 GByte VHDX attached to Hyper-V's SCSI interface.
sdc is a 111.8 GByte SSD being passed through Hyper-V's SCSI interface.
sdd is a 95 GByte VHDX being passed through Hyper-V's SCSI interface.

-- 
Sitsofe | http://sucs.org/~sits/

[-- Attachment #2: 0001-Add-debugging-output-to-SCSI-disk-initalisation-to-t.patch --]
[-- Type: text/x-diff, Size: 9362 bytes --]

>From abc3c29ce756f8be6aa8a945a8c81b29d396dbb6 Mon Sep 17 00:00:00 2001
From: Sitsofe Wheeler <sitsofe@yahoo.com>
Date: Wed, 23 Jul 2014 15:41:31 +0000
Subject: [PATCH] Add debugging output to SCSI disk initalisation to trace LBP
 testing.

---
 drivers/scsi/scsi.c         |  5 +++++
 drivers/scsi/scsi_devinfo.c |  7 +++++++
 drivers/scsi/scsi_scan.c    | 10 ++++++++--
 drivers/scsi/sd.c           | 28 ++++++++++++++++++++++++++++
 drivers/scsi/storvsc_drv.c  |  1 +
 5 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 88d46fe..6a7a854 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -992,11 +992,13 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 	if (sdev->skip_vpd_pages)
 		goto fail;
 
+	printk(KERN_NOTICE "Don't skip\n");
 	/* Ask for all the pages supported by this device */
 	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
 	if (result < 4)
 		goto fail;
 
+	printk(KERN_NOTICE "Got all pages\n");
 	/* If the user actually wanted this page, we can skip the rest */
 	if (page == 0)
 		return 0;
@@ -1008,13 +1010,16 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 	if (i < result && i >= buf_len)
 		/* ran off the end of the buffer, give us benefit of doubt */
 		goto found;
+	printk(KERN_NOTICE "Couldn't find page\n");
 	/* The device claims it doesn't support the requested page */
 	goto fail;
 
  found:
+	printk(KERN_NOTICE "Read vpd page\n");
 	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
 	if (result < 0)
 		goto fail;
+	printk(KERN_NOTICE "Read vpd page success\n");
 
 	return 0;
 
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index f969aca..cbde61f 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -619,14 +619,21 @@ int scsi_get_device_flags_keyed(struct scsi_device *sdev,
 				return devinfo->flags;
 		}
 	}
+	sdev_printk(KERN_NOTICE, sdev,
+		    "scsi_get_device_flags_keyed: key: %d\n",
+		    sdev->scsi_level);
 	/* nothing found, return nothing */
 	if (key != SCSI_DEVINFO_GLOBAL)
 		return 0;
 
+	sdev_printk(KERN_NOTICE, sdev,
+		    "scsi_get_device_flags_keyed: Post SCSI_DEVINFO_GLOBAL\n");
 	/* except for the global list, where we have an exception */
 	if (sdev->sdev_bflags)
 		return sdev->sdev_bflags;
 
+	sdev_printk(KERN_NOTICE, sdev,
+		    "scsi_get_device_flags_keyed: No sdev_bflags\n");
 	return scsi_default_dev_flags;
 }
 EXPORT_SYMBOL(scsi_get_device_flags_keyed);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 492cd70..7de78b8 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -734,6 +734,7 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
 		sdev->scsi_level++;
 	sdev->sdev_target->scsi_level = sdev->scsi_level;
+	sdev_printk(KERN_NOTICE, sdev, "sdev->scsi_level: %d\n", sdev->scsi_level);
 
 	return 0;
 }
@@ -950,9 +951,14 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 
 	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
 
-	if (*bflags & BLIST_TRY_VPD_PAGES)
+	sdev_printk(KERN_NOTICE, sdev,
+		    "scsi_add_lun: Have BLIST_TRY_VPD_PAGES? %s",
+		    ((*bflags & BLIST_TRY_VPD_PAGES) ? "Yes" : "No"));
+	if (*bflags & BLIST_TRY_VPD_PAGES) {
 		sdev->try_vpd_pages = 1;
-	else if (*bflags & BLIST_SKIP_VPD_PAGES)
+		sdev_printk(KERN_NOTICE, sdev,
+			    "scsi_add_lun: Set try_vpd_pages");
+	} else if (*bflags & BLIST_SKIP_VPD_PAGES)
 		sdev->skip_vpd_pages = 1;
 
 	transport_configure_device(&sdev->sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index ed2e99e..b905e1e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -636,6 +636,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 
 	sdkp->provisioning_mode = mode;
 
+	//dump_stack();
+	sd_printk(KERN_NOTICE, sdkp, "Discard mode: %u\n", mode);
 	switch (mode) {
 
 	case SD_LBP_DISABLE:
@@ -1947,6 +1949,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	unsigned long long lba;
 	unsigned sector_size;
 
+	sd_printk(KERN_NOTICE, sdkp, "Entered read_capacity_16\n");
 	if (sdp->no_read_capacity_16)
 		return -EINVAL;
 
@@ -1985,6 +1988,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		retries--;
 
 	} while (the_result && retries);
+	sd_printk(KERN_ERR, sdkp, "Past illegal req\n");
 
 	if (the_result) {
 		sd_printk(KERN_NOTICE, sdkp, "READ CAPACITY(16) failed\n");
@@ -1995,10 +1999,13 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	sector_size = get_unaligned_be32(&buffer[8]);
 	lba = get_unaligned_be64(&buffer[0]);
 
+	sd_printk(KERN_ERR, sdkp, "Protection check\n");
 	if (sd_read_protection_type(sdkp, buffer) < 0) {
+		sd_printk(KERN_ERR, sdkp, "Protection %d\n", sd_read_protection_type(sdkp, buffer));
 		sdkp->capacity = 0;
 		return -ENODEV;
 	}
+	sd_printk(KERN_ERR, sdkp, "Got past protection check\n");
 
 	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
@@ -2018,8 +2025,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		sd_printk(KERN_NOTICE, sdkp,
 			  "physical block alignment offset: %u\n", alignment);
 
+	sd_printk(KERN_NOTICE, sdkp, "Checking LBPME\n");
 	if (buffer[14] & 0x80) { /* LBPME */
 		sdkp->lbpme = 1;
+		sd_printk(KERN_NOTICE, sdkp, "LBPME OK!\n");
 
 		if (buffer[14] & 0x40) /* LBPRZ */
 			sdkp->lbprz = 1;
@@ -2109,6 +2118,9 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
 		return 1;
 	if (scsi_device_protection(sdp))
 		return 1;
+	sdev_printk(KERN_NOTICE, sdp,
+		    "sd_try_rc16_first: sdp->scsi_level: %d\n",
+		    sdp->scsi_level);
 	return 0;
 }
 
@@ -2542,24 +2554,29 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 	const int vpd_len = 64;
 	unsigned char *buffer = kmalloc(vpd_len, GFP_KERNEL);
 
+	sd_printk(KERN_NOTICE, sdkp, "Entered block limits\n");
 	if (!buffer ||
 	    /* Block Limits VPD */
 	    scsi_get_vpd_page(sdkp->device, 0xb0, buffer, vpd_len))
 		goto out;
+	sd_printk(KERN_NOTICE, sdkp, "Started block limits\n");
 
 	blk_queue_io_min(sdkp->disk->queue,
 			 get_unaligned_be16(&buffer[6]) * sector_sz);
 	blk_queue_io_opt(sdkp->disk->queue,
 			 get_unaligned_be32(&buffer[12]) * sector_sz);
 
+	sd_printk(KERN_NOTICE, sdkp, "0x3c...\n");
 	if (buffer[3] == 0x3c) {
 		unsigned int lba_count, desc_count;
 
 		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
 
+		sd_printk(KERN_NOTICE, sdkp, "Testing lbpme...\n");
 		if (!sdkp->lbpme)
 			goto out;
 
+		sd_printk(KERN_NOTICE, sdkp, "...lbpme test done\n");
 		lba_count = get_unaligned_be32(&buffer[20]);
 		desc_count = get_unaligned_be32(&buffer[24]);
 
@@ -2574,6 +2591,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 		if (!sdkp->lbpvpd) { /* LBP VPD page not provided */
 
+			sd_printk(KERN_NOTICE, sdkp, "Entering discard switch with NO LBP VPD\n");
 			if (sdkp->max_unmap_blocks)
 				sd_config_discard(sdkp, SD_LBP_UNMAP);
 			else
@@ -2581,6 +2599,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 		} else {	/* LBP VPD page tells us what to use */
 
+			sd_printk(KERN_NOTICE, sdkp, "Entering discard switch via LBP VPD\n");
 			if (sdkp->lbpu && sdkp->max_unmap_blocks)
 				sd_config_discard(sdkp, SD_LBP_UNMAP);
 			else if (sdkp->lbpws)
@@ -2631,14 +2650,19 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 	unsigned char *buffer;
 	const int vpd_len = 8;
 
+	sd_printk(KERN_NOTICE, sdkp,
+		  "sd_read_block_provisioning: Entered, lbmpe: %u\n",
+		  sdkp->lbpme);
 	if (sdkp->lbpme == 0)
 		return;
+	sd_printk(KERN_NOTICE, sdkp, "sd_read_block_provisioning: Passed lbmpe test\n");
 
 	buffer = kmalloc(vpd_len, GFP_KERNEL);
 
 	if (!buffer || scsi_get_vpd_page(sdkp->device, 0xb2, buffer, vpd_len))
 		goto out;
 
+	sd_printk(KERN_NOTICE, sdkp, "sd_read_block_provisioning: Setting block provisioning\n");
 	sdkp->lbpvpd	= 1;
 	sdkp->lbpu	= (buffer[5] >> 7) & 1;	/* UNMAP */
 	sdkp->lbpws	= (buffer[5] >> 6) & 1;	/* WRITE SAME(16) with UNMAP */
@@ -2734,10 +2758,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	if (sdkp->media_present) {
 		sd_read_capacity(sdkp, buffer);
 
+		sd_printk(KERN_NOTICE, sdkp, "sd_revalidate_disk: Extended inquiry check...\n");
 		if (sd_try_extended_inquiry(sdp)) {
+			sd_printk(KERN_NOTICE, sdkp, "sd_revalidate_disk: Performing extended inquiries\n");
 			sd_read_block_provisioning(sdkp);
 			sd_read_block_limits(sdkp);
 			sd_read_block_characteristics(sdkp);
+		} else {
+			sd_printk(KERN_NOTICE, sdkp, "sd_revalidate_disk: Skipped extended inquiries\n");
 		}
 
 		sd_read_write_protect_flag(sdkp, buffer);
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 5ad2810..5f733c7 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1450,6 +1450,7 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
 	 * With this patch we can correctly handle WRITE_SAME_16 issues.
 	 */
 	sdevice->sdev_bflags |= msft_blist_flags;
+	sdev_printk(KERN_NOTICE, sdevice, "storvsc_device_configure: Added BLIST_TRY_VPD_PAGES\n");
 
 	return 0;
 }
-- 
1.9.3

[PATCH 0/3] Enable discard on Hyper-V

[Sitsofe Wheeler]

On Wed, Jul 23, 2014 at 09:13:41PM +0100, Sitsofe Wheeler wrote:
> On Wed, Jul 23, 2014 at 07:15:58AM -0700, Christoph Hellwig wrote:
> > On Wed, Jul 23, 2014 at 03:10:28PM +0100, Sitsofe Wheeler wrote:
> > > I'm not sure this alone will work - won't sdev_bflags/bflags have
> > > already been built at this point?
> > 
> > They've been built up, but we can still or new values into it.  It looks
> > fine to me from review, but if you can test it on an actualy hypverv
> > setup that would be valueable feedback.
> 
> The previous patches didn't work for me with a Windows 2012 R2 host with a
> 3.16.0-rc6.x86_64-00076-g2f7d2ec-dirty guest. After applying
> 
> > term project, this late in the 3.17 cycle I'd just like to merge
> > something that gets discards on hyperv to work.

OK how about the following patches:

Sitsofe Wheeler (3):
  [SCSI] Add quirk for forcing logical block provisioning tests
  [SCSI] storvsc: Add Hyper-V logical block provisioning quirk
  [SCSI] Make LBP quirk skip lbpme checks

 drivers/scsi/scsi_scan.c    |  4 +++-
 drivers/scsi/sd.c           | 12 ++++++++++--
 drivers/scsi/storvsc_drv.c  |  6 ++++++
 include/scsi/scsi_device.h  |  1 +
 include/scsi/scsi_devinfo.h |  2 ++
 5 files changed, 22 insertions(+), 3 deletions(-)

-- 
1.9.3

-- 
Sitsofe | http://sucs.org/~sits/

[PATCH 1/3] [SCSI] Add quirk for forcing logical block provisioning tests

[Sitsofe Wheeler]

Despite supporting modern SCSI features (such an UNMAP) some storage
devices continue to claim conformance to an older version of the SPC
spec for compatibility with legacy operating systems.

Linux by default will not attempt to read VPD pages on devices that
claim SPC-2 or older.

Introduce a blacklist flag that allows the forcing of the paths leading
to logical block provisioning tests.

See https://lkml.org/lkml/2014/7/13/59 for the previous version.

Reported-by: K. Y. Srinivasan <kys@microsoft.com>
Original-patch-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com>
---
 drivers/scsi/scsi_scan.c    |  4 +++-
 drivers/scsi/sd.c           |  8 ++++++++
 drivers/scsi/storvsc_drv.c  | 10 ++++++++++
 include/scsi/scsi_device.h  |  1 +
 include/scsi/scsi_devinfo.h |  2 ++
 5 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e02b3aa..02ca1c2 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -950,7 +950,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 
 	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
 
-	if (*bflags & BLIST_SKIP_VPD_PAGES)
+	if (*bflags & BLIST_TRY_LBP)
+		sdev->try_lbp = 1;
+	else if (*bflags & BLIST_SKIP_VPD_PAGES)
 		sdev->skip_vpd_pages = 1;
 
 	transport_configure_device(&sdev->sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6825eda..8249e51 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2109,6 +2109,8 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
 		return 1;
 	if (scsi_device_protection(sdp))
 		return 1;
+	if (sdp->try_lbp)
+		return 1;
 	return 0;
 }
 
@@ -2682,6 +2684,12 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
 static int sd_try_extended_inquiry(struct scsi_device *sdp)
 {
 	/*
+	 * Attempt VPD inquiry if the device blacklist explicitly calls
+	 * for it.
+	 */
+	if (sdp->try_lbp)
+		return 1;
+	/*
 	 * Although VPD inquiries can go to SCSI-2 type devices,
 	 * some USB ones crash on receiving them, and the pages
 	 * we currently ask for are for SPC-3 and beyond
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 9969fa1..5ad2810 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -326,6 +326,8 @@ MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
  */
 static int storvsc_timeout = 180;
 
+static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
+
 #define STORVSC_MAX_IO_REQUESTS				200
 
 static void storvsc_on_channel_callback(void *context);
@@ -1441,6 +1443,14 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
 
 	sdevice->no_write_same = 1;
 
+	/*
+	 * Add blist flags to permit the reading of the VPD pages even when
+	 * the target may claim SPC-2 compliance. MSFT targets currently
+	 * claim SPC-2 compliance while they implement post SPC-2 features.
+	 * With this patch we can correctly handle WRITE_SAME_16 issues.
+	 */
+	sdevice->sdev_bflags |= msft_blist_flags;
+
 	return 0;
 }
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 27ab310..0a5c6fa 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -155,6 +155,7 @@ struct scsi_device {
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
 	unsigned skip_ms_page_3f:1;	/* do not use MODE SENSE page 0x3f */
 	unsigned skip_vpd_pages:1;	/* do not read VPD pages */
+	unsigned try_lbp:1;		/* Try LBP tests */
 	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 447d2d7..9d15d78 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -32,4 +32,6 @@
 #define BLIST_ATTACH_PQ3	0x1000000 /* Scan: Attach to PQ3 devices */
 #define BLIST_NO_DIF		0x2000000 /* Disable T10 PI (DIF) */
 #define BLIST_SKIP_VPD_PAGES	0x4000000 /* Ignore SBC-3 VPD pages */
+#define BLIST_TRY_LBP		0x10000000 /* Check Logical Block Provisioning
+					      even on < SPC-3 devices */
 #endif
-- 
1.9.3

[PATCH 2/3] [SCSI] storvsc: Add Hyper-V logical block provisioning tests

[Sitsofe Wheeler]

Microsoft Hyper-V targets currently only claim SPC-2 compliance / no
compliance indicated even though they implement post SPC-2 features
which means those features are not tested for. Add a blacklist flag to
Hyper-V devices that forces said testing.

See https://lkml.org/lkml/2014/7/21/627 for the previous version of this
patch and https://lkml.org/lkml/2014/7/23/615 for example devices.

Original-patch-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com>
---
 drivers/scsi/storvsc_drv.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 5ad2810..88b7173 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -326,8 +326,6 @@ MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
  */
 static int storvsc_timeout = 180;
 
-static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
-
 #define STORVSC_MAX_IO_REQUESTS				200
 
 static void storvsc_on_channel_callback(void *context);
@@ -1444,12 +1442,10 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
 	sdevice->no_write_same = 1;
 
 	/*
-	 * Add blist flags to permit the reading of the VPD pages even when
-	 * the target may claim SPC-2 compliance. MSFT targets currently
-	 * claim SPC-2 compliance while they implement post SPC-2 features.
-	 * With this patch we can correctly handle WRITE_SAME_16 issues.
+	 * Forcefully enable logical block provisioning testing.
 	 */
-	sdevice->sdev_bflags |= msft_blist_flags;
+	sdevice->sdev_bflags |= BLIST_TRY_LBP;
+	sdevice->try_lbp = 1;
 
 	return 0;
 }
-- 
1.9.3

Re: [PATCH 2/3] [SCSI] storvsc: Add Hyper-V logical block provisioning tests

[James Bottomley]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2528 bytes --]

On Thu, 2014-07-24 at 08:56 +0100, Sitsofe Wheeler wrote:
> Microsoft Hyper-V targets currently only claim SPC-2 compliance / no
> compliance indicated even though they implement post SPC-2 features
> which means those features are not tested for. Add a blacklist flag to
> Hyper-V devices that forces said testing.

This description is misleading: you don't force the test now, you force
the driver to send unmap commands down to the device.

> See https://lkml.org/lkml/2014/7/21/627 for the previous version of this
> patch and https://lkml.org/lkml/2014/7/23/615 for example devices.
> 
> Original-patch-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com>
> ---
>  drivers/scsi/storvsc_drv.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 5ad2810..88b7173 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -326,8 +326,6 @@ MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
>   */
>  static int storvsc_timeout = 180;
>  
> -static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
> -
>  #define STORVSC_MAX_IO_REQUESTS				200
>  
>  static void storvsc_on_channel_callback(void *context);
> @@ -1444,12 +1442,10 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
>  	sdevice->no_write_same = 1;
>  
>  	/*
> -	 * Add blist flags to permit the reading of the VPD pages even when
> -	 * the target may claim SPC-2 compliance. MSFT targets currently
> -	 * claim SPC-2 compliance while they implement post SPC-2 features.
> -	 * With this patch we can correctly handle WRITE_SAME_16 issues.
> +	 * Forcefully enable logical block provisioning testing.
>  	 */
> -	sdevice->sdev_bflags |= msft_blist_flags;
> +	sdevice->sdev_bflags |= BLIST_TRY_LBP;
> +	sdevice->try_lbp = 1;

Really no way to this one.  You're forcing unmap support on every
hyper-v device; including spinning rust pass through ones which won't
support it.  The hyper-v storage interface has proven to be somewhat
fragile, so it may explode when the device tries to send illegal command
sense back.

If you have a specific IDENTITY string for a faulty SSD that fails to
report unmap support correctly, then we could quirk for that, but not
everything attached to the hyper-v driver.

James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/3] [SCSI] storvsc: Add Hyper-V logical block provisioning tests

[Sitsofe Wheeler]

On Thu, Jul 24, 2014 at 02:09:11PM +0000, James Bottomley wrote:
> On Thu, 2014-07-24 at 08:56 +0100, Sitsofe Wheeler wrote:
> > Microsoft Hyper-V targets currently only claim SPC-2 compliance / no
> > compliance indicated even though they implement post SPC-2 features
> > which means those features are not tested for. Add a blacklist flag to
> > Hyper-V devices that forces said testing.
> 
> This description is misleading: you don't force the test now, you force
> the driver to send unmap commands down to the device.

I disagree - UNMAP will only be used if the vpd pages say that UNMAP is
supported by the device. I've added two hard disks (one SATA, one on USB) to
the VM and here's the debug output with all three patches applied and some
extra logging:

sd 1:0:0:4: [sdc] Entered read_capacity_16
sd 1:0:0:4: [sdc] Past illegal req
sd 1:0:0:4: [sdc] Protection check
sd 1:0:0:4: [sdc] Got past protection check
sd 1:0:0:4: [sdc] Checking LBPME
sd 1:0:0:4: [sdc] 1953525168 512-byte logical blocks: (1.00 TB/931 GiB)
sd 1:0:0:4: [sdc] 4096-byte physical blocks
sd 1:0:0:4: [sdc] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:4: [sdc] sd_revalidate_disk: Performing extended inquiries
sd 1:0:0:4: [sdc] sd_read_block_provisioning: Entered, lbmpe: 0
sd 1:0:0:4: [sdc] sd_read_block_provisioning: Passed lbmpe test
sd 1:0:0:4: [sdc] Entered block limits
sd 1:0:0:4: [sdc] Started block limits
sd 1:0:0:4: [sdc] 0x3c...
sd 1:0:0:4: [sdc] Write Protect is off
sd 1:0:0:4: [sdc] Mode Sense: 0f 00 00 00
sd 1:0:0:2: [sdd] Entered read_capacity_16
sd 1:0:0:4: [sdc] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 1:0:0:2: [sdd] 976773168 512-byte logical blocks: (500 GB/465 GiB)
sd 1:0:0:4: [sdc] Entered read_capacity_16
sd 1:0:0:2: [sdd] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:2: [sdd] sd_revalidate_disk: Performing extended inquiries
sd 1:0:0:2: [sdd] sd_read_block_provisioning: Entered, lbmpe: 0
sd 1:0:0:2: [sdd] sd_read_block_provisioning: Passed lbmpe test
sd 1:0:0:4: [sdc] Past illegal req
sd 1:0:0:4: [sdc] Protection check
sd 1:0:0:4: [sdc] Got past protection check
sd 1:0:0:4: [sdc] Checking LBPME
sd 1:0:0:2: [sdd] Entered block limits
sd 1:0:0:4: [sdc] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:4: [sdc] sd_revalidate_disk: Performing extended inquiries
sd 1:0:0:4: [sdc] sd_read_block_provisioning: Entered, lbmpe: 0
sd 1:0:0:4: [sdc] sd_read_block_provisioning: Passed lbmpe test
sd 1:0:0:4: [sdc] Entered block limits
sd 1:0:0:4: [sdc] Started block limits
sd 1:0:0:4: [sdc] 0x3c...
sd 1:0:0:2: [sdd] Write Protect is off
sd 1:0:0:2: [sdd] Mode Sense: 0f 00 00 00
sd 1:0:0:2: [sdd] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
sd 1:0:0:2: [sdd] Entered read_capacity_16
sd 1:0:0:2: [sdd] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:2: [sdd] sd_revalidate_disk: Performing extended inquiries
sd 1:0:0:2: [sdd] sd_read_block_provisioning: Entered, lbmpe: 0
sd 1:0:0:2: [sdd] sd_read_block_provisioning: Passed lbmpe test
sd 1:0:0:2: [sdd] Entered block limits
 sdd: unknown partition table
sd 1:0:0:2: [sdd] Entered read_capacity_16
sd 1:0:0:2: [sdd] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:2: [sdd] sd_revalidate_disk: Performing extended inquiries
sd 1:0:0:2: [sdd] sd_read_block_provisioning: Entered, lbmpe: 0
sd 1:0:0:2: [sdd] sd_read_block_provisioning: Passed lbmpe test
sd 1:0:0:2: [sdd] Entered block limits
sd 1:0:0:2: [sdd] Attached SCSI disk
 sdc: sdc1 sdc2
sd 1:0:0:4: [sdc] Entered read_capacity_16
sd 1:0:0:4: [sdc] Past illegal req
sd 1:0:0:4: [sdc] Protection check
sd 1:0:0:4: [sdc] Got past protection check
sd 1:0:0:4: [sdc] Checking LBPME
sd 1:0:0:4: [sdc] sd_revalidate_disk: Extended inquiry check...
sd 1:0:0:4: [sdc] sd_revalidate_disk: Performing extended inquiries
sd 1:0:0:4: [sdc] sd_read_block_provisioning: Entered, lbmpe: 0
sd 1:0:0:4: [sdc] sd_read_block_provisioning: Passed lbmpe test
sd 1:0:0:4: [sdc] Entered block limits
sd 1:0:0:4: [sdc] Started block limits
sd 1:0:0:4: [sdc] 0x3c...
sd 1:0:0:4: [sdc] Attached SCSI disk

Both
/sys/block/sdc/device/scsi_disk/1\:0\:0\:4/thin_provisioning
and
cat /sys/block/sdd/device/scsi_disk/1\:0\:0\:2/thin_provisioning
return 0.

A Hyper-V VHDX disk had output like this:
sd 0:0:0:0: [sda] Entered read_capacity_16
sd 0:0:0:0: [sda] Past illegal req
sd 0:0:0:0: [sda] Protection check
sd 0:0:0:0: [sda] Got past protection check
sd 0:0:0:0: [sda] Checking LBPME
sd 0:0:0:0: [sda] LBPME OK!
sd 0:0:0:0: [sda] Discard mode: 2
sd 0:0:0:0: [sda] 8388608 512-byte logical blocks: (4.29 GB/4.00 GiB)
sd 0:0:0:0: [sda] sd_revalidate_disk: Extended inquiry check...
sd 0:0:0:0: [sda] sd_revalidate_disk: Performing extended inquiries
sd 0:0:0:0: [sda] sd_read_block_provisioning: Entered, lbmpe: 1
sd 0:0:0:0: [sda] sd_read_block_provisioning: Passed lbmpe test
sd 0:0:0:0: [sda] sd_read_block_provisioning: Setting block provisioning
sd 0:0:0:0: [sda] Entered block limits
sd 0:0:0:0: [sda] Started block limits
sd 0:0:0:0: [sda] 0x3c...
sd 0:0:0:0: [sda] Testing lbpme...
sd 0:0:0:0: [sda] ...lbpme test done
sd 0:0:0:0: [sda] Entering discard switch via LBP VPD
sd 0:0:0:0: [sda] Discard mode: 1
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 0f 00 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sd 0:0:0:0: [sda] Entered read_capacity_16
sd 0:0:0:0: [sda] Past illegal req
sd 0:0:0:0: [sda] Protection check
sd 0:0:0:0: [sda] Got past protection check
sd 0:0:0:0: [sda] Checking LBPME
sd 0:0:0:0: [sda] LBPME OK!
sd 0:0:0:0: [sda] Discard mode: 2
sd 0:0:0:0: [sda] sd_revalidate_disk: Extended inquiry check...
sd 0:0:0:0: [sda] sd_revalidate_disk: Performing extended inquiries
sd 0:0:0:0: [sda] sd_read_block_provisioning: Entered, lbmpe: 1
sd 0:0:0:0: [sda] sd_read_block_provisioning: Passed lbmpe test
sd 0:0:0:0: [sda] sd_read_block_provisioning: Setting block provisioning
sd 0:0:0:0: [sda] Entered block limits
sd 0:0:0:0: [sda] Started block limits
sd 0:0:0:0: [sda] 0x3c...
sd 0:0:0:0: [sda] Testing lbpme...
sd 0:0:0:0: [sda] ...lbpme test done
sd 0:0:0:0: [sda] Entering discard switch via LBP VPD
sd 0:0:0:0: [sda] Discard mode: 1
 sda: sda1
sd 0:0:0:0: [sda] Entered read_capacity_16
sd 0:0:0:0: [sda] Past illegal req
sd 0:0:0:0: [sda] Protection check
sd 0:0:0:0: [sda] Got past protection check
sd 0:0:0:0: [sda] Checking LBPME
sd 0:0:0:0: [sda] LBPME OK!
sd 0:0:0:0: [sda] Discard mode: 2
sd 0:0:0:0: [sda] sd_revalidate_disk: Extended inquiry check...
sd 0:0:0:0: [sda] sd_revalidate_disk: Performing extended inquiries
sd 0:0:0:0: [sda] sd_read_block_provisioning: Entered, lbmpe: 1
sd 0:0:0:0: [sda] sd_read_block_provisioning: Passed lbmpe test
sd 0:0:0:0: [sda] sd_read_block_provisioning: Setting block provisioning
sd 0:0:0:0: [sda] Entered block limits
sd 0:0:0:0: [sda] Started block limits
sd 0:0:0:0: [sda] 0x3c...
sd 0:0:0:0: [sda] Testing lbpme...
sd 0:0:0:0: [sda] ...lbpme test done
sd 0:0:0:0: [sda] Entering discard switch via LBP VPD
sd 0:0:0:0: [sda] Discard mode: 1
sd 0:0:0:0: [sda] Attached SCSI disk


The hard disk never set the Discard mode so why would UNMAP be forced.

> >  	/*
> > -	 * Add blist flags to permit the reading of the VPD pages even when
> > -	 * the target may claim SPC-2 compliance. MSFT targets currently
> > -	 * claim SPC-2 compliance while they implement post SPC-2 features.
> > -	 * With this patch we can correctly handle WRITE_SAME_16 issues.
> > +	 * Forcefully enable logical block provisioning testing.
> >  	 */
> > -	sdevice->sdev_bflags |= msft_blist_flags;
> > +	sdevice->sdev_bflags |= BLIST_TRY_LBP;
> > +	sdevice->try_lbp = 1;
> 
> Really no way to this one.  You're forcing unmap support on every
> hyper-v device; including spinning rust pass through ones which won't
> support it.  The hyper-v storage interface has proven to be somewhat
> fragile, so it may explode when the device tries to send illegal command
> sense back.

OK this one I can't deny but I couldn't see how else to cope with passthrough
disks (see below)...

> If you have a specific IDENTITY string for a faulty SSD that fails to
> report unmap support correctly, then we could quirk for that, but not
> everything attached to the hyper-v driver.

Let me be clear that there are two cases:

1. Hyper-V VHDX files.
2. Passthrough devices (such as SSDs) that support discard.

We can definitely identify 1. (and I have an unposted patch that
modified the struct in scsi_devinfo.c to only quirk on Hyper-V VHDX
devices) but with 2. the SATA SSD I have enables discard fine with Linux
on the host directly - it's the act of passing it through Hyper-V that
causes the issue (no SCSI conformance level is claimed and lbmpe is not
set) and that could theoretically apply to all TRIM supporting SATA SSDs
that are passed through Hyper-V.

Additionally K. Y.  Srinivasan had a similar (but slightly different)
patch to my unposted one and Christoph said:

> This looks way to complicated - should be a single line added to your
> slave_configure function, maybe plus a comment stating what you do
> in your commit message:
> 
> 
>         sdev->sdev_bflags |= BLIST_TRY_VPD_PAGES;

So it was changed to be similar to impact everything attached to
Hyper-V. If you don't like this you may want to chime in on
https://lkml.org/lkml/2014/7/24/33 (Re: [PATCH 1/1] Drivers: scsi:
storvsc: Add blist flags).

-- 
Sitsofe | http://sucs.org/~sits/

[PATCH 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Sitsofe Wheeler]

Some block devices (such as Hyper-V passthrough SSDs) support logical
block provisioning (e.g. via UNMAP) but don't set lbpme thus disabling
discard. If the try_lbp quirk is in use skip lbpme checks that lead up
to the logical block provisioning tests.

Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com>
---
 drivers/scsi/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8249e51..8bf34bc 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2559,7 +2559,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
 
-		if (!sdkp->lbpme)
+		if (!sdkp->lbpme && !sdkp->device->try_lbp)
 			goto out;
 
 		lba_count = get_unaligned_be32(&buffer[20]);
@@ -2633,7 +2633,7 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 	unsigned char *buffer;
 	const int vpd_len = 8;
 
-	if (sdkp->lbpme == 0)
+	if (sdkp->lbpme == 0 && sdkp->device->test_lbp == 0)
 		return;
 
 	buffer = kmalloc(vpd_len, GFP_KERNEL);
-- 
1.9.3

[PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Sitsofe Wheeler]

v1 -> v2: Fix incorrectly named variable.

Some block devices (such as Hyper-V passthrough SSDs) support logical
block provisioning (e.g. via UNMAP) but don't set lbpme thus disabling
discard. If the try_lbp quirk is in use skip lbpme checks that lead up
to the logical block provisioning tests.

Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com>
---
 drivers/scsi/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8249e51..8bf34bc 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2559,7 +2559,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
 
-		if (!sdkp->lbpme)
+		if (!sdkp->lbpme && !sdkp->device->try_lbp)
 			goto out;
 
 		lba_count = get_unaligned_be32(&buffer[20]);
@@ -2633,7 +2633,7 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 	unsigned char *buffer;
 	const int vpd_len = 8;
 
-	if (sdkp->lbpme == 0)
+	if (sdkp->lbpme == 0 && sdkp->device->try_lbp == 0)
 		return;
 
 	buffer = kmalloc(vpd_len, GFP_KERNEL);
-- 
1.9.3

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Martin K. Petersen]

>>>>> "Sitsofe" == Sitsofe Wheeler <sitsofe@gmail.com> writes:

Sitsofe> Fix incorrectly named variable.  Some block devices (such as
Sitsofe> Hyper-V passthrough SSDs) support logical block provisioning
Sitsofe> (e.g. via UNMAP) but don't set lbpme thus disabling discard. 

The fix for an SSD that is known to support LBP but which does not claim
support for it is to use:

    echo unmap > /sys/class/scsi_disk/foo/provisioning_mode

I'm very much against short-circuiting the LBP logic in a passthrough
driver because then we might end up in the exact situation we were
trying to avoid with this patch series. Namely sending down commands
unsupported by the target device.

This kind of thing really needs to be a sysadmin decision and can be
handled with a udev rule.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Christoph Hellwig]

On Thu, Jul 24, 2014 at 09:54:24AM -0400, Martin K. Petersen wrote:
> I'm very much against short-circuiting the LBP logic in a passthrough
> driver because then we might end up in the exact situation we were
> trying to avoid with this patch series. Namely sending down commands
> unsupported by the target device.
> 
> This kind of thing really needs to be a sysadmin decision and can be
> handled with a udev rule.

I agree - I'd like to pull in KY's simple fix as soon as I get a second
review for it.

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Christoph Hellwig]

On Thu, Jul 24, 2014 at 08:34:19AM -0700, Christoph Hellwig wrote:
> I agree - I'd like to pull in KY's simple fix as soon as I get a second
> review for it.

Ok, looks like I just got that from Hannes.  Let's see if there's more
to be done for the pass through case, but I'd rather wait for the next
window for that.

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Sitsofe Wheeler]

On Thu, Jul 24, 2014 at 08:35:13AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 24, 2014 at 08:34:19AM -0700, Christoph Hellwig wrote:
> > I agree - I'd like to pull in KY's simple fix as soon as I get a second
> > review for it.
> 
> Ok, looks like I just got that from Hannes.  Let's see if there's more
> to be done for the pass through case, but I'd rather wait for the next
> window for that.

There's nothing Linux can do for today's Hyper-V passthrough wrt to
automatically turning on discard on my SATA SSD.

Martin explained that vendors often disable UNMAP/WRITE SAME to DSM TRIM
translation on purpose and/or have explicit device whitelists/blacklists
for those devices they want to support. It could be that Microsoft are
doing something similar (although it might be interesting to know how a
Windows 2012 guest on Hyper-V handles things).

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Sitsofe Wheeler]

On Thu, Jul 24, 2014 at 09:54:24AM -0400, Martin K. Petersen wrote:
> >>>>> "Sitsofe" == Sitsofe Wheeler <sitsofe@gmail.com> writes:
> 
> Sitsofe> Fix incorrectly named variable.  Some block devices (such as
> Sitsofe> Hyper-V passthrough SSDs) support logical block provisioning
> Sitsofe> (e.g. via UNMAP) but don't set lbpme thus disabling discard. 
> 
> The fix for an SSD that is known to support LBP but which does not claim
> support for it is to use:
> 
>     echo unmap > /sys/class/scsi_disk/foo/provisioning_mode
> 
> I'm very much against short-circuiting the LBP logic in a passthrough
> driver because then we might end up in the exact situation we were
> trying to avoid with this patch series. Namely sending down commands
> unsupported by the target device.
> 
> This kind of thing really needs to be a sysadmin decision and can be
> handled with a udev rule.

The problem is that the SSD _does_ claim to support it. Here are the
results of booting Linux directly on the host hardware and looking at
the device directly with a 3.10.35 kernel:

root@sysresccd /root % uname -a
Linux sysresccd 3.10.35-std420-amd64 #2 SMP Wed Apr 2 18:31:51 UTC 2014 x86_64 Intel(R) Core(TM) i7-3930K CPU @ 3.20GHz GenuineIntel GNU/Linux
root@sysresccd /root % sg_vpd -p lbpv /dev/sdb
root@sysresccd /root % sg_inq /dev/sdb   
standard INQUIRY:
  PQual=0  Device_type=0  RMB=0  version=0x05  [SPC-3]
  [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
  SCCS=0  ACC=0  TPGS=0  3PC=0  Protect=0  [BQue=0]
  EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
  [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=0
  [SPI: Clocking=0x0  QAS=0  IUS=0]
    length=96 (0x60)   Peripheral device type: disk
 Vendor identification: ATA     
 Product identification: ADATA SSD S510 1
 Product revision level: 5.2.
 Unit serial number: 03205115500300002076
root@sysresccd /root % sg_readcap -l /dev/sdb
Read Capacity results:
   Protection: prot_en=0, p_type=0, p_i_exponent=0
   Logical block provisioning: lbpme=1, lbprz=0
   Last logical block address=234441647 (0xdf94baf), Number of logical blocks=234441648
   Logical block length=512 bytes
   Logical blocks per physical block exponent=0
   Lowest aligned logical block address=0
Hence:
   Device size: 120034123776 bytes, 114473.5 MiB, 120.03 GB
Logical block provisioning VPD page (SBC):
  Unmap command supported (LBPU): 0
  Write same (16) with unmap bit supported (LBWS): 1
  Write same (10) with unmap bit supported (LBWS10): 0
  Logical block provisioning read zeros (LBPRZ): 0
  Anchored LBAs supported (ANC_SUP): 0
  Threshold exponent: 0
  Descriptor present (DP): 0
  Provisioning type: 0
root@sysresccd /root % sg_vpd -p bl /dev/sdb
Block limits VPD page (SBC):
  Write same no zero (WSNZ): 0
  Maximum compare and write length: 0 blocks
  Optimal transfer length granularity: 1 blocks
  Maximum transfer length: 0 blocks
  Optimal transfer length: 0 blocks
  Maximum prefetch length: 0 blocks
  Maximum unmap LBA count: 0
  Maximum unmap block descriptor count: 0
  Optimal unmap granularity: 1
  Unmap granularity alignment valid: 0
  Unmap granularity alignment: 0
  Maximum write same length: 0x3fffc0 blocks
root@sysresccd /root % grep . /sys/block/sdb/device/scsi_disk/1:0:0:0/*                
/sys/block/sdb/device/scsi_disk/1:0:0:0/allow_restart:1
/sys/block/sdb/device/scsi_disk/1:0:0:0/app_tag_own:0
/sys/block/sdb/device/scsi_disk/1:0:0:0/cache_type:write back
grep: /sys/block/sdb/device/scsi_disk/1:0:0:0/device: Is a directory
/sys/block/sdb/device/scsi_disk/1:0:0:0/FUA:0
/sys/block/sdb/device/scsi_disk/1:0:0:0/manage_start_stop:1
/sys/block/sdb/device/scsi_disk/1:0:0:0/max_medium_access_timeouts:2
/sys/block/sdb/device/scsi_disk/1:0:0:0/max_write_same_blocks:0
grep: /sys/block/sdb/device/scsi_disk/1:0:0:0/power: Is a directory
/sys/block/sdb/device/scsi_disk/1:0:0:0/protection_mode:none
/sys/block/sdb/device/scsi_disk/1:0:0:0/protection_type:0
/sys/block/sdb/device/scsi_disk/1:0:0:0/provisioning_mode:writesame_16
grep: /sys/block/sdb/device/scsi_disk/1:0:0:0/subsystem: Is a directory
/sys/block/sdb/device/scsi_disk/1:0:0:0/thin_provisioning:1

So we can see it is really a SATA device that announces discard
correctly and supports discard through WRITE_SAME(16). It is the act of
passing it through Hyper-V that turned it into a SCSI device that supports
UNMAP (but not WRITE_SAME(16)), doesn't announce its SCSI conformance number
and doesn't correctly announce which features it supports. Surely in
this case it's reasonable to quirk our way around the problem?

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Martin K. Petersen]

>>>>> "Sitsofe" == Sitsofe Wheeler <sitsofe@gmail.com> writes:

Sitsofe> So we can see it is really a SATA device that announces discard
Sitsofe> correctly and supports discard through WRITE_SAME(16).

No, that's the SATA device that announces support for DSM TRIM, and as a
result the Linux SATL reports support for WRITE SAME(16) w. the UNMAP
bit set and LBPME.

Sitsofe> It is the act of passing it through Hyper-V that turned it into
Sitsofe> a SCSI device that supports UNMAP (but not WRITE_SAME(16)),
Sitsofe> doesn't announce its SCSI conformance number and doesn't
Sitsofe> correctly announce which features it supports. Surely in this
Sitsofe> case it's reasonable to quirk our way around the problem?

No. That's an issue in Hyper-V that'll you'll have to take up with
Microsoft. I don't know what their passthrough limitations are for
SCSI-ATA translation. Maybe K. Y. has some insight into this?

There must be a reason why the VPD page was added and yet the device not
flagged as LBPME=1.

Many vendors do not support UNMAP/WRITE SAME to DSM TRIM translation.
Additionally, many vendors explicitly only whitelist drives that are
known to be working correctly. Your drive is an ADATA and therefore very
likely to be blacklisted by default by a vendor SATL.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -----Original Message-----
> From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> Sent: Thursday, July 24, 2014 8:54 AM
> To: Sitsofe Wheeler
> Cc: Martin K. Petersen; Christoph Hellwig; KY Srinivasan;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> jasowang@redhat.com; jbottomley@parallels.com; linux-
> scsi@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "Sitsofe" == Sitsofe Wheeler <sitsofe@gmail.com> writes:
> 
> Sitsofe> So we can see it is really a SATA device that announces discard
> Sitsofe> correctly and supports discard through WRITE_SAME(16).
> 
> No, that's the SATA device that announces support for DSM TRIM, and as a
> result the Linux SATL reports support for WRITE SAME(16) w. the UNMAP bit
> set and LBPME.
> 
> Sitsofe> It is the act of passing it through Hyper-V that turned it into
> Sitsofe> a SCSI device that supports UNMAP (but not WRITE_SAME(16)),
> Sitsofe> doesn't announce its SCSI conformance number and doesn't
> Sitsofe> correctly announce which features it supports. Surely in this
> Sitsofe> case it's reasonable to quirk our way around the problem?
> 
> No. That's an issue in Hyper-V that'll you'll have to take up with Microsoft. I
> don't know what their passthrough limitations are for SCSI-ATA translation.
> Maybe K. Y. has some insight into this?

For the pass through case, the host validates the request and passes the request to the device.
However, not all scsi commands are passed through even though the device it is being passed through
may support the command. WRITE_SAME is one such command. Consequently, in the EVPD page,
we will set state indicating that WRITE_SAME is not supported (even if the device supports it).

Hope this helps.

K. Y 
> 
> There must be a reason why the VPD page was added and yet the device not
> flagged as LBPME=1.
> 
> Many vendors do not support UNMAP/WRITE SAME to DSM TRIM
> translation.
> Additionally, many vendors explicitly only whitelist drives that are known to
> be working correctly. Your drive is an ADATA and therefore very likely to be
> blacklisted by default by a vendor SATL.
> 
> --
> Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Martin K. Petersen]

>>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:

KY> For the pass through case, the host validates the request and passes
KY> the request to the device.  However, not all scsi commands are
KY> passed through even though the device it is being passed through may
KY> support the command. WRITE_SAME is one such command. Consequently,
KY> in the EVPD page, we will set state indicating that WRITE_SAME is
KY> not supported (even if the device supports it).

The LBP VPD page flags UNMAP as being supported. Do you actually support
UNMAP to DSM TRIM SCSI-ATA translation?

One challenge in that department is that a single UNMAP command may turn
into many, many, many DSM TRIM commands on the underlying SATA device.
That's why we went with WRITE SAME for the internal Linux SATL, capping
the maximum number of blocks to what we can fit in a single DSM TRIM
command.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -----Original Message-----
> From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> Sent: Friday, July 25, 2014 9:57 AM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> jasowang@redhat.com; jbottomley@parallels.com; linux-
> scsi@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> 
> KY> For the pass through case, the host validates the request and passes
> KY> the request to the device.  However, not all scsi commands are
> KY> passed through even though the device it is being passed through may
> KY> support the command. WRITE_SAME is one such command.
> Consequently,
> KY> in the EVPD page, we will set state indicating that WRITE_SAME is
> KY> not supported (even if the device supports it).
> 
> The LBP VPD page flags UNMAP as being supported. Do you actually support
> UNMAP to DSM TRIM SCSI-ATA translation?

Martin,

I have been told by the Windows folks that this is done. I am trying to get
additional details.

K. Y
> 
> One challenge in that department is that a single UNMAP command may turn
> into many, many, many DSM TRIM commands on the underlying SATA
> device.
> That's why we went with WRITE SAME for the internal Linux SATL, capping
> the maximum number of blocks to what we can fit in a single DSM TRIM
> command.
> 
> --
> Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Martin K. Petersen]

>>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:

>> The LBP VPD page flags UNMAP as being supported. Do you actually
>> support UNMAP to DSM TRIM SCSI-ATA translation?

KY> I have been told by the Windows folks that this is done. I am trying
KY> to get additional details.

Great! I'd just like to have a reasonable level of confidence in what's
happening down the stack before I entertain turning something on that's
not being properly advertised.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -----Original Message-----
> From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> Sent: Saturday, July 26, 2014 9:55 AM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> jasowang@redhat.com; jbottomley@parallels.com; linux-
> scsi@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> 
> >> The LBP VPD page flags UNMAP as being supported. Do you actually
> >> support UNMAP to DSM TRIM SCSI-ATA translation?
> 
> KY> I have been told by the Windows folks that this is done. I am trying
> KY> to get additional details.
> 
> Great! I'd just like to have a reasonable level of confidence in what's
> happening down the stack before I entertain turning something on that's not
> being properly advertised.

As I look at the output of inquiry between Linux on Hyper-V and native Linux, is not specifying
conformance level the main issue?

K. Y
> 
> --
> Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Martin K. Petersen]

>>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:

>> Great! I'd just like to have a reasonable level of confidence in
>> what's happening down the stack before I entertain turning something
>> on that's not being properly advertised.

KY> As I look at the output of inquiry between Linux on Hyper-V and
KY> native Linux, is not specifying conformance level the main issue?

The main problem for this particular use case (aside from the issue
we've already addressed) is that the passthrough device (SATA SSD) has
LBPME=0 in the READ CAPACITY(16) response. The LBP VPD is correctly
provided with LBPU flag set but because LBPME is reported as disabled we
will not attempt to issue UNMAP commands to the device.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -----Original Message-----
> From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> Sent: Saturday, July 26, 2014 12:25 PM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> jasowang@redhat.com; jbottomley@parallels.com; linux-
> scsi@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> 
> >> Great! I'd just like to have a reasonable level of confidence in
> >> what's happening down the stack before I entertain turning something
> >> on that's not being properly advertised.
> 
> KY> As I look at the output of inquiry between Linux on Hyper-V and
> KY> native Linux, is not specifying conformance level the main issue?
> 
> The main problem for this particular use case (aside from the issue we've
> already addressed) is that the passthrough device (SATA SSD) has
> LBPME=0 in the READ CAPACITY(16) response. The LBP VPD is correctly
> provided with LBPU flag set but because LBPME is reported as disabled we
> will not attempt to issue UNMAP commands to the device.

Oh; ok. I missed the read_capacity response.

Thanks,

K. Y
> 
> --
> Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -----Original Message-----
> From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> Sent: Saturday, July 26, 2014 12:25 PM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> jasowang@redhat.com; jbottomley@parallels.com; linux-
> scsi@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> 
> >> Great! I'd just like to have a reasonable level of confidence in
> >> what's happening down the stack before I entertain turning something
> >> on that's not being properly advertised.
> 
> KY> As I look at the output of inquiry between Linux on Hyper-V and
> KY> native Linux, is not specifying conformance level the main issue?
> 
> The main problem for this particular use case (aside from the issue we've
> already addressed) is that the passthrough device (SATA SSD) has
> LBPME=0 in the READ CAPACITY(16) response. The LBP VPD is correctly
> provided with LBPU flag set but because LBPME is reported as disabled we
> will not attempt to issue UNMAP commands to the device.

Had a chance to chat with the Windows sscsi folks. Here is their response:

"At the time  thin-provisioning was defined, the discovery information was first proposed in READ CAPACITY 16 command. And then moved into the new dedicated VPD page - B2h. You can see the information reported in this VPD page is richer than READ CAPACITY 16 command. As this transition happened during we added the feature, Windows uses the newer method that based on VPD page B2h. It looks Linux tries to use both new and old method which is weird to me."

Would it make sense for us to work  around this issue for Linux on Hyper-V (just use the VPD page instead of looking at the output of READ CAPACITY)?

Regards,

K. Y
> 
> --
> Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Martin K. Petersen]

>>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:

KY,

KY> "At the time thin-provisioning was defined, the discovery
KY> information was first proposed in READ CAPACITY 16 command. And then
KY> moved into the new dedicated VPD page - B2h. You can see the
KY> information reported in this VPD page is richer than READ CAPACITY
KY> 16 command. As this transition happened during we added the feature,
KY> Windows uses the newer method that based on VPD page B2h. It looks
KY> Linux tries to use both new and old method which is weird to me."

The READ CAPACITY(16) response is not optional.

SBC3r36 section 4.7.3.3 Thin provisioned logical unit:

The device server in a thin provisioned logical unit shall set:

a) the LBPME bit to one in the READ CAPACITY (16) parameter data (see
   5.16.2); and

b) the PROVISIONING TYPE field to 010b (i.e., thin provisioned) in the
   Logical Block Provisioning VPD page (see 6.6.4).

That's a "shall". The LBP VPD elaborates on the provisioning type,
commands preference, etc. But it's all gated by LBPME=1 in the READ
CAPACITY(16) response.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

> -----Original Message-----
> From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> Sent: Monday, July 28, 2014 12:03 PM
> To: KY Srinivasan
> Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> jasowang@redhat.com; jbottomley@parallels.com; linux-
> scsi@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> 
> KY,
> 
> KY> "At the time thin-provisioning was defined, the discovery
> KY> information was first proposed in READ CAPACITY 16 command. And
> then
> KY> moved into the new dedicated VPD page - B2h. You can see the
> KY> information reported in this VPD page is richer than READ CAPACITY
> KY> 16 command. As this transition happened during we added the feature,
> KY> Windows uses the newer method that based on VPD page B2h. It looks
> KY> Linux tries to use both new and old method which is weird to me."
> 
> The READ CAPACITY(16) response is not optional.

Ok; that settles the issue then. I will attempt to get it fixed on Windows.

K. Y

> 
> --
> Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[James Bottomley]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1753 bytes --]

On Mon, 2014-07-28 at 19:05 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> > Sent: Monday, July 28, 2014 12:03 PM
> > To: KY Srinivasan
> > Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> > jasowang@redhat.com; jbottomley@parallels.com; linux-
> > scsi@vger.kernel.org
> > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> > 
> > >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> > 
> > KY,
> > 
> > KY> "At the time thin-provisioning was defined, the discovery
> > KY> information was first proposed in READ CAPACITY 16 command. And
> > then
> > KY> moved into the new dedicated VPD page - B2h. You can see the
> > KY> information reported in this VPD page is richer than READ CAPACITY
> > KY> 16 command. As this transition happened during we added the feature,
> > KY> Windows uses the newer method that based on VPD page B2h. It looks
> > KY> Linux tries to use both new and old method which is weird to me."
> > 
> > The READ CAPACITY(16) response is not optional.
> 
> Ok; that settles the issue then. I will attempt to get it fixed on Windows.

Like Martin says, this isn't optional either/or; it's mandatory to
support the RC 16 bits.  If you don't want to get into playing the
messenger between us and the windows guys on SCSI standards, we'd be
happy to communicate directly, either by email or a phone meeting.

James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2370 bytes --]



> -----Original Message-----
> From: James Bottomley [mailto:jbottomley@parallels.com]
> Sent: Monday, July 28, 2014 1:03 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; hch@infradead.org; sitsofe@gmail.com;
> devel@linuxdriverproject.org; apw@canonical.com;
> martin.petersen@oracle.com; linux-scsi@vger.kernel.org;
> ohering@suse.com; gregkh@linuxfoundation.org; jasowang@redhat.com
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> On Mon, 2014-07-28 at 19:05 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> > > Sent: Monday, July 28, 2014 12:03 PM
> > > To: KY Srinivasan
> > > Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> > > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> > > jasowang@redhat.com; jbottomley@parallels.com; linux-
> > > scsi@vger.kernel.org
> > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks
> > > tests
> > >
> > > >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> > >
> > > KY,
> > >
> > > KY> "At the time thin-provisioning was defined, the discovery
> > > KY> information was first proposed in READ CAPACITY 16 command. And
> > > then
> > > KY> moved into the new dedicated VPD page - B2h. You can see the
> > > KY> information reported in this VPD page is richer than READ
> > > KY> CAPACITY
> > > KY> 16 command. As this transition happened during we added the
> > > KY> feature, Windows uses the newer method that based on VPD page
> > > KY> B2h. It looks Linux tries to use both new and old method which is
> weird to me."
> > >
> > > The READ CAPACITY(16) response is not optional.
> >
> > Ok; that settles the issue then. I will attempt to get it fixed on Windows.
> 
> Like Martin says, this isn't optional either/or; it's mandatory to support the RC
> 16 bits.  If you don't want to get into playing the messenger between us and
> the windows guys on SCSI standards, we'd be happy to communicate
> directly, either by email or a phone meeting.

Thanks James. I will take up your offer if needed.

Regards,

K. Y
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2445 bytes --]



> -----Original Message-----
> From: James Bottomley [mailto:jbottomley@parallels.com]
> Sent: Monday, July 28, 2014 1:03 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; hch@infradead.org; sitsofe@gmail.com;
> devel@linuxdriverproject.org; apw@canonical.com;
> martin.petersen@oracle.com; linux-scsi@vger.kernel.org;
> ohering@suse.com; gregkh@linuxfoundation.org; jasowang@redhat.com
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> On Mon, 2014-07-28 at 19:05 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> > > Sent: Monday, July 28, 2014 12:03 PM
> > > To: KY Srinivasan
> > > Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig;
> > > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> > > jasowang@redhat.com; jbottomley@parallels.com; linux-
> > > scsi@vger.kernel.org
> > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks
> > > tests
> > >
> > > >>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:
> > >
> > > KY,
> > >
> > > KY> "At the time thin-provisioning was defined, the discovery
> > > KY> information was first proposed in READ CAPACITY 16 command. And
> > > then
> > > KY> moved into the new dedicated VPD page - B2h. You can see the
> > > KY> information reported in this VPD page is richer than READ
> > > KY> CAPACITY
> > > KY> 16 command. As this transition happened during we added the
> > > KY> feature, Windows uses the newer method that based on VPD page
> > > KY> B2h. It looks Linux tries to use both new and old method which is
> weird to me."
> > >
> > > The READ CAPACITY(16) response is not optional.
> >
> > Ok; that settles the issue then. I will attempt to get it fixed on Windows.
> 
> Like Martin says, this isn't optional either/or; it's mandatory to support the RC
> 16 bits.  If you don't want to get into playing the messenger between us and
> the windows guys on SCSI standards, we'd be happy to communicate
> directly, either by email or a phone meeting.

We will fix this bug in the next release of Windows; we are also looking at backporting the fix to prior versions of Windows.

Thanks,

K. Y

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[Martin K. Petersen]

>>>>> "KY" == KY Srinivasan <kys@microsoft.com> writes:

KY> We will fix this bug in the next release of Windows; we are also
KY> looking at backporting the fix to prior versions of Windows.

Excellent. Thanks for looking into this!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[James Bottomley]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3093 bytes --]

On Fri, 2014-07-25 at 16:47 +0000, KY Srinivasan wrote:
> 
> > -----Original Message-----
> > From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> > Sent: Thursday, July 24, 2014 8:54 AM
> > To: Sitsofe Wheeler
> > Cc: Martin K. Petersen; Christoph Hellwig; KY Srinivasan;
> > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> > jasowang@redhat.com; jbottomley@parallels.com; linux-
> > scsi@vger.kernel.org
> > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> > 
> > >>>>> "Sitsofe" == Sitsofe Wheeler <sitsofe@gmail.com> writes:
> > 
> > Sitsofe> So we can see it is really a SATA device that announces discard
> > Sitsofe> correctly and supports discard through WRITE_SAME(16).
> > 
> > No, that's the SATA device that announces support for DSM TRIM, and as a
> > result the Linux SATL reports support for WRITE SAME(16) w. the UNMAP bit
> > set and LBPME.
> > 
> > Sitsofe> It is the act of passing it through Hyper-V that turned it into
> > Sitsofe> a SCSI device that supports UNMAP (but not WRITE_SAME(16)),
> > Sitsofe> doesn't announce its SCSI conformance number and doesn't
> > Sitsofe> correctly announce which features it supports. Surely in this
> > Sitsofe> case it's reasonable to quirk our way around the problem?
> > 
> > No. That's an issue in Hyper-V that'll you'll have to take up with Microsoft. I
> > don't know what their passthrough limitations are for SCSI-ATA translation.
> > Maybe K. Y. has some insight into this?
> 
> For the pass through case, the host validates the request and passes
> the request to the device.
> However, not all scsi commands are passed through even though the
> device it is being passed through
> may support the command. WRITE_SAME is one such command. Consequently,
> in the EVPD page,
> we will set state indicating that WRITE_SAME is not supported (even if
> the device supports it).

I think you haven't appreciated the problem: He's passing a SATA SSD via
the SCSI hyper-v interface.  That means that the windows host is doing
SCSI<->ATA translation.  The problem is that the Windows translation
layer (SATL) looks to be incomplete and it's not correctly translating
the IDENTIFY bit that corresponds to TRIM to the correct VPD pages so
consequently, Linux  won't send UNMAP commands to the device (to be
translated back to TRIM).

We already know this is a bug in the Windows SATL which needs fixing (if
you could report it and get a fix, that would be great) and that we're
not going to be able to work around this automatically in Linux because
the proposed patch would have us unconditionally try UNMAP for all
Hyper-V devices.  The current proposed fix is to enable UNMAP manually
via sysfs in the guest boot scripts, but obviously that means that
Hyper-V guests with direct pass through of SSDs need operator
intervention to turn on TRIM.

James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3902 bytes --]



> -----Original Message-----
> From: James Bottomley [mailto:jbottomley@parallels.com]
> Sent: Friday, July 25, 2014 10:10 AM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; hch@infradead.org; sitsofe@gmail.com;
> devel@linuxdriverproject.org; apw@canonical.com;
> martin.petersen@oracle.com; linux-scsi@vger.kernel.org;
> ohering@suse.com; gregkh@linuxfoundation.org; jasowang@redhat.com
> Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
> 
> On Fri, 2014-07-25 at 16:47 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> > > Sent: Thursday, July 24, 2014 8:54 AM
> > > To: Sitsofe Wheeler
> > > Cc: Martin K. Petersen; Christoph Hellwig; KY Srinivasan;
> > > gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; ohering@suse.com; apw@canonical.com;
> > > jasowang@redhat.com; jbottomley@parallels.com; linux-
> > > scsi@vger.kernel.org
> > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks
> > > tests
> > >
> > > >>>>> "Sitsofe" == Sitsofe Wheeler <sitsofe@gmail.com> writes:
> > >
> > > Sitsofe> So we can see it is really a SATA device that announces
> > > Sitsofe> discard correctly and supports discard through WRITE_SAME(16).
> > >
> > > No, that's the SATA device that announces support for DSM TRIM, and
> > > as a result the Linux SATL reports support for WRITE SAME(16) w. the
> > > UNMAP bit set and LBPME.
> > >
> > > Sitsofe> It is the act of passing it through Hyper-V that turned it
> > > Sitsofe> into a SCSI device that supports UNMAP (but not
> > > Sitsofe> WRITE_SAME(16)), doesn't announce its SCSI conformance
> > > Sitsofe> number and doesn't correctly announce which features it
> > > Sitsofe> supports. Surely in this case it's reasonable to quirk our way
> around the problem?
> > >
> > > No. That's an issue in Hyper-V that'll you'll have to take up with
> > > Microsoft. I don't know what their passthrough limitations are for SCSI-
> ATA translation.
> > > Maybe K. Y. has some insight into this?
> >
> > For the pass through case, the host validates the request and passes
> > the request to the device.
> > However, not all scsi commands are passed through even though the
> > device it is being passed through may support the command. WRITE_SAME
> > is one such command. Consequently, in the EVPD page, we will set state
> > indicating that WRITE_SAME is not supported (even if the device
> > supports it).
> 
> I think you haven't appreciated the problem: He's passing a SATA SSD via the
> SCSI hyper-v interface.  That means that the windows host is doing SCSI<-
> >ATA translation.  The problem is that the Windows translation layer (SATL)
> looks to be incomplete and it's not correctly translating the IDENTIFY bit that
> corresponds to TRIM to the correct VPD pages so consequently, Linux  won't
> send UNMAP commands to the device (to be translated back to TRIM).
> 
> We already know this is a bug in the Windows SATL which needs fixing (if you
> could report it and get a fix, that would be great) and that we're not going to
> be able to work around this automatically in Linux because the proposed
> patch would have us unconditionally try UNMAP for all Hyper-V devices.  The
> current proposed fix is to enable UNMAP manually via sysfs in the guest boot
> scripts, but obviously that means that Hyper-V guests with direct pass
> through of SSDs need operator intervention to turn on TRIM.

James,

Thanks for the clarification. I am talking to the folks in MSFT that develop the native scsi
stack on Windows. Hyper-V's back-end driver is not involved in SATL. I will keep you guys
posted.

Regards,

K. Y
> 
> James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/3] Enable discard on Hyper-V

[Sitsofe Wheeler]

On Thu, Jul 24, 2014 at 08:47:39AM +0100, Sitsofe Wheeler wrote:
> On Wed, Jul 23, 2014 at 09:13:41PM +0100, Sitsofe Wheeler wrote:
> > On Wed, Jul 23, 2014 at 07:15:58AM -0700, Christoph Hellwig wrote:
> > > On Wed, Jul 23, 2014 at 03:10:28PM +0100, Sitsofe Wheeler wrote:
> > > > I'm not sure this alone will work - won't sdev_bflags/bflags have
> > > > already been built at this point?
> > > 
> > > They've been built up, but we can still or new values into it.  It looks
> > > fine to me from review, but if you can test it on an actualy hypverv
> > > setup that would be valueable feedback.
> > 
> > The previous patches didn't work for me with a Windows 2012 R2 host with a
> > 3.16.0-rc6.x86_64-00076-g2f7d2ec-dirty guest. After applying
> > 
> > > term project, this late in the 3.17 cycle I'd just like to merge
> > > something that gets discards on hyperv to work.
> 
> OK how about the following patches:
> 
> Sitsofe Wheeler (3):
>   [SCSI] Add quirk for forcing logical block provisioning tests
>   [SCSI] storvsc: Add Hyper-V logical block provisioning quirk
>   [SCSI] Make LBP quirk skip lbpme checks

With the updated "Make LBP quirk skip lbpme checks" the above patches
enable discard on all Hyper-V disks (both VHDXs and passthrough SSDs) on
Windows 2012 R2 running a Linux 3.16.0-rc6.x86_64-00076-g2f7d2ec-dirty
guest.

Adding back the debugging and looking at just the discard messages shows
this:

sd 0:0:0:0: [sda] Discard mode: 2
sd 0:0:0:0: [sda] Entering discard switch via LBP VPD
sd 0:0:0:0: [sda] Discard mode: 1
sd 0:0:0:0: [sda] Discard mode: 2
sd 0:0:0:0: [sda] Entering discard switch via LBP VPD
sd 0:0:0:0: [sda] Discard mode: 1
sd 0:0:0:0: [sda] Discard mode: 2
sd 0:0:0:0: [sda] Entering discard switch via LBP VPD
sd 0:0:0:0: [sda] Discard mode: 1
sd 1:0:0:3: [sdd] Discard mode: 2
sd 1:0:0:1: [sdc] Entering discard switch with NO LBP VPD
sd 1:0:0:1: [sdc] Discard mode: 1
sd 1:0:0:3: [sdd] Entering discard switch via LBP VPD
sd 1:0:0:3: [sdd] Discard mode: 1
sd 1:0:0:0: [sdb] Discard mode: 2
sd 1:0:0:3: [sdd] Discard mode: 2
sd 1:0:0:1: [sdc] Entering discard switch with NO LBP VPD
sd 1:0:0:1: [sdc] Discard mode: 1
sd 1:0:0:3: [sdd] Entering discard switch via LBP VPD
sd 1:0:0:3: [sdd] Discard mode: 1
sd 1:0:0:1: [sdc] Entering discard switch with NO LBP VPD
sd 1:0:0:1: [sdc] Discard mode: 1
sd 1:0:0:0: [sdb] Entering discard switch via LBP VPD
sd 1:0:0:0: [sdb] Discard mode: 1
sd 1:0:0:3: [sdd] Discard mode: 2
sd 1:0:0:0: [sdb] Discard mode: 2
sd 1:0:0:0: [sdb] Entering discard switch via LBP VPD
sd 1:0:0:0: [sdb] Discard mode: 1
sd 1:0:0:0: [sdb] Discard mode: 2
sd 1:0:0:0: [sdb] Entering discard switch via LBP VPD
sd 1:0:0:0: [sdb] Discard mode: 1
sd 1:0:0:3: [sdd] Entering discard switch via LBP VPD
sd 1:0:0:3: [sdd] Discard mode: 1

All devices eventually end up using discard mode 1 (SD_LBP_UNMAP) but
it's a bit strange the VHDX devices (which set lbpme) flip back and
forth between discard mode 1 and discard mode 2 (SD_LBP_WS16)...

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Hannes Reinecke]

On 07/22/2014 01:06 AM, K. Y. Srinivasan wrote:
> Add blist flags to permit the reading of the VPD pages even when
> the target may claim SPC-2 compliance. MSFT targets currently
> claim SPC-2 compliance while they implement post SPC-2 features.
> With this patch we can correctly handle WRITE_SAME_16 issues.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>   drivers/scsi/storvsc_drv.c |   10 ++++++++++
>   1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 29d0329..15ba695 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -327,6 +327,8 @@ MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size (bytes)");
>    */
>   static int storvsc_timeout = 180;
>
> +static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
> +
>   #define STORVSC_MAX_IO_REQUESTS				200
>
>   static void storvsc_on_channel_callback(void *context);
> @@ -1449,6 +1451,14 @@ static int storvsc_device_configure(struct scsi_device *sdevice)
>
>   	sdevice->no_write_same = 1;
>
> +	/*
> +	 * Add blist flags to permit the reading of the VPD pages even when
> +	 * the target may claim SPC-2 compliance. MSFT targets currently
> +	 * claim SPC-2 compliance while they implement post SPC-2 features.
> +	 * With this patch we can correctly handle WRITE_SAME_16 issues.
> +	 */
> +	sdevice->sdev_bflags |= msft_blist_flags;
> +
>   	return 0;
>   }
>
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 1/1] Drivers: scsi: storvsc: Add blist flags

[Sitsofe Wheeler]

On Thu, Jul 24, 2014 at 07:40:36AM +0200, Hannes Reinecke wrote:
> On 07/22/2014 01:06 AM, K. Y. Srinivasan wrote:
> >Add blist flags to permit the reading of the VPD pages even when
> >the target may claim SPC-2 compliance. MSFT targets currently
> >claim SPC-2 compliance while they implement post SPC-2 features.
> >With this patch we can correctly handle WRITE_SAME_16 issues.
> >
> >Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> >
> Reviewed-by: Hannes Reinecke <hare@suse.de>

On Wed, Jul 23, 2014 at 09:13:41PM +0100, Sitsofe Wheeler wrote:
> On Wed, Jul 23, 2014 at 07:15:58AM -0700, Christoph Hellwig wrote:
> > On Wed, Jul 23, 2014 at 03:10:28PM +0100, Sitsofe Wheeler wrote:
> > > I'm not sure this alone will work - won't sdev_bflags/bflags have
> > > already been built at this point?
> > 
> > They've been built up, but we can still or new values into it.  It looks
> > fine to me from review, but if you can test it on an actualy hypverv
> > setup that would be valueable feedback.
> 
> The previous patches didn't work for me with a Windows 2012 R2 host with a
> 3.16.0-rc6.x86_64-00076-g2f7d2ec-dirty guest. After applying
> https://patchwork.kernel.org/patch/4541201 (which needed a small fixup) and
> https://patchwork.kernel.org/patch/4598601 and the attached debugging output

I've tested f3cfabce7a2e92564d380de3aad4b43901fb7ae6 (Drivers: add blist
flags) from the drivers-for-3.17 branch of scsi-queue
(http://git.infradead.org/users/hch/scsi-queue.git/commit/f3cfabce7a2e92564d380de3aad4b43901fb7ae6
) and this patch still doesn't appear to work (thin provisioning on
Hyper-V's Virtual Disks is not enabled):

# sg_inq  /dev/sda
standard INQUIRY:
  PQual=0  Device_type=0  RMB=0  version=0x04  [SPC-2]
  [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
  SCCS=0  ACC=0  TPGS=0  3PC=0  Protect=0  [BQue=0]
  EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
  [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
    length=36 (0x24)   Peripheral device type: disk
 Vendor identification: Msft    
 Product identification: Virtual Disk    
 Product revision level: 1.0 
# sg_vpd -p lbpv /dev/sda
Logical block provisioning VPD page (SBC):
  Unmap command supported (LBPU): 1
  Write same (16) with unmap bit supported (LBWS): 0
  Write same (10) with unmap bit supported (LBWS10): 0
  Logical block provisioning read zeros (LBPRZ): 0
  Anchored LBAs supported (ANC_SUP): 0
  Threshold exponent: 1
  Descriptor present (DP): 0
  Provisioning type: 2

grep . /sys/block/sd*/device/scsi_disk/*/{thin,prov,rotat,device/model}* | sort
/sys/block/sda/device/scsi_disk/0:0:0:0/device/model:Virtual Disk    
/sys/block/sda/device/scsi_disk/0:0:0:0/provisioning_mode:full
/sys/block/sda/device/scsi_disk/0:0:0:0/thin_provisioning:0
/sys/block/sdb/device/scsi_disk/1:0:0:0/device/model:Virtual Disk    
/sys/block/sdb/device/scsi_disk/1:0:0:0/provisioning_mode:full
/sys/block/sdb/device/scsi_disk/1:0:0:0/thin_provisioning:0
/sys/block/sdc/device/scsi_disk/1:0:0:2/device/model:00AAKS-00TMA0   
/sys/block/sdc/device/scsi_disk/1:0:0:2/provisioning_mode:full
/sys/block/sdc/device/scsi_disk/1:0:0:2/thin_provisioning:0
/sys/block/sdd/device/scsi_disk/1:0:0:1/device/model:SSD S510 120GB  
/sys/block/sdd/device/scsi_disk/1:0:0:1/provisioning_mode:full
/sys/block/sdd/device/scsi_disk/1:0:0:1/thin_provisioning:0
/sys/block/sde/device/scsi_disk/1:0:0:3/device/model:Virtual Disk    
/sys/block/sde/device/scsi_disk/1:0:0:3/provisioning_mode:full
/sys/block/sde/device/scsi_disk/1:0:0:3/thin_provisioning:0
/sys/block/sdf/device/scsi_disk/1:0:0:4/device/model:ST1000DM003-9YN1
/sys/block/sdf/device/scsi_disk/1:0:0:4/provisioning_mode:full
/sys/block/sdf/device/scsi_disk/1:0:0:4/thin_provisioning:0

At least the virtual disks should have thin_provisioning... I was also
expecting the passthrough SSD to be reported as non-rotational with this patch:

# sg_inq /dev/sdd
standard INQUIRY:
  PQual=0  Device_type=0  RMB=0  version=0x00  [no conformance claimed]
  [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
  SCCS=0  ACC=0  TPGS=0  3PC=0  Protect=0  [BQue=0]
  EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
  [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
  [SPI: Clocking=0x0  QAS=0  IUS=0]
    length=60 (0x3c)   Peripheral device type: disk
 Vendor identification: ADATA   
 Product identification: SSD S510 120GB
 Product revision level: 5.2.
 Unit serial number: 03205115500300002076
# sg_vpd -p bdc /dev/sdd
Block device characteristics VPD page (SBC):
  Non-rotating medium (e.g. solid state)
  Product type: Not specified
  WABEREQ=0
  WACEREQ=0
  Nominal form factor not reported
  FUAB=0
  VBULS=0
# grep -H . /sys/block/sdd/queue/rot*
/sys/block/sdd/queue/rotational:1

-- 
Sitsofe | http://sucs.org/~sits/