[PATCH] scsi: relax PAGE LENGTH check for thin provisioning UNMAP support

[PATCH] scsi: relax PAGE LENGTH check for thin provisioning UNMAP support

[Mike Snitzer]

Thin Provisioning fields are assumed available in the BLOCK LIMITS VPD
page if PAGE LENGTH is 0x3c.  The BLOCK LIMITS VPD page may be extended
over time.  Allow for the possibility that the PAGE LENGTH exceeds 0x3c.

Further details:
Linux SCSI defaults to using WRITE SAME(16) with UNMAP bit set for
discards.

If PAGE LENGTH is 0x3c and both MAXIMUM UNMAP LBA COUNT and MAXIMUM
UNMAP BLOCK DESCRIPTOR COUNT are set (in BLOCK LIMITS VPD page), Linux
SCSI will issue UNMAP instead of using the default (WRITE SAME(16) with
UNMAP bit set).

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/scsi/sd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8b827f3..40ab22e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1963,7 +1963,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 			 get_unaligned_be32(&buffer[12]) * sector_sz);
 
 	/* Thin provisioning enabled and page length indicates TP support */
-	if (sdkp->thin_provisioning && buffer[3] == 0x3c) {
+	if (sdkp->thin_provisioning && buffer[3] >= 0x3c) {
 		unsigned int lba_count, desc_count, granularity;
 
 		lba_count = get_unaligned_be32(&buffer[20]);
-- 
1.6.6.1

Re: [PATCH] scsi: relax PAGE LENGTH check for thin provisioning UNMAP support

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike> Thin Provisioning fields are assumed available in the BLOCK LIMITS
Mike> VPD page if PAGE LENGTH is 0x3c.  The BLOCK LIMITS VPD page may be
Mike> extended over time.  Allow for the possibility that the PAGE
Mike> LENGTH exceeds 0x3c.

SBC3 states that: 

"If the device server supports thin provisioning, then the device server
shall set the PAGE LENGTH field to 3Ch."

That's a "shall".  That's a non-negotiable requirement in standards
speak.  And besides, only half of that mandated page length is currently
spoken for.  So I wonder what motivated your change?  Do you have any
examples of devices with a bigger page length?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: relax PAGE LENGTH check for thin provisioning UNMAP support

[Mike Snitzer]

On Thu, May 6, 2010 at 12:28 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
>
> Mike> Thin Provisioning fields are assumed available in the BLOCK LIMITS
> Mike> VPD page if PAGE LENGTH is 0x3c.  The BLOCK LIMITS VPD page may be
> Mike> extended over time.  Allow for the possibility that the PAGE
> Mike> LENGTH exceeds 0x3c.
>
> SBC3 states that:
>
> "If the device server supports thin provisioning, then the device server
> shall set the PAGE LENGTH field to 3Ch."
>
> That's a "shall".  That's a non-negotiable requirement in standards
> speak.  And besides, only half of that mandated page length is currently
> spoken for.  So I wonder what motivated your change?  Do you have any
> examples of devices with a bigger page length?

I forgot to cc Fred Knight on my patch, but Fred was/is attending the
most recent T10.  Based on the discussions there, and me having shared
with him the PAGE LENGTH == 0x3c constraint before, he sent me private
mail sharing that SBC3 is not yet final nor approved.  And that at
this recent T10 meeting they were talking of extending the BLOCK
LIMITS VPD.  If not for SBC3 then for SBC4.

So coming full circle: why impose a precise PAGE LENGTH of 0x3c?
It'll only cause compatibility problems for us down the road when new
devices do come to market.

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] scsi: relax PAGE LENGTH check for thin provisioning UNMAP support

[Knight, Frederick]

In the early 90s, when the SCSI INQUIRY - ANSI Version field had only 2 values, I worked on a host that, because SCSI-1 and SCSI-2 were incompatible, did the following:

If ( ANSI_VERSION == 2 ) {use SCSI-2 protocol} else {use SCSI-1 protocol}

When the first SCSI-3 device came along, this host fell over dead because the SCSI-1 protocol did not work on that SCSI-3 device.  They had to delay support for that device until they fixed their code (change == to >=), and they could NOT support that device in older versions, even thought it would have worked if not for the above "bug".

There are no guarantees about what the T10 committee will do to that page in the draft, or in any future versions of SBC.

In reality, any value > 32 (20h) will mean that you probably have all the data you want to look at today.  YES, any device that puts such a small value into the page length field is non-compliant (and therefore, the contents of those fields may be non-compliant as well).  So, allowing a value less than 3Ch would be risky.

But, a compliant SBC-4/5/... device might put 200 (or other > 3Ch value) into that field, while the existing values all still remain valid.  It would be a shame to disable feature "A" just because a device added the implementation of feature "X".

Future proofing is never guaranteed, but it is good to at least try.

	Fred

-----Original Message-----
From: Mike Snitzer [mailto:snitzer@redhat.com] 
Sent: Thursday, May 06, 2010 7:24 AM
To: Martin K. Petersen
Cc: linux-scsi@vger.kernel.org; Christoph Hellwig; Knight, Frederick
Subject: Re: [PATCH] scsi: relax PAGE LENGTH check for thin provisioning UNMAP support

On Thu, May 6, 2010 at 12:28 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
>
> Mike> Thin Provisioning fields are assumed available in the BLOCK LIMITS
> Mike> VPD page if PAGE LENGTH is 0x3c.  The BLOCK LIMITS VPD page may be
> Mike> extended over time.  Allow for the possibility that the PAGE
> Mike> LENGTH exceeds 0x3c.
>
> SBC3 states that:
>
> "If the device server supports thin provisioning, then the device server
> shall set the PAGE LENGTH field to 3Ch."
>
> That's a "shall".  That's a non-negotiable requirement in standards
> speak.  And besides, only half of that mandated page length is currently
> spoken for.  So I wonder what motivated your change?  Do you have any
> examples of devices with a bigger page length?

I forgot to cc Fred Knight on my patch, but Fred was/is attending the
most recent T10.  Based on the discussions there, and me having shared
with him the PAGE LENGTH == 0x3c constraint before, he sent me private
mail sharing that SBC3 is not yet final nor approved.  And that at
this recent T10 meeting they were talking of extending the BLOCK
LIMITS VPD.  If not for SBC3 then for SBC4.

So coming full circle: why impose a precise PAGE LENGTH of 0x3c?
It'll only cause compatibility problems for us down the road when new
devices do come to market.

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: relax PAGE LENGTH check for thin provisioning UNMAP support

[Martin K. Petersen]

>>>>> "Fred" == Knight, Frederick <Frederick.Knight@netapp.com> writes:

Fred,

Fred> There are no guarantees about what the T10 committee will do to
Fred> that page in the draft, or in any future versions of SBC.

Obviously.  However, we're usually pretty good at tracking the drafts
and have a pretty quick turnaround for changes.


Fred> In reality, any value > 32 (20h) will mean that you probably have
Fred> all the data you want to look at today.  YES, any device that puts
Fred> such a small value into the page length field is non-compliant
Fred> (and therefore, the contents of those fields may be non-compliant
Fred> as well).  So, allowing a value less than 3Ch would be risky.

Our main concern is that we're dealing with a ton of low-end devices
that violate the SCSI protocols in the weirdest ways.  Some return the
same blob of data regardless of which VPD page you ask for.  So when we
have a mandated value we can key off of as an extra safeguard we tend to
use it.

In this particular case it might not be such a big deal since we're
already gated by TPE being enabled in READ CAPACITY(16).  The latter is
currently restricted by the device server claiming compliance with SPC3
or better.  However, we may have to relax that criteria soon since we'll
have USB drives coming out where READ CAPACITY(10) won't cut it.  And
that opens the flood gates for random debris lingering at byte 12 and
beyond in READ CAPACITY(16).

My main concern with Mike's patch is that I think it's a bit premature
given that there's already room to grow in the current block limits VPD
and that relaxing the page length test could cause us to accept random
garbage as valid thin provisioning information.

-- 
Martin K. Petersen	Oracle Linux Engineering