[PATCH] [SCSI] sd: workaround invalid OPTIMAL TRANSFER LENGTH

[PATCH] [SCSI] sd: workaround invalid OPTIMAL TRANSFER LENGTH

[Mike Snitzer]

Workaround disk firmware that improperly sets OPTIMAL TRANSFER LENGTH to
0xFFFFFFFF (aka UINT_MAX or 4294967295U) by assuming this _optional_
BLOCK LIMITS VPD field was not specified (0).

It should be noted that a disk firmware fix is being developed but that
these disks are already in the wild (and when optimal_io_size is so
large it prevents standard Linux distribution installers, e.g. Anaconda,
from creating partitions smaller than 4GB via libparted due to parted
looking to align the created partitions relative to optimal_io_size).

The disk model in this instance is "SEAGATE ST900MM0006".  The BLOCK
LIMITS VPD page for this disk is:
# sg_inq --vpd --page=0xb0 /dev/sdb
VPD INQUIRY: Block limits page (SBC)
  Optimal transfer length granularity: 1 blocks
  Maximum transfer length: 0 blocks
  Optimal transfer length: 4294967295 blocks
  Maximum prefetch, xdread, xdwrite transfer length: 0 blocks
  Maximum unmap LBA count: 0
  Maximum unmap block descriptor count: 0
  Optimal unmap granularity: 0
  Unmap granularity alignment valid: 0
  Unmap granularity alignment: 0

Before this fix:
# cat /sys/block/sdb/queue/optimal_io_size
4294966784

After this fix:
# cat /sys/block/sdb/queue/optimal_io_size
0

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7992635..34638c1 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2516,6 +2516,7 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
  */
 static void sd_read_block_limits(struct scsi_disk *sdkp)
 {
+	unsigned int optimal_transfer_length;
 	unsigned int sector_sz = sdkp->device->sector_size;
 	const int vpd_len = 64;
 	unsigned char *buffer = kmalloc(vpd_len, GFP_KERNEL);
@@ -2527,8 +2528,11 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 	blk_queue_io_min(sdkp->disk->queue,
 			 get_unaligned_be16(&buffer[6]) * sector_sz);
+	optimal_transfer_length = get_unaligned_be32(&buffer[12]);
+	if (optimal_transfer_length == UINT_MAX)
+		optimal_transfer_length = 0; /* firmware bug, use 0 instead */
 	blk_queue_io_opt(sdkp->disk->queue,
-			 get_unaligned_be32(&buffer[12]) * sector_sz);
+			 optimal_transfer_length * sector_sz);
 
 	if (buffer[3] == 0x3c) {
 		unsigned int lba_count, desc_count;

Re: [PATCH] [SCSI] sd: workaround invalid OPTIMAL TRANSFER LENGTH

[Ewan Milne]

On Wed, 2013-04-03 at 16:21 -0400, Mike Snitzer wrote:
> Workaround disk firmware that improperly sets OPTIMAL TRANSFER LENGTH to
> 0xFFFFFFFF (aka UINT_MAX or 4294967295U) by assuming this _optional_
> BLOCK LIMITS VPD field was not specified (0).
> 

Acked-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH] [SCSI] sd: workaround invalid OPTIMAL TRANSFER LENGTH

[Martin K. Petersen]

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

Mike,

Following up on our discussion at LSF/MM last week...

Mike> Workaround disk firmware that improperly sets OPTIMAL TRANSFER
Mike> LENGTH to 0xFFFFFFFF (aka UINT_MAX or 4294967295U) by assuming
Mike> this _optional_ BLOCK LIMITS VPD field was not specified (0).

My only real objection is that UINT_MAX appears to be completely
coincidental. What if the next drive specifies UINT_MAX-1 or $RANDOM?

Lacking a decent heuristic I'd rather just blacklist the offending drive
using the skip_vpd_pages knob we already have in place.

-- 
Martin K. Petersen	Oracle Linux Engineering


[SCSI] Workaround for disks that report bad optimal transfer length

Not all disks fill out the VPD pages correctly. Add a blacklist flag
that allows us ignore the SBC-3 VPD pages for a given device. The
BLIST_SKIP_VPD_PAGES flag triggers our existing skip_vpd_pages
scsi_device parameter to bypass VPD scanning.

Also blacklist the offending Seagate drive model.

Reported-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>


diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 43fca91..f969aca 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -228,6 +228,7 @@ static struct {
 	{"SanDisk", "ImageMate CF-SD1", NULL, BLIST_FORCELUN},
 	{"SEAGATE", "ST34555N", "0930", BLIST_NOTQ},	/* Chokes on tagged INQUIRY */
 	{"SEAGATE", "ST3390N", "9546", BLIST_NOTQ},
+	{"SEAGATE", "ST900MM0006", NULL, BLIST_SKIP_VPD_PAGES},
 	{"SGI", "RAID3", "*", BLIST_SPARSELUN},
 	{"SGI", "RAID5", "*", BLIST_SPARSELUN},
 	{"SGI", "TP9100", "*", BLIST_REPORTLUN2},
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3e58b22..b9e39e0 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -924,6 +924,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (*bflags & BLIST_NO_DIF)
 		sdev->no_dif = 1;
 
+	if (*bflags & BLIST_SKIP_VPD_PAGES)
+		sdev->skip_vpd_pages = 1;
+
 	transport_configure_device(&sdev->sdev_gendev);
 
 	if (sdev->host->hostt->slave_configure) {
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index cc1f3e7..447d2d7 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -31,4 +31,5 @@
 #define BLIST_MAX_512		0x800000 /* maximum 512 sector cdb length */
 #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 */
 #endif

Re: sd: workaround invalid OPTIMAL TRANSFER LENGTH

[Mike Snitzer]

On Wed, Apr 24 2013 at  9:19pm -0400,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> Mike,
> 
> Following up on our discussion at LSF/MM last week...
> 
> Mike> Workaround disk firmware that improperly sets OPTIMAL TRANSFER
> Mike> LENGTH to 0xFFFFFFFF (aka UINT_MAX or 4294967295U) by assuming
> Mike> this _optional_ BLOCK LIMITS VPD field was not specified (0).
> 
> My only real objection is that UINT_MAX appears to be completely
> coincidental. What if the next drive specifies UINT_MAX-1 or $RANDOM?
> 
> Lacking a decent heuristic I'd rather just blacklist the offending drive
> using the skip_vpd_pages knob we already have in place.

OK, sounds fine to me, thanks.