On Mon, May 22, 2023 at 03:25:35PM -0700, Bart Van Assche wrote:
> Allow block drivers to configure the following:
> * Maximum number of hardware sectors values smaller than
> PAGE_SIZE >> SECTOR_SHIFT. For PAGE_SIZE = 4096 this means that values
> below 8 become supported.
> * A maximum segment size below the page size. This is most useful
> for page sizes above 4096 bytes.
>
> The blk_sub_page_segments static branch will be used in later patches to
> prevent that performance of block drivers that support segments >=
> PAGE_SIZE and max_hw_sectors >= PAGE_SIZE >> SECTOR_SHIFT would be affected.
>
> This patch may change the behavior of existing block drivers from not
> working into working.
That's quite an understatement.
> If a block driver calls
> blk_queue_max_hw_sectors() or blk_queue_max_segment_size(), this is
> usually done to configure the maximum supported limits. An attempt to
> configure a limit below what is supported by the block layer causes the
> block layer to select a larger value. If that value is not supported by
> the block driver, this may cause other data to be transferred than
> requested, a kernel crash or other undesirable behavior.
Right, which in the worst case could expose a firmware bug or whatever.
So I see this as a critical fix too. And it gets me wondering what has
happened for 512 byte controllers on 4K PAGE_SIZE?
> + * blk_enable_sub_page_limits - enable support for max_segment_size values smaller than PAGE_SIZE and for max_hw_sectors values below PAGE_SIZE >> SECTOR_SHIFT
Length, 100 is an exception. I'm sticking to the old school 80.
> + * blk_disable_sub_page_limits - disable support for max_segment_size values smaller than PAGE_SIZE and for max_hw_sectors values below PAGE_SIZE >> SECTOR_SHIFT
Same.
> @@ -126,6 +173,11 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto
> unsigned int min_max_hw_sectors = PAGE_SIZE >> SECTOR_SHIFT;
> unsigned int max_sectors;
>
> + if (max_hw_sectors < min_max_hw_sectors) {
> + blk_enable_sub_page_limits(limits);
> + min_max_hw_sectors = 1;
> + }
> +
> if (max_hw_sectors < min_max_hw_sectors) {
> max_hw_sectors = min_max_hw_sectors;
> pr_info("%s: set to minimum %u\n", __func__, max_hw_sectors);
It would seem like max_dev_sectors would have saved the day too,
but that is said to be set by the "disk" on the documentation.
I see scsi/sd.c and drivers/s390/block/dasd_*.c set this too,
is that a layering violation, or was that to help perhaps with
similar problems? If not could stroage controller have used this
for this issue as well?
Could the documentation for blk_queue_max_hw_sectors() be enhanced to
clarify this?
The way I'm thinking about this is, if this is a fix for stable too,
what would a minimum safe stable fix be like? And then after whatever
we need to make it better (non stable fixes).
Luis
