diff mbox series

[08/14] scsi: sd: Optimal I/O size should be a multiple of reported granularity

Message ID 20220302053559.32147-9-martin.petersen@oracle.com
State New
Headers show
Series [01/14] scsi: mpt3sas: Use cached ATA Information VPD page | expand

Commit Message

Martin K. Petersen March 2, 2022, 5:35 a.m. UTC
Commit a83da8a4509d ("scsi: sd: Optimal I/O size should be a multiple of
physical block size") validated the reported optimal I/O size against the
physical block size to overcome problems with devices reporting nonsensical
transfer sizes.

However, some devices claim conformity to older SCSI versions that predate
the physical block size being reported. Other devices do not report a
physical block size at all. We need to be able to validate the optimal I/O
size on those devices as well.

Many devices report an OPTIMAL TRANSFER LENGTH GRANULARITY in the same VPD
page as the OPTIMAL TRANSFER LENGTH. Use this value to validate the optimal
I/O size. Also check that the reported granularity is a multiple of the
physical block size, if supported.

Link: https://lore.kernel.org/r/33fb522e-4f61-1b76-914f-c9e6a3553c9b@gmail.com
Reported-by: Bernhard Sulzer <micraft.b@gmail.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/sd.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 drivers/scsi/sd.h |  1 +
 2 files changed, 42 insertions(+), 4 deletions(-)

Comments

Bart Van Assche March 3, 2022, 8:17 p.m. UTC | #1
On 3/1/22 21:35, Martin K. Petersen wrote:
> +	if (min_xfer_bytes && opt_xfer_bytes & (min_xfer_bytes - 1)) {
> +		sd_first_printk(KERN_WARNING, sdkp,
> +				"Optimal transfer size %u bytes not a " \
> +				"multiple of preferred minimum block " \
> +				"size (%u bytes)\n",
> +				opt_xfer_bytes, min_xfer_bytes);
> +		return false;
> +	}

Hmm ... what guarantees that min_xfer_bytes is a power of two? Did I perhaps 
overlook something?

Thanks,

Bart.
Martin K. Petersen March 4, 2022, 3:45 a.m. UTC | #2
Bart,

> On 3/1/22 21:35, Martin K. Petersen wrote:
>> +	if (min_xfer_bytes && opt_xfer_bytes & (min_xfer_bytes - 1)) {
>> +		sd_first_printk(KERN_WARNING, sdkp,
>> +				"Optimal transfer size %u bytes not a " \
>> +				"multiple of preferred minimum block " \
>> +				"size (%u bytes)\n",
>> +				opt_xfer_bytes, min_xfer_bytes);
>> +		return false;
>> +	}
>
> Hmm ... what guarantees that min_xfer_bytes is a power of two? Did I
> perhaps overlook something?

Nothing in the spec, obviously, but I think we'd have a lot of headaches
further up the stack if that wasn't the case.

Do you know of any devices that report something unusual here?
Bart Van Assche March 4, 2022, 5:06 a.m. UTC | #3
On 3/3/22 19:45, Martin K. Petersen wrote:
>> On 3/1/22 21:35, Martin K. Petersen wrote:
>>> +	if (min_xfer_bytes && opt_xfer_bytes & (min_xfer_bytes - 1)) {
>>> +		sd_first_printk(KERN_WARNING, sdkp,
>>> +				"Optimal transfer size %u bytes not a " \
>>> +				"multiple of preferred minimum block " \
>>> +				"size (%u bytes)\n",
>>> +				opt_xfer_bytes, min_xfer_bytes);
>>> +		return false;
>>> +	}
>>
>> Hmm ... what guarantees that min_xfer_bytes is a power of two? Did I
>> perhaps overlook something?
> 
> Nothing in the spec, obviously, but I think we'd have a lot of headaches
> further up the stack if that wasn't the case.
> 
> Do you know of any devices that report something unusual here?

Hi Martin,

I'm not aware of any devices that report an unusual OPTIMAL TRANSFER LENGTH 
GRANULARITY value.

Since this code is not in the hot path, how about using the modulo operator 
instead of binary and?

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 84c04691841f..8595c4f2e915 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2871,7 +2871,6 @@  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 sector_sz = sdkp->device->sector_size;
 	struct scsi_vpd *vpd;
 
 	rcu_read_lock();
@@ -2880,9 +2879,7 @@  static void sd_read_block_limits(struct scsi_disk *sdkp)
 	if (!vpd || vpd->len < 16)
 		goto out;
 
-	blk_queue_io_min(sdkp->disk->queue,
-			 get_unaligned_be16(&vpd->data[6]) * sector_sz);
-
+	sdkp->min_xfer_blocks = get_unaligned_be16(&vpd->data[6]);
 	sdkp->max_xfer_blocks = get_unaligned_be32(&vpd->data[8]);
 	sdkp->opt_xfer_blocks = get_unaligned_be32(&vpd->data[12]);
 
@@ -3140,6 +3137,29 @@  static void sd_read_cpr(struct scsi_disk *sdkp)
 	kfree(buffer);
 }
 
+static bool sd_validate_min_xfer_size(struct scsi_disk *sdkp)
+{
+	struct scsi_device *sdp = sdkp->device;
+	unsigned int min_xfer_bytes =
+		logical_to_bytes(sdp, sdkp->min_xfer_blocks);
+
+	if (sdkp->min_xfer_blocks == 0)
+		return false;
+
+	if (min_xfer_bytes & (sdkp->physical_block_size - 1)) {
+		sd_first_printk(KERN_WARNING, sdkp,
+				"Preferred minimum I/O size %u bytes not a " \
+				"multiple of physical block size (%u bytes)\n",
+				min_xfer_bytes, sdkp->physical_block_size);
+		sdkp->min_xfer_blocks = 0;
+		return false;
+	}
+
+	sd_first_printk(KERN_INFO, sdkp, "Preferred minimum I/O size %u bytes\n",
+			min_xfer_bytes);
+	return true;
+}
+
 /*
  * Determine the device's preferred I/O size for reads and writes
  * unless the reported value is unreasonably small, large, not a
@@ -3151,6 +3171,8 @@  static bool sd_validate_opt_xfer_size(struct scsi_disk *sdkp,
 	struct scsi_device *sdp = sdkp->device;
 	unsigned int opt_xfer_bytes =
 		logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
+	unsigned int min_xfer_bytes =
+		logical_to_bytes(sdp, sdkp->min_xfer_blocks);
 
 	if (sdkp->opt_xfer_blocks == 0)
 		return false;
@@ -3179,6 +3201,15 @@  static bool sd_validate_opt_xfer_size(struct scsi_disk *sdkp,
 		return false;
 	}
 
+	if (min_xfer_bytes && opt_xfer_bytes & (min_xfer_bytes - 1)) {
+		sd_first_printk(KERN_WARNING, sdkp,
+				"Optimal transfer size %u bytes not a " \
+				"multiple of preferred minimum block " \
+				"size (%u bytes)\n",
+				opt_xfer_bytes, min_xfer_bytes);
+		return false;
+	}
+
 	if (opt_xfer_bytes & (sdkp->physical_block_size - 1)) {
 		sd_first_printk(KERN_WARNING, sdkp,
 				"Optimal transfer size %u bytes not a " \
@@ -3271,6 +3302,12 @@  static int sd_revalidate_disk(struct gendisk *disk)
 	dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
 	q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max);
 
+	if (sd_validate_min_xfer_size(sdkp))
+		blk_queue_io_min(sdkp->disk->queue,
+				 logical_to_bytes(sdp, sdkp->min_xfer_blocks));
+	else
+		blk_queue_io_min(sdkp->disk->queue, 0);
+
 	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
 		q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
 		rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 2e5932bde43d..f4fbca90e997 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -91,6 +91,7 @@  struct scsi_disk {
 	atomic_t	openers;
 	sector_t	capacity;	/* size in logical blocks */
 	int		max_retries;
+	u32		min_xfer_blocks;
 	u32		max_xfer_blocks;
 	u32		opt_xfer_blocks;
 	u32		max_ws_blocks;