Re: [PATCH] fat: Avoid oops when bdi->io_pages==0

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Matthew Wilcox <willy@infradead.org> writes:

> On Sun, Aug 30, 2020 at 10:54:35AM +0900, OGAWA Hirofumi wrote:
>> Matthew Wilcox <willy@infradead.org> writes:
>> 
>> Hm, io_pages is limited by driver setting too, and io_pages can be lower
>> than ra_pages, e.g. usb storage.
>> 
>> Assuming ra_pages is user intent of readahead window. So if io_pages is
>> lower than ra_pages, this try ra_pages to align of io_pages chunk, but
>> not bigger than ra_pages. Because if block layer splits I/O requests to
>> hard limit, then I/O is not optimal.
>> 
>> So it is intent, I can be misunderstanding though.
>
> Looking at this some more, I'm not sure it makes sense to consult ->io_pages
> at all.  I see how it gets set to 0 -- the admin can write '1' to
> /sys/block/<device>/queue/max_sectors_kb and that gets turned into 0
> in ->io_pages.

	if (max_sectors_kb > max_hw_sectors_kb || max_sectors_kb < page_kb)
		return -EINVAL;

It should not set to 0 via /sys/.../max_sectors_kb. However the default
of bdi->io_pages is 0. So it happened if a driver didn't initialized it.

> But I'm not sure it makes any sense to respect that.  Looking at
> mm/readahead.c, all it does is limit the size of a read request which
> exceeds the current readahead window.  It's not used to limit the
> readahead window itself.  For example:
>
>         unsigned long max_pages = ra->ra_pages;
> ...
>         if (req_size > max_pages && bdi->io_pages > max_pages)
>                 max_pages = min(req_size, bdi->io_pages);
>
> Setting io_pages below ra_pages has no effect.  So maybe fat should also
> disregard it?

          |-----------------------| requested blocks
[before]
 ra_pages |===========|===========|===========|
 io_pages |---------|---------|---------|
 req      |---------|-|-------|---|

[after]
 ra_pages |=========|=========|=========|
 io_pages |---------|---------|---------|
 req      |---------|---------|---|

This path is known the large sequential read there. Well, anyway, this
intent is to use [after] as 3 req, instead of [before] as 4 req.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>