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/16/25 14:34, Damien Le Moal wrote:
> 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;

Oops... This should of course be:

		io_opt = (u64)lim->max_sectors << SECTOR_SHIFT;

> +
>         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