[PATCH] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[PATCH] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

This patch fixes an issue that the following error happens on
swiotlb environment:

	xhci-hcd ee000000.usb: swiotlb buffer is full (sz: 524288 bytes), total 32768 (slots), used 1338 (slots)

On the kernel v5.1, block settings of a usb-storage with SuperSpeed
were the following so that the block layer will allocate buffers
up to 64 KiB, and then the issue didn't happen.

	max_segment_size = 65536
	max_hw_sectors_kb = 1024

After the commit 09324d32d2a0 ("block: force an unlimited segment
size on queues with a virt boundary") is applied, the block settings
are the following. So, the block layer will allocate buffers up to
1024 KiB, and then the issue happens:

	max_segment_size = 4294967295
	max_hw_sectors_kb = 1024

To fix the issue, the usb-storage driver checks the maximum size of
a mapping for the device and then adjusts the max_hw_sectors_kb
if required. After this patch is applied, the block settings will
be the following, and then the issue doesn't happen.

	max_segment_size = 4294967295
	max_hw_sectors_kb = 256

Fixes: 09324d32d2a0 ("block: force an unlimited segment size on queues with a virt boundary")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 We investigated this issue on the following ML:
 https://marc.info/?l=linux-block&m=156033909218970&w=2

 drivers/usb/storage/scsiglue.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 59190d8..89c3640 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -28,6 +28,8 @@
  * status of a command.
  */
 
+#include <linux/blkdev.h>
+#include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 
@@ -99,6 +101,7 @@ static int slave_alloc (struct scsi_device *sdev)
 static int slave_configure(struct scsi_device *sdev)
 {
 	struct us_data *us = host_to_us(sdev->host);
+	unsigned int max_sectors = 0;	/* 0 means no update required */
 
 	/*
 	 * Many devices have trouble transferring more than 32KB at a time,
@@ -106,26 +109,34 @@ static int slave_configure(struct scsi_device *sdev)
 	 * are limiting both to 32K (64 sectores).
 	 */
 	if (us->fflags & (US_FL_MAX_SECTORS_64 | US_FL_MAX_SECTORS_MIN)) {
-		unsigned int max_sectors = 64;
+		max_sectors = 64;
 
 		if (us->fflags & US_FL_MAX_SECTORS_MIN)
 			max_sectors = PAGE_SIZE >> 9;
-		if (queue_max_hw_sectors(sdev->request_queue) > max_sectors)
-			blk_queue_max_hw_sectors(sdev->request_queue,
-					      max_sectors);
+		if (queue_max_hw_sectors(sdev->request_queue) <= max_sectors)
+			max_sectors = 0;
 	} 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);
+		max_sectors = 0x7FFFFF;
 	} else if (us->pusb_dev->speed >= USB_SPEED_SUPER) {
 		/*
 		 * USB3 devices will be limited to 2048 sectors. This gives us
 		 * better throughput on most devices.
 		 */
-		blk_queue_max_hw_sectors(sdev->request_queue, 2048);
+		max_sectors = 2048;
+	}
+
+	if (max_sectors > 0) {
+		struct device *dev = us->pusb_dev->bus->sysdev;
+		size_t max_dma_sectors = dma_max_mapping_size(dev) >>
+					 SECTOR_SHIFT;
+
+		max_sectors = min_t(size_t, max_sectors, max_dma_sectors);
+		blk_queue_max_hw_sectors(sdev->request_queue, max_sectors);
 	}
 
 	/*
-- 
2.7.4

Re: [PATCH] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Christoph Hellwig]

> +	if (max_sectors > 0) {
> +		struct device *dev = us->pusb_dev->bus->sysdev;
> +		size_t max_dma_sectors = dma_max_mapping_size(dev) >>
> +					 SECTOR_SHIFT;
> +
> +		max_sectors = min_t(size_t, max_sectors, max_dma_sectors);
> +		blk_queue_max_hw_sectors(sdev->request_queue, max_sectors);

I think we need to do this unconditionally for the rare (or maybe even
theoretical case) of a dma max mapping size smaller than the default
max_sectos.

So something like this:

	blk_queue_max_hw_sectors(sdev->request_queue,
		min_t(unsigned long,
			queue_max_hw_sectors(sdev->request_queue),
			dma_max_mapping_size(dev) >> SECTOR_SHIFT));

RE: [PATCH] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

Hi Christoph,

> From: Christoph Hellwig, Sent: Thursday, June 13, 2019 4:34 PM
> 
> > +	if (max_sectors > 0) {
> > +		struct device *dev = us->pusb_dev->bus->sysdev;
> > +		size_t max_dma_sectors = dma_max_mapping_size(dev) >>
> > +					 SECTOR_SHIFT;
> > +
> > +		max_sectors = min_t(size_t, max_sectors, max_dma_sectors);
> > +		blk_queue_max_hw_sectors(sdev->request_queue, max_sectors);
> 
> I think we need to do this unconditionally for the rare (or maybe even
> theoretical case) of a dma max mapping size smaller than the default
> max_sectos.
> 
> So something like this:
> 
> 	blk_queue_max_hw_sectors(sdev->request_queue,
> 		min_t(unsigned long,
> 			queue_max_hw_sectors(sdev->request_queue),
> 			dma_max_mapping_size(dev) >> SECTOR_SHIFT));

I believe this patch I sent has already covered it. What do you think?

For examples (all value units are "sectors"):
	default	mapping size	max_sectors
case 1	240	MAX		2048		--> we use 2048
case 2	240	512		2048		--> we use 512
case 3	240	128		2048		--> we use 128
case 4	240	128		64		--> we use 64

Best regards,
Yoshihiro Shimoda

Re: [PATCH] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Christoph Hellwig]

On Thu, Jun 13, 2019 at 08:46:00AM +0000, Yoshihiro Shimoda wrote:
> I believe this patch I sent has already covered it. What do you think?
> 
> For examples (all value units are "sectors"):
> 	default	mapping size	max_sectors
> case 1	240	MAX		2048		--> we use 2048
> case 2	240	512		2048		--> we use 512
> case 3	240	128		2048		--> we use 128
> case 4	240	128		64		--> we use 64

Yes, I guess your version is fine after all:

Reviewed-by: Christoph Hellwig <hch@lst.de>

although I think it might be simpler to just read the value back
from the queue in the end.

RE: [PATCH] usb-storage: Add a limitation for blk_queue_max_hw_sectors()

[Yoshihiro Shimoda]

Hi Christoph,

> From: Christoph Hellwig, Sent: Thursday, June 13, 2019 5:51 PM
> 
> On Thu, Jun 13, 2019 at 08:46:00AM +0000, Yoshihiro Shimoda wrote:
> > I believe this patch I sent has already covered it. What do you think?
> >
> > For examples (all value units are "sectors"):
> > 	default	mapping size	max_sectors
> > case 1	240	MAX		2048		--> we use 2048
> > case 2	240	512		2048		--> we use 512
> > case 3	240	128		2048		--> we use 128
> > case 4	240	128		64		--> we use 64
> 
> Yes, I guess your version is fine after all:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thank you for your review!

> although I think it might be simpler to just read the value back
> from the queue in the end.

Ah, now I understand why you suggest it.
I agree with you. This patch changed 23 lines. But, it will change a few lines only.
So, I'll make such a patch.

Best regards,
Yoshihiro Shimoda