diff mbox

Issues with commit 34b48db6 ("block: remove artifical max_hw_sectors cap")

Message ID Pine.LNX.4.44L0.1412301109050.32416-100000@netrider.rowland.org
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Alan Stern Dec. 30, 2014, 4:12 p.m. UTC 
On Tue, 30 Dec 2014, James Bottomley wrote:

> > _Is_ there any way to communicate the maximum transfer size?  I'm not
> > aware of any SCSI command for it.  It isn't part of the USB
> > mass-storage spec.
> 
> For the device, it's in the Block limits VPD page.  However, what the
> device supports isn't necessarily what the bridge or host bus adapter
> will support.  We need to set the limit to the lowest of what the
> device, the bridge and the HBA support.  We know the device (provided
> the bridge allows VPD inquiries ... not all do) and host, so we really
> need to know what the bridge will support.  From the error it does look
> like we're running into a bridge limit.

I see.

> > usb-storage has no clear idea what sort of device lies on the other
> > side of the USB bridge.  It might be an ATA drive, it might be a flash
> > drive, it might not be a disk at all -- usb-storage does its best not
> > to know or care.
> 
> That's fine, but is there any way in USB to query the bridge to get it's
> transfer characteristics?

No, there isn't.  The only query that a USB mass-storage bridge accepts 
is for the maximum LUN value.

> > If you think that usb-storage needs to set a maximum transfer size for
> > disk drives, it won't be hard to write a patch.  But what about all the
> > other possible transports?  Will they each have to implement the same
> > transfer limit?  If so, shouldn't the limit be set up from a more
> > central location, such as the sd driver?
> 
> This isn't a transport problem, this is a bridge problem.  T10 has
> always recognised there might be a bridge issue linking two transports,
> so it did initially come up with a bridge spec (BCC) but it was
> abandoned a decade ago in favour of transparent bridges (every switch in
> a FC topology is effectively a transparent bridge) or making them
> explicit in the standards, like SAS expanders.
> 
> > Why not have sd always set max_sectors_kb to 32767 if it isn't already 
> > smaller?  Would that cause any problems?
> 
> This wouldn't be sd ... we have lots of requirements for large transfer
> sizes for efficiency.  It has to be the layer that knows there's a
> bridge, so that would make it usb.

All right.  How does this patch look?

Alan Stern




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

Comments

James Bottomley Dec. 30, 2014, 4:25 p.m. UTC | #1 
On Tue, 2014-12-30 at 11:12 -0500, Alan Stern wrote:
> On Tue, 30 Dec 2014, James Bottomley wrote:
> 
> > > _Is_ there any way to communicate the maximum transfer size?  I'm not
> > > aware of any SCSI command for it.  It isn't part of the USB
> > > mass-storage spec.
> > 
> > For the device, it's in the Block limits VPD page.  However, what the
> > device supports isn't necessarily what the bridge or host bus adapter
> > will support.  We need to set the limit to the lowest of what the
> > device, the bridge and the HBA support.  We know the device (provided
> > the bridge allows VPD inquiries ... not all do) and host, so we really
> > need to know what the bridge will support.  From the error it does look
> > like we're running into a bridge limit.
> 
> I see.
> 
> > > usb-storage has no clear idea what sort of device lies on the other
> > > side of the USB bridge.  It might be an ATA drive, it might be a flash
> > > drive, it might not be a disk at all -- usb-storage does its best not
> > > to know or care.
> > 
> > That's fine, but is there any way in USB to query the bridge to get it's
> > transfer characteristics?
> 
> No, there isn't.  The only query that a USB mass-storage bridge accepts 
> is for the maximum LUN value.
> 
> > > If you think that usb-storage needs to set a maximum transfer size for
> > > disk drives, it won't be hard to write a patch.  But what about all the
> > > other possible transports?  Will they each have to implement the same
> > > transfer limit?  If so, shouldn't the limit be set up from a more
> > > central location, such as the sd driver?
> > 
> > This isn't a transport problem, this is a bridge problem.  T10 has
> > always recognised there might be a bridge issue linking two transports,
> > so it did initially come up with a bridge spec (BCC) but it was
> > abandoned a decade ago in favour of transparent bridges (every switch in
> > a FC topology is effectively a transparent bridge) or making them
> > explicit in the standards, like SAS expanders.
> > 
> > > Why not have sd always set max_sectors_kb to 32767 if it isn't already 
> > > smaller?  Would that cause any problems?
> > 
> > This wouldn't be sd ... we have lots of requirements for large transfer
> > sizes for efficiency.  It has to be the layer that knows there's a
> > bridge, so that would make it usb.
> 
> All right.  How does this patch look?

OK, I suppose.  The transfer limits are a little on the low side, but
for usb-storage (i.e. non-UAS) performance devices, they should be OK.
For TYPE_TAPE, you still have no guarantee that the bridge won't screw
up ... and if the argument is that tapes are always connected to
sensible bridges, why aren't SATA devices?

There's also a spelling mistake below.

> Alan Stern
> 
> 
> 
> Index: usb-3.18/drivers/usb/storage/scsiglue.c
> ===================================================================
> --- usb-3.18.orig/drivers/usb/storage/scsiglue.c
> +++ usb-3.18/drivers/usb/storage/scsiglue.c
> @@ -114,26 +114,30 @@ static int slave_alloc (struct scsi_devi
>  static int slave_configure(struct scsi_device *sdev)
>  {
>  	struct us_data *us = host_to_us(sdev->host);
> +	unsigned int max_sectors;
>  
> -	/* Many devices have trouble transferring more than 32KB at a time,
> -	 * while others have trouble with more than 64K. At this time we
> -	 * are limiting both to 32K (64 sectores).
> +	/*
> +	 * Many devices have trouble transferring more than 32 KB at a time,
> +	 * while others have trouble with more than 64 KB. At this time we
> +	 * are limiting both to 32 KB (64 sectores).

sectors

James



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Dec. 30, 2014, 4:45 p.m. UTC | #2 
On Tue, 30 Dec 2014, James Bottomley wrote:

> > All right.  How does this patch look?
> 
> OK, I suppose.  The transfer limits are a little on the low side, but
> for usb-storage (i.e. non-UAS) performance devices, they should be OK.

If you're referring to the 32-KB and 64-KB limits, we know from 
experience that some devices really do need them.  If you're referring 
to the 32-MB limit...  Well, that's what this whole thread is about, 
right?

That limit could be restricted to apply only when a device is not 
connected over a SuperSpeed USB-3 link.  But knowing the low quality of 
commodity USB hardware, I suspect it wouldn't work well.

> For TYPE_TAPE, you still have no guarantee that the bridge won't screw
> up ... and if the argument is that tapes are always connected to
> sensible bridges, why aren't SATA devices?

True, we have no guarantee.  But tape drives do have special 
requirements because of the way they write blocks and gaps; this code 
was added by someone with a tape drive who did need the large limit.

I guess there are a lot more bridges in the low-budget consumer world 
targeted to disk drives than to tape drives.  That could explain a lot.

> There's also a spelling mistake below.

I'll fix it in the final patch submission.  Thanks.

Alan Stern

PS: What's the current situation of my "SCSI: fix regression in 
scsi_send_eh_cmnd()" patch:

	http://marc.info/?l=linux-scsi&m=141658469207765&w=2

submitted on November 21?  Since it was a bug-fix, I rather expected it 
to get merged before 3.18 was released.  Since it didn't, I certainly 
hope it will get in before 3.19.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley Dec. 30, 2014, 4:54 p.m. UTC | #3 
On Tue, 2014-12-30 at 11:45 -0500, Alan Stern wrote:

> PS: What's the current situation of my "SCSI: fix regression in 
> scsi_send_eh_cmnd()" patch:
> 
> 	http://marc.info/?l=linux-scsi&m=141658469207765&w=2
> 
> submitted on November 21?  Since it was a bug-fix, I rather expected it 
> to get merged before 3.18 was released.  Since it didn't, I certainly 
> hope it will get in before 3.19.

Looks like it just got overlooked.  Since Hannes has reviewed it, I'll
add it to the fixes branch.

James



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

Patch

Index: usb-3.18/drivers/usb/storage/scsiglue.c
===================================================================
--- usb-3.18.orig/drivers/usb/storage/scsiglue.c
+++ usb-3.18/drivers/usb/storage/scsiglue.c
@@ -114,26 +114,30 @@  static int slave_alloc (struct scsi_devi
 static int slave_configure(struct scsi_device *sdev)
 {
 	struct us_data *us = host_to_us(sdev->host);
+	unsigned int max_sectors;
 
-	/* Many devices have trouble transferring more than 32KB at a time,
-	 * while others have trouble with more than 64K. At this time we
-	 * are limiting both to 32K (64 sectores).
+	/*
+	 * Many devices have trouble transferring more than 32 KB at a time,
+	 * while others have trouble with more than 64 KB. At this time we
+	 * are limiting both to 32 KB (64 sectores).
+	 * Still other devices have trouble unless the transfer size is as
+	 * small as possible (one memory page).
+	 *
+	 * Tape drives need much higher max_sector limits, so just
+	 * raise it to the maximum possible (4 GB / 512) and let the
+	 * queue segment size sort out the real limit.
+	 * For safety, limit all other devices to 32 MB transfer size.
 	 */
-	if (us->fflags & (US_FL_MAX_SECTORS_64 | US_FL_MAX_SECTORS_MIN)) {
-		unsigned int max_sectors = 64;
-
-		if (us->fflags & US_FL_MAX_SECTORS_MIN)
-			max_sectors = PAGE_CACHE_SIZE >> 9;
-		if (queue_max_hw_sectors(sdev->request_queue) > max_sectors)
-			blk_queue_max_hw_sectors(sdev->request_queue,
-					      max_sectors);
-	} else if (sdev->type == TYPE_TAPE) {
-		/* Tapes need much higher max_sector limits, so just
-		 * raise it to the maximum possible (4 GB / 512) and
-		 * let the queue segment size sort out the real limit.
-		 */
-		blk_queue_max_hw_sectors(sdev->request_queue, 0x7FFFFF);
-	}
+	if (us->fflags & US_FL_MAX_SECTORS_MIN)
+		max_sectors = PAGE_CACHE_SIZE >> 9;
+	else if (us->fflags & US_FL_MAX_SECTORS_64)
+		max_sectors = 64;
+	else if (sdev->type == TYPE_TAPE)
+		max_sectors = 0x7FFFFF;
+	else
+		max_sectors = 65535;
+	if (queue_max_hw_sectors(sdev->request_queue) > max_sectors)
+		blk_queue_max_hw_sectors(sdev->request_queue, max_sectors);
 
 	/* Some USB host controllers can't do DMA; they have to use PIO.
 	 * They indicate this by setting their dma_mask to NULL.  For