[PATCH v3 0/2] Improve optimal IO size initialization

[PATCH v3 0/2] Improve optimal IO size initialization

[Damien Le Moal]

A couple of patches to improve setting the optimal I/O size limit of
scsi disks. A fallback default is added to make sure we always have a
non-zero optimal I/O size so that file systems operate with a
reasonnably sized default read_ahead_kb value, for improving buffered
read performance.

Changes from v1:
 - Changed message level from wrong WARNING level to INFO level
 - Added review tag

Changes from v2:
 - Added patch 1
 - Make sure we do not overflow variables and limits in patch 2

Damien Le Moal (2):
  scsi: sd: Prevent logical_to_bytes() from returning overflowed values
  scsi: sd: Set a default optimal IO size if one is not defined

 drivers/scsi/sd.c | 45 +++++++++++++++++++++++++++++++++++----------
 drivers/scsi/sd.h |  2 +-
 2 files changed, 36 insertions(+), 11 deletions(-)

-- 
2.49.0

[PATCH v3 1/2] scsi: sd: Prevent logical_to_bytes() from returning overflowed values

[Damien Le Moal]

Make sure that logical_to_bytes() does not return an overflowed value
by changing its return type from unsigned int (32-bits) to size_t
(64-bits).

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/scsi/sd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 36382eca941c..3803eb8cb532 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -213,7 +213,7 @@ static inline sector_t logical_to_sectors(struct scsi_device *sdev, sector_t blo
 	return blocks << (ilog2(sdev->sector_size) - 9);
 }
 
-static inline unsigned int logical_to_bytes(struct scsi_device *sdev, sector_t blocks)
+static inline size_t logical_to_bytes(struct scsi_device *sdev, sector_t blocks)
 {
 	return blocks * sdev->sector_size;
 }
-- 
2.49.0

[PATCH v3 2/2] scsi: sd: Set a default optimal IO size if one is not defined

[Damien Le Moal]

Introduce the helper function sd_set_io_opt() to set a disk io_opt
limit. This new way of setting this limit falls back to using the
max_sectors limit if the host does not define an optimal sector limit
and the device did not indicate an optimal transfer size (e.g. as is
the case for ATA devices). io_opt calculation is done using a local
64-bits variable to avoid overflows. The final value is clamped to
UINT_MAX aligned down to the device physical block size.

This fallback io_opt limit avoids setting up the disk with a zero
io_opt limit, which result in the rather small 128 KB read_ahead_kb
attribute. The larger read_ahead_kb value set with the default non-zero
io_opt limit significantly improves buffered read performance with file
systems without any intervention from the user.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 drivers/scsi/sd.c | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index daddef2e9e87..8070356285a7 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3681,6 +3681,40 @@ static void sd_read_block_zero(struct scsi_disk *sdkp)
 	kfree(buffer);
 }
 
+/*
+ * Set the optimal I/O size: limit the default to the SCSI host optimal sector
+ * limit if it is set. There may be an impact on performance when the size of
+ * a request exceeds this host limit. If the host did not set any optimal
+ * sector limit and the device did not indicate an optimal transfer size
+ * (e.g. ATA devices), default to using the device max_sectors limit.
+ */
+static void sd_set_io_opt(struct scsi_disk *sdkp, unsigned int dev_max,
+			  struct queue_limits *lim)
+{
+	struct scsi_device *sdp = sdkp->device;
+	struct Scsi_Host *shost = sdp->host;
+	u64 io_opt;
+
+	io_opt = (u64)shost->opt_sectors << SECTOR_SHIFT;
+	if (sd_validate_opt_xfer_size(sdkp, dev_max))
+		io_opt = min_not_zero(io_opt,
+				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
+	if (io_opt) {
+		lim->io_opt = ALIGN_DOWN(min_t(u64, io_opt, UINT_MAX),
+					 sdkp->physical_block_size - 1);
+		return;
+	}
+
+	/* Set default */
+	io_opt = (u64)lim->max_sectors << SECTOR_SHIFT;
+	lim->io_opt = ALIGN_DOWN(min_t(u64, io_opt, UINT_MAX),
+				 sdkp->physical_block_size - 1);
+
+	sd_first_printk(KERN_INFO, sdkp,
+			"Using default optimal transfer size of %u bytes\n",
+			lim->io_opt);
+}
+
 /**
  *	sd_revalidate_disk - called the first time a new disk is seen,
  *	performs disk spin up, read_capacity, etc.
@@ -3777,16 +3811,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	else
 		lim.io_min = 0;
 
-	/*
-	 * Limit default to SCSI host optimal sector limit if set. There may be
-	 * an impact on performance for when the size of a request exceeds this
-	 * host limit.
-	 */
-	lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
-	if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
-		lim.io_opt = min_not_zero(lim.io_opt,
-				logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
-	}
+	sd_set_io_opt(sdkp, dev_max, &lim);
 
 	sdkp->first_scan = 0;
 
-- 
2.49.0

Re: [PATCH v3 1/2] scsi: sd: Prevent logical_to_bytes() from returning overflowed values

[Bart Van Assche]

On 6/11/25 11:02 PM, Damien Le Moal wrote:
> Make sure that logical_to_bytes() does not return an overflowed value
> by changing its return type from unsigned int (32-bits) to size_t
> (64-bits).

size_t is only 64 bits on 64-bit systems. Shouldn't size_t be changed
into u64? See also https://en.wikipedia.org/wiki/64-bit_computing.

> -static inline unsigned int logical_to_bytes(struct scsi_device *sdev, sector_t blocks)
> +static inline size_t logical_to_bytes(struct scsi_device *sdev, sector_t blocks)
>   {
>   	return blocks * sdev->sector_size;
>   }

Since 'blocks' represents an LBA instead of a byte offset divided by
512, please consider changing "sector_t blocks" into
"u64 logical_blocks". Independent of this patch, "sector_size" probably
should be renamed into "logical_block_size" since the word "sector" is
only used in references to "physical sector" in the SBC specification.

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH v3 2/2] scsi: sd: Set a default optimal IO size if one is not defined

[Bart Van Assche]

On 6/11/25 11:02 PM, Damien Le Moal wrote:
> Introduce the helper function sd_set_io_opt() to set a disk io_opt
> limit. This new way of setting this limit falls back to using the
> max_sectors limit if the host does not define an optimal sector limit
> and the device did not indicate an optimal transfer size (e.g. as is
> the case for ATA devices). io_opt calculation is done using a local
> 64-bits variable to avoid overflows. The final value is clamped to
> UINT_MAX aligned down to the device physical block size.
> 
> This fallback io_opt limit avoids setting up the disk with a zero
> io_opt limit, which result in the rather small 128 KB read_ahead_kb
> attribute. The larger read_ahead_kb value set with the default non-zero
> io_opt limit significantly improves buffered read performance with file
> systems without any intervention from the user.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v3 1/2] scsi: sd: Prevent logical_to_bytes() from returning overflowed values

[Damien Le Moal]

On 6/13/25 00:53, Bart Van Assche wrote:
> On 6/11/25 11:02 PM, Damien Le Moal wrote:
>> Make sure that logical_to_bytes() does not return an overflowed value
>> by changing its return type from unsigned int (32-bits) to size_t
>> (64-bits).
> 
> size_t is only 64 bits on 64-bit systems. Shouldn't size_t be changed
> into u64? See also https://en.wikipedia.org/wiki/64-bit_computing.
> 
>> -static inline unsigned int logical_to_bytes(struct scsi_device *sdev, sector_t blocks)
>> +static inline size_t logical_to_bytes(struct scsi_device *sdev, sector_t blocks)
>>   {
>>   	return blocks * sdev->sector_size;
>>   }
> 
> Since 'blocks' represents an LBA instead of a byte offset divided by
> 512, please consider changing "sector_t blocks" into
> "u64 logical_blocks". Independent of this patch, "sector_size" probably
> should be renamed into "logical_block_size" since the word "sector" is
> only used in references to "physical sector" in the SBC specification.

Well, one could argue that struct scsi_device is SPC territory, not SBC, and
that "sector_size" or block size has no business being in struct scsi_device.
But changing that would not make access to that value easy when all we have is a
scsi command :)

> 
> Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.


-- 
Damien Le Moal
Western Digital Research