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

[Damien Le Moal <dlemoal@kernel.org>]

On 6/13/25 23:31, John Garry wrote:
> On 13/06/2025 07:29, 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.
> 
> Out of curiosity, why do this just for sd.c and not always set up the 
> default like this in blk_validate_limits()?

Good point. Though I think we do not want to have a large io_opt for slow
devices like MMC/SD Cards. So something like this, which is indeed simpler than
hacking lim->io_opt in sd.c.

diff --git a/block/blk-settings.c b/block/blk-settings.c
index a000daafbfb4..d3ec6f4100f4 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -58,16 +58,24 @@ EXPORT_SYMBOL(blk_set_stacking_limits);
 void blk_apply_bdi_limits(struct backing_dev_info *bdi,
                struct queue_limits *lim)
 {
+       u64 io_opt = lim->io_opt;
+
        /*
         * For read-ahead of large files to be effective, we need to read ahead
-        * at least twice the optimal I/O size.
+        * at least twice the optimal I/O size. For rotational devices that do
+        * not report an optimal I/O size (e.g. ATA HDDs), use the maximum I/O
+        * size to avoid falling back to the (rather inefficient) small default
+        * read-ahead size.
         *
         * There is no hardware limitation for the read-ahead size and the user
         * might have increased the read-ahead size through sysfs, so don't ever
         * decrease it.
         */
+       if (!io_opt && (lim->features & BLK_FEAT_ROTATIONAL))
+               io_opt = lim->max_sectors;
+
        bdi->ra_pages = max3(bdi->ra_pages,
-                               lim->io_opt * 2 / PAGE_SIZE,
+                               io_opt * 2 >> PAGE_SHIFT,
                                VM_READAHEAD_PAGES);
        bdi->io_pages = lim->max_sectors >> PAGE_SECTORS_SHIFT;
 }

I will make a proper patch of this and send it out as a replacement.

-- 
Damien Le Moal
Western Digital Research