scsi: sd_zbc: Fix sd_zbc_report_zones() buffer allocation

Commit Message

Damien Le Moal Feb. 13, 2019, 4:37 a.m. UTC

From: Masato Suzuki <[e-mail]>

The function sd_zbc_do_report_zones() issues a REPORT ZONES command
with a buffer size calculated based on the number of zones requested
by the caller. This value should however not exceed the capabilities
of the hardware maximum command size, that is, should not exceed the
max_sectors limit of the device. This problem leads to failures of
report zones commands when re-validating disks with some SAS HBAs.

Fix this by limiting a report zone command buffer size to the minimum
of the device max_sectors and calculated value based on the requested
number of zones. This does not change the semantic of the report_zones
file operation as report zones can always return less zone reports
than requested. Short reports are handled using a loop execution of
the report_zones file operation in the function blk_report_zones().

[Damien]
Before commit 'e76239a3748c ("block: add a report_zones method")',
report zones buffer allocation was limited to max_sectors when
allocated in blk_report_zones(). This however does not consider the
actual format of the device reply which is interface dependent.
Limiting the allocation based on the expected reply format rather than
the generic struct blkzone size expected by blk_report_zones() makes
more sense.

Fixes: e76239a3748c ("block: add a report_zones method")
Cc: [e-mail]
Signed-off-by: Damien Le Moal <[e-mail]>
Signed-off-by: Masato Suzuki <[e-mail]>
---
 drivers/scsi/sd_zbc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Martin K. Petersen Feb. 14, 2019, 3:22 a.m. UTC | #1

Damien,

> +	buflen = min(queue_max_sectors(disk->queue) << 9,
> +		     roundup((nrz + 1) * 64, 512));

Shouldn't this be queue_max_hw_sectors()?

max_sectors is the soft limit for filesystem reads and writes.

max_hw_sectors is the per-command maximum data transfer supported by the
controller.

Damien Le Moal Feb. 14, 2019, 4:17 a.m. UTC | #2

On 2019/02/14 12:22, Martin K. Petersen wrote:
> 
> Damien,
> 
>> +	buflen = min(queue_max_sectors(disk->queue) << 9,
>> +		     roundup((nrz + 1) * 64, 512));
> 
> Shouldn't this be queue_max_hw_sectors()?
> 
> max_sectors is the soft limit for filesystem reads and writes.
> 
> max_hw_sectors is the per-command maximum data transfer supported by the
> controller.

Indeed, it should be.

Checking again with the problematic HBA (smartpqi), max_sectors_kb is 1024 and
so is max_hw_sectors_kb. Interestingly, with this HBA, max_sectors_kb cannot be
changed to a value larger than 1024, which seems weird since max_segments is 257
and max_segment_size is 64K. I guess the driver wants to guarantee that any I/O
can always be mapped with fragmented 4K pages.

On most other HBAs I have, max_sectors_kb default to 1280, that is,
BLK_DEF_MAX_SECTORS and max_hw_sectors_kb is several megabytes (16 for SAS and
32 for SATA), which is plenty for even very large blkdev_report_zones()
requests. But the value given with smartpqi is too small for even the report
zones calls from blk_revalidate_zones().

Updating and resending.

Thanks !

Patch

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index fff86940388b..fa75603a4090 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -142,10 +142,12 @@  int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 		return -EOPNOTSUPP;
 
 	/*
-	 * Get a reply buffer for the number of requested zones plus a header.
-	 * For ATA, buffers must be aligned to 512B.
+	 * Get a reply buffer for the number of requested zones plus a header,
+	 * without exceeding the device maximum command size. For ATA disks,
+	 * buffers must be aligned to 512B.
 	 */
-	buflen = roundup((nrz + 1) * 64, 512);
+	buflen = min(queue_max_sectors(disk->queue) << 9,
+		     roundup((nrz + 1) * 64, 512));
 	buf = kmalloc(buflen, gfp_mask);
 	if (!buf)
 		return -ENOMEM;