Re: usb-storage: Set virt_boundary_mask to avoid SG overflows

Re: usb-storage: Set virt_boundary_mask to avoid SG overflows

[Alan Stern]

Questions like this should always be CC'ed to the appropriate mailing 
list.  (And note that the mailing list does not accept messages in HTML 
format.)

On Thu, 29 Aug 2019, sandeep krishnaswamy wrote:

> Hi Alan,
> 
> 
> 
> We are using the Android 8.x (i.e Android ‘O’) with Android kernel 4.9.182
> (derived from open source Linux kernel 4.9.182)
> 
> 
> 
> Since Android does not support the NTFS by default, we have enabled NTFS
> support in Kernel & added changes in user space to mount NTFS USB drives
> (as part of Android Vold).
> 
> 
> 
> With the integration of K4.9.182, we have seen regression in read speed
> from NTFS mounted USB drive. FAT32 mounted USB drives read speeds are fine
> without any regression.
> 
> 
> 
> By reverting the commit -
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/usb/storage/scsiglue.c?h=v4.9.190&id=c685caf6e5d896472c67b348d23936c6dc479749
> , I could see read speed of NTFS mounted USB drives are fine.
> 
> 
> 
> Could you please help me to understand the commit and help me, how this
> commit could impact only on NTFS mounted USB drive but not with FAT32
> mounted USB drive ?

I have no idea how that commit could have affected read speeds for NTFS 
volumes.  Possibly it might cause the system to use unnecessary bounce 
buffers, but that seems unlikely to make such a large difference.

Furthermore, I now believe that the commit isn't necessary at all.  
We'll probably get rid of the blk_queue_virt_boundary() call in the
not-too-distant future.  If you want to understand better how it works,
you should ask people involved in maintaining the block layer.

Alan Stern

Re: usb-storage: Set virt_boundary_mask to avoid SG overflows

[Daniel Walker]

On Mon, Nov 04, 2019 at 01:45:11PM -0500, Alan Stern wrote:
> On Mon, 4 Nov 2019, Daniel Walker wrote:
> 
> > Hi,
> > 
> > This is a stable defect report.
> > 
> > We're tracking v4.9 stable for some of our products (i.e. Cisco Systems, Inc.)
> > We noticed a speed degradation of roughly %30 on writes to a /dev/sdaX device
> > over USB (no file system). We bisected the issue to this commit from Alan Stern.
> > We also found a prior report of speed degradation on NTFS,
> > 
> > https://lore.kernel.org/linux-usb/Pine.LNX.4.44L0.1908291030400.1306-100000@iolanthe.rowland.org/T/
> > 
> > We have the patch reverted in our v4.9 tree on top of stable. It seems Alan was
> > planning to remove these lines. If the lines are planned to be removed is there
> > an reason why they haven't been ?
> 
> See https://marc.info/?l=linux-usb&m=157167288816325&w=2
> 

Ok .. Thanks.

Daniel

Re: usb-storage: Set virt_boundary_mask to avoid SG overflows

[Alan Stern]

On Mon, 4 Nov 2019, Daniel Walker wrote:

> Hi,
> 
> This is a stable defect report.
> 
> We're tracking v4.9 stable for some of our products (i.e. Cisco Systems, Inc.)
> We noticed a speed degradation of roughly %30 on writes to a /dev/sdaX device
> over USB (no file system). We bisected the issue to this commit from Alan Stern.
> We also found a prior report of speed degradation on NTFS,
> 
> https://lore.kernel.org/linux-usb/Pine.LNX.4.44L0.1908291030400.1306-100000@iolanthe.rowland.org/T/
> 
> We have the patch reverted in our v4.9 tree on top of stable. It seems Alan was
> planning to remove these lines. If the lines are planned to be removed is there
> an reason why they haven't been ?

See https://marc.info/?l=linux-usb&m=157167288816325&w=2

Alan Stern

Re: usb-storage: Set virt_boundary_mask to avoid SG overflows

[Daniel Walker]

Hi,

This is a stable defect report.

We're tracking v4.9 stable for some of our products (i.e. Cisco Systems, Inc.)
We noticed a speed degradation of roughly %30 on writes to a /dev/sdaX device
over USB (no file system). We bisected the issue to this commit from Alan Stern.
We also found a prior report of speed degradation on NTFS,

https://lore.kernel.org/linux-usb/Pine.LNX.4.44L0.1908291030400.1306-100000@iolanthe.rowland.org/T/

We have the patch reverted in our v4.9 tree on top of stable. It seems Alan was
planning to remove these lines. If the lines are planned to be removed is there
an reason why they haven't been ?

Thanks,
Daniel

usb-storage: Set virt_boundary_mask to avoid SG overflows

[Alan Stern]

The USB subsystem has always had an unusual requirement for its
scatter-gather transfers: Each element in the scatterlist (except the
last one) must have a length divisible by the bulk maxpacket size.
This is a particular issue for USB mass storage, which uses SG lists
created by the block layer rather than setting up its own.

So far we have scraped by okay because most devices have a logical
block size of 512 bytes or larger, and the bulk maxpacket sizes for
USB 2 and below are all <= 512.  However, USB 3 has a bulk maxpacket
size of 1024.  Since the xhci-hcd driver includes native SG support,
this hasn't mattered much.  But now people are trying to use USB-3
mass storage devices with USBIP, and the vhci-hcd driver currently
does not have full SG support.

The result is an overflow error, when the driver attempts to implement
an SG transfer of 63 512-byte blocks as a single
3584-byte (7 blocks) transfer followed by seven 4096-byte (8 blocks)
transfers.  The device instead sends 31 1024-byte packets followed by
a 512-byte packet, and this overruns the first SG buffer.

Ideally this would be fixed by adding better SG support to vhci-hcd.
But for now it appears we can work around the problem by
asking the block layer to respect the maxpacket limitation, through
the use of the virt_boundary_mask.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Seth Bollinger <Seth.Bollinger@digi.com>
Tested-by: Seth Bollinger <Seth.Bollinger@digi.com>
CC: Ming Lei <tom.leiming@gmail.com>
---

uas should get a similar change.  Oliver said he would take care of 
it.


[as1886]


 drivers/usb/storage/scsiglue.c |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Index: usb-devel/drivers/usb/storage/scsiglue.c
===================================================================
--- usb-devel.orig/drivers/usb/storage/scsiglue.c
+++ usb-devel/drivers/usb/storage/scsiglue.c
@@ -65,6 +65,7 @@ static const char* host_info(struct Scsi
 static int slave_alloc (struct scsi_device *sdev)
 {
 	struct us_data *us = host_to_us(sdev->host);
+	int maxp;
 
 	/*
 	 * Set the INQUIRY transfer length to 36.  We don't use any of
@@ -74,20 +75,17 @@ static int slave_alloc (struct scsi_devi
 	sdev->inquiry_len = 36;
 
 	/*
-	 * USB has unusual DMA-alignment requirements: Although the
-	 * starting address of each scatter-gather element doesn't matter,
-	 * the length of each element except the last must be divisible
-	 * by the Bulk maxpacket value.  There's currently no way to
-	 * express this by block-layer constraints, so we'll cop out
-	 * and simply require addresses to be aligned at 512-byte
-	 * boundaries.  This is okay since most block I/O involves
-	 * hardware sectors that are multiples of 512 bytes in length,
-	 * and since host controllers up through USB 2.0 have maxpacket
-	 * values no larger than 512.
-	 *
-	 * But it doesn't suffice for Wireless USB, where Bulk maxpacket
-	 * values can be as large as 2048.  To make that work properly
-	 * will require changes to the block layer.
+	 * USB has unusual scatter-gather requirements: the length of each
+	 * scatterlist element except the last must be divisible by the
+	 * Bulk maxpacket value.  Fortunately this value is always a
+	 * power of 2.  Inform the block layer about this requirement.
+	 */
+	maxp = usb_maxpacket(us->pusb_dev, us->recv_bulk_pipe, 0);
+	blk_queue_virt_boundary(sdev->request_queue, maxp - 1);
+
+	/*
+	 * Some host controllers may have alignment requirements.
+	 * We'll play it safe by requiring 512-byte alignment always.
 	 */
 	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));