[PATCH] sd: always scan VPD pages if thin provisioning is enabled

[PATCH] sd: always scan VPD pages if thin provisioning is enabled

[Hannes Reinecke]

For any device with an older SCSI revision we might not
be scanning VPD pages, which results in a wrongly configured
discard mode if thin provisioned is enabled.
According to sbc3 any thin provisioned device (ie devices which
have the LBPME bit set in the output of READ CAPACITY(16)) need
to support VPD pages. So this patch always enables VPD pages
even for older SCSI revisions if thin provisioning is enabled.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/sd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1fbb1ec..08963b0 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2169,7 +2169,15 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 		if (buffer[14] & 0x40) /* LBPRZ */
 			sdkp->lbprz = 1;
 
-		sd_config_discard(sdkp, SD_LBP_WS16);
+		/*
+		 * sbc3r36 states:
+		 * The device server in a logical unit the supports
+		 * logical block provisioning management shall support
+		 * the Logical Block Provisioning VPD page.
+		 * So VPD pages should be supported if lbpme is set.
+		 */
+		if (!scsi_device_supports_vpd(sdp))
+			sdp->try_vpd_pages = 1;
 	}
 
 	sdkp->capacity = lba + 1;
-- 
1.8.5.6

Re: [PATCH] sd: always scan VPD pages if thin provisioning is enabled

[Johannes Thumshirn]

On Wed, Jan 25, 2017 at 08:26:05AM +0100, Hannes Reinecke wrote:
> For any device with an older SCSI revision we might not
> be scanning VPD pages, which results in a wrongly configured
> discard mode if thin provisioned is enabled.
> According to sbc3 any thin provisioned device (ie devices which
> have the LBPME bit set in the output of READ CAPACITY(16)) need
> to support VPD pages. So this patch always enables VPD pages
> even for older SCSI revisions if thin provisioning is enabled.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---


Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] sd: always scan VPD pages if thin provisioning is enabled

[Christoph Hellwig]

On Wed, Jan 25, 2017 at 08:26:05AM +0100, Hannes Reinecke wrote:
> For any device with an older SCSI revision we might not
> be scanning VPD pages, which results in a wrongly configured
> discard mode if thin provisioned is enabled.
> According to sbc3 any thin provisioned device (ie devices which
> have the LBPME bit set in the output of READ CAPACITY(16)) need
> to support VPD pages. So this patch always enables VPD pages
> even for older SCSI revisions if thin provisioning is enabled.

Can you explain what you need this for?  A device with a per-SBC3
revision that wants us to use UNMAP?

> +		/*
> +		 * sbc3r36 states:
> +		 * The device server in a logical unit the supports
> +		 * logical block provisioning management shall support
> +		 * the Logical Block Provisioning VPD page.
> +		 * So VPD pages should be supported if lbpme is set.
> +		 */

It's a bit odd to quote SBC3 when the device clearly is pre-SBC3
to need this workaround..

> +		if (!scsi_device_supports_vpd(sdp))
> +			sdp->try_vpd_pages = 1;

Do the assignment unconditionally?

Re: [PATCH] sd: always scan VPD pages if thin provisioning is enabled

[Hannes Reinecke]

On 01/25/2017 11:23 AM, Christoph Hellwig wrote:
> On Wed, Jan 25, 2017 at 08:26:05AM +0100, Hannes Reinecke wrote:
>> For any device with an older SCSI revision we might not
>> be scanning VPD pages, which results in a wrongly configured
>> discard mode if thin provisioned is enabled.
>> According to sbc3 any thin provisioned device (ie devices which
>> have the LBPME bit set in the output of READ CAPACITY(16)) need
>> to support VPD pages. So this patch always enables VPD pages
>> even for older SCSI revisions if thin provisioning is enabled.
> 
> Can you explain what you need this for?  A device with a per-SBC3
> revision that wants us to use UNMAP?
> 
Some storage arrays essentially lie about the SCSI revision (most
notably Hitachi :-), and some claim to support SPC-2 (or even SPC) but
support newer features, too. Most notably VPD pages support.
In this case it was an HP EVA claiming to support SPC-2 only, but
providing thin provisioning.

>> +		/*
>> +		 * sbc3r36 states:
>> +		 * The device server in a logical unit the supports
>> +		 * logical block provisioning management shall support
>> +		 * the Logical Block Provisioning VPD page.
>> +		 * So VPD pages should be supported if lbpme is set.
>> +		 */
> 
> It's a bit odd to quote SBC3 when the device clearly is pre-SBC3
> to need this workaround..
> 
_Actually_ it's pre-SPC-3.

But that was the earliest draft I had :-(
I'd be happy to modify this if I had access to sbc-2 drafts.

>> +		if (!scsi_device_supports_vpd(sdp))
>> +			sdp->try_vpd_pages = 1;
> 
> Do the assignment unconditionally?
> 
Yeah, can do.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] sd: always scan VPD pages if thin provisioning is enabled

[Ewan D. Milne]

On Wed, 2017-01-25 at 11:38 +0100, Hannes Reinecke wrote:
> On 01/25/2017 11:23 AM, Christoph Hellwig wrote:
> > On Wed, Jan 25, 2017 at 08:26:05AM +0100, Hannes Reinecke wrote:
> >> For any device with an older SCSI revision we might not
> >> be scanning VPD pages, which results in a wrongly configured
> >> discard mode if thin provisioned is enabled.
> >> According to sbc3 any thin provisioned device (ie devices which
> >> have the LBPME bit set in the output of READ CAPACITY(16)) need
> >> to support VPD pages. So this patch always enables VPD pages
> >> even for older SCSI revisions if thin provisioning is enabled.
> > 
> > Can you explain what you need this for?  A device with a per-SBC3
> > revision that wants us to use UNMAP?
> > 
> Some storage arrays essentially lie about the SCSI revision (most
> notably Hitachi :-), and some claim to support SPC-2 (or even SPC) but
> support newer features, too. Most notably VPD pages support.
> In this case it was an HP EVA claiming to support SPC-2 only, but
> providing thin provisioning.

Um, isn't this why we added:

commit c1d40a527e885a40bb9ea6c46a1b1145d42b66a0
Author: Martin K. Petersen <martin.petersen@oracle.com>
Date:   Tue Jul 15 12:49:17 2014 -0400

    scsi: add a blacklist flag which enables VPD page inquiries

(well, it was for storvsc, but we could add an entry for the HP EVA)

> >> +		/*
> >> +		 * sbc3r36 states:
> >> +		 * The device server in a logical unit the supports
> >> +		 * logical block provisioning management shall support
> >> +		 * the Logical Block Provisioning VPD page.
> >> +		 * So VPD pages should be supported if lbpme is set.
> >> +		 */
> > 
> > It's a bit odd to quote SBC3 when the device clearly is pre-SBC3
> > to need this workaround..
> > 
> _Actually_ it's pre-SPC-3.
> 
> But that was the earliest draft I had :-(
> I'd be happy to modify this if I had access to sbc-2 drafts.
> 
> >> +		if (!scsi_device_supports_vpd(sdp))
> >> +			sdp->try_vpd_pages = 1;
> > 
> > Do the assignment unconditionally?
> > 
> Yeah, can do.
> 
> Cheers,
> 
> Hannes

Re: [PATCH] sd: always scan VPD pages if thin provisioning is enabled

[Hannes Reinecke]

On 01/25/2017 03:27 PM, Ewan D. Milne wrote:
> On Wed, 2017-01-25 at 11:38 +0100, Hannes Reinecke wrote:
>> On 01/25/2017 11:23 AM, Christoph Hellwig wrote:
>>> On Wed, Jan 25, 2017 at 08:26:05AM +0100, Hannes Reinecke wrote:
>>>> For any device with an older SCSI revision we might not
>>>> be scanning VPD pages, which results in a wrongly configured
>>>> discard mode if thin provisioned is enabled.
>>>> According to sbc3 any thin provisioned device (ie devices which
>>>> have the LBPME bit set in the output of READ CAPACITY(16)) need
>>>> to support VPD pages. So this patch always enables VPD pages
>>>> even for older SCSI revisions if thin provisioning is enabled.
>>>
>>> Can you explain what you need this for?  A device with a per-SBC3
>>> revision that wants us to use UNMAP?
>>>
>> Some storage arrays essentially lie about the SCSI revision (most
>> notably Hitachi :-), and some claim to support SPC-2 (or even SPC) but
>> support newer features, too. Most notably VPD pages support.
>> In this case it was an HP EVA claiming to support SPC-2 only, but
>> providing thin provisioning.
> 
> Um, isn't this why we added:
> 
> commit c1d40a527e885a40bb9ea6c46a1b1145d42b66a0
> Author: Martin K. Petersen <martin.petersen@oracle.com>
> Date:   Tue Jul 15 12:49:17 2014 -0400
> 
>     scsi: add a blacklist flag which enables VPD page inquiries
> 
> (well, it was for storvsc, but we could add an entry for the HP EVA)
> 
I knew someone would raise this objection :-)

Thing is, setting 'WS16' here is arguably wrong, as LPBME just means
'logical block provisioning management enabled', not 'WRITE SAME 16 with
UNMAP' supported.

And we've set the restriction for scanning VPD pages rather high by
moving it to at least SPC-3; meaning we lose out on all SPC-2 devices
with logical block provisioning.

So rather than blacklisting each and every device (and incurring loads
of customer calls) I'd rather fix it once and for all.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] sd: always scan VPD pages if thin provisioning is enabled

[James Bottomley]

On Wed, 2017-01-25 at 15:38 +0100, Hannes Reinecke wrote:
> On 01/25/2017 03:27 PM, Ewan D. Milne wrote:
> > On Wed, 2017-01-25 at 11:38 +0100, Hannes Reinecke wrote:
> > > On 01/25/2017 11:23 AM, Christoph Hellwig wrote:
> > > > On Wed, Jan 25, 2017 at 08:26:05AM +0100, Hannes Reinecke
> > > > wrote:
> > > > > For any device with an older SCSI revision we might not
> > > > > be scanning VPD pages, which results in a wrongly configured
> > > > > discard mode if thin provisioned is enabled.
> > > > > According to sbc3 any thin provisioned device (ie devices 
> > > > > which have the LBPME bit set in the output of READ 
> > > > > CAPACITY(16)) need to support VPD pages. So this patch always 
> > > > > enables VPD pages even for older SCSI revisions if thin 
> > > > > provisioning is enabled.
> > > > 
> > > > Can you explain what you need this for?  A device with a per
> > > > -SBC3 revision that wants us to use UNMAP?
> > > > 
> > > Some storage arrays essentially lie about the SCSI revision (most
> > > notably Hitachi :-), and some claim to support SPC-2 (or even 
> > > SPC) but support newer features, too. Most notably VPD pages 
> > > support. In this case it was an HP EVA claiming to support SPC-2 
> > > only, but providing thin provisioning.
> > 
> > Um, isn't this why we added:
> > 
> > commit c1d40a527e885a40bb9ea6c46a1b1145d42b66a0
> > Author: Martin K. Petersen <martin.petersen@oracle.com>
> > Date:   Tue Jul 15 12:49:17 2014 -0400
> > 
> >     scsi: add a blacklist flag which enables VPD page inquiries
> > 
> > (well, it was for storvsc, but we could add an entry for the HP
> > EVA)
> > 
> I knew someone would raise this objection :-)
> 
> Thing is, setting 'WS16' here is arguably wrong, as LPBME just means
> 'logical block provisioning management enabled', not 'WRITE SAME 16 
> with UNMAP' supported.
> 
> And we've set the restriction for scanning VPD pages rather high by
> moving it to at least SPC-3; meaning we lose out on all SPC-2 devices
> with logical block provisioning.
> 
> So rather than blacklisting each and every device (and incurring 
> loads of customer calls) I'd rather fix it once and for all.

Anything with a capacity over 2TB gets into RC16 .,. that includes a
lot of USB storage nowadays.  What would this proposed addition do to
them?

James

Re: [PATCH] sd: always scan VPD pages if thin provisioning is enabled

[Hannes Reinecke]

On 01/25/2017 04:38 PM, James Bottomley wrote:
> On Wed, 2017-01-25 at 15:38 +0100, Hannes Reinecke wrote:
>> On 01/25/2017 03:27 PM, Ewan D. Milne wrote:
>>> On Wed, 2017-01-25 at 11:38 +0100, Hannes Reinecke wrote:
>>>> On 01/25/2017 11:23 AM, Christoph Hellwig wrote:
>>>>> On Wed, Jan 25, 2017 at 08:26:05AM +0100, Hannes Reinecke
>>>>> wrote:
>>>>>> For any device with an older SCSI revision we might not
>>>>>> be scanning VPD pages, which results in a wrongly configured
>>>>>> discard mode if thin provisioned is enabled.
>>>>>> According to sbc3 any thin provisioned device (ie devices
>>>>>> which have the LBPME bit set in the output of READ
>>>>>> CAPACITY(16)) need to support VPD pages. So this patch always
>>>>>> enables VPD pages even for older SCSI revisions if thin
>>>>>> provisioning is enabled.
>>>>>
>>>>> Can you explain what you need this for?  A device with a per
>>>>> -SBC3 revision that wants us to use UNMAP?
>>>>>
>>>> Some storage arrays essentially lie about the SCSI revision (most
>>>> notably Hitachi :-), and some claim to support SPC-2 (or even
>>>> SPC) but support newer features, too. Most notably VPD pages
>>>> support. In this case it was an HP EVA claiming to support SPC-2
>>>> only, but providing thin provisioning.
>>>
>>> Um, isn't this why we added:
>>>
>>> commit c1d40a527e885a40bb9ea6c46a1b1145d42b66a0
>>> Author: Martin K. Petersen <martin.petersen@oracle.com>
>>> Date:   Tue Jul 15 12:49:17 2014 -0400
>>>
>>>     scsi: add a blacklist flag which enables VPD page inquiries
>>>
>>> (well, it was for storvsc, but we could add an entry for the HP
>>> EVA)
>>>
>> I knew someone would raise this objection :-)
>>
>> Thing is, setting 'WS16' here is arguably wrong, as LPBME just means
>> 'logical block provisioning management enabled', not 'WRITE SAME 16
>> with UNMAP' supported.
>>
>> And we've set the restriction for scanning VPD pages rather high by
>> moving it to at least SPC-3; meaning we lose out on all SPC-2 devices
>> with logical block provisioning.
>>
>> So rather than blacklisting each and every device (and incurring
>> loads of customer calls) I'd rather fix it once and for all.
>
> Anything with a capacity over 2TB gets into RC16 .,. that includes a
> lot of USB storage nowadays.  What would this proposed addition do to
> them?
>
Nothing.
We're latching off the 'LPBME' bit from the READ CAPACITY 16 
information, which (hopefully) isn't set.
And the 'skip_vpd_pages' as being set by the USB drives is evaluated at 
a later point, so they won't be affected.

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] sd: always scan VPD pages if thin provisioning is enabled

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes,

Hannes> Thing is, setting 'WS16' here is arguably wrong, as LPBME just
Hannes> means 'logical block provisioning management enabled', not
Hannes> 'WRITE SAME 16 with UNMAP' supported.

Things are the way they are because many devices predate the final SBC
spec. They report LBPME=1 but do not have the VPD page indicating their
preferred deprovisioning command. The VPD was added very late in the
spec and there are a ton of devices that don't have it. As a result, we
default to WS16 unless the device reports the UNMAP parameters in the
Block Limits VPD.

That has been a very solid heuristic for a long time so please don't
mess with that.

-- 
Martin K. Petersen	Oracle Linux Engineering