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

[PATCH v2] 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). This fallback io_opt limit avoids the disk to
be setup with the rather small 128 KB for as the read_ahead_kb
attribute. The larger read_ahead_kb value set with the default 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>
---
Changes from v1:
 - Changed message level from wrong WARNING level to INFO level
 - Added review tag

 drivers/scsi/sd.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index daddef2e9e87..92d5b4900c0e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3681,6 +3681,32 @@ 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;
+
+	lim->io_opt = shost->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));
+	if (!lim->io_opt) {
+		lim->io_opt = ALIGN_DOWN(lim->max_sectors << SECTOR_SHIFT,
+					 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 +3803,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 v2] scsi: sd: Set a default optimal IO size if one is not defined

[Bart Van Assche]

On 6/11/25 5:29 AM, Damien Le Moal wrote:
> +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;
> +
> +	lim->io_opt = shost->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));
> +	if (!lim->io_opt) {
> +		lim->io_opt = ALIGN_DOWN(lim->max_sectors << SECTOR_SHIFT,
> +					 sdkp->physical_block_size - 1);
> +		sd_first_printk(KERN_INFO, sdkp,
> +			"Using default optimal transfer size of %u bytes\n",
> +			lim->io_opt);
> +	}
> +}

shost->opt_sectors and lim->max_sectors both have type unsigned int so
when shifting these left there is a risk of overflow. As an example,
from the scsi_debug driver:

	.max_sectors =		-1U,

Shouldn't integer overflows be prevented instead of letting these
happen?

Thanks,

Bart.

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

[Damien Le Moal]

On 6/12/25 00:35, Bart Van Assche wrote:
> On 6/11/25 5:29 AM, Damien Le Moal wrote:
>> +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;
>> +
>> +	lim->io_opt = shost->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));
>> +	if (!lim->io_opt) {
>> +		lim->io_opt = ALIGN_DOWN(lim->max_sectors << SECTOR_SHIFT,
>> +					 sdkp->physical_block_size - 1);
>> +		sd_first_printk(KERN_INFO, sdkp,
>> +			"Using default optimal transfer size of %u bytes\n",
>> +			lim->io_opt);
>> +	}
>> +}
> 
> shost->opt_sectors and lim->max_sectors both have type unsigned int so
> when shifting these left there is a risk of overflow. As an example,
> from the scsi_debug driver:
> 
> 	.max_sectors =		-1U,

The code already was like that for opt_sectors and no-one complained.

> Shouldn't integer overflows be prevented instead of letting these
> happen?

Sure, can do that.


-- 
Damien Le Moal
Western Digital Research