On Mon, Jan 29, 2024 at 04:15:16PM +1100, Dave Chinner wrote:

On Mon, Jan 29, 2024 at 11:57:45AM +0800, Ming Lei wrote:

quoted

On Mon, Jan 29, 2024 at 12:47:41PM +1100, Dave Chinner wrote:

quoted

On Sun, Jan 28, 2024 at 07:39:49PM -0500, Mike Snitzer wrote:

quoted

On Sun, Jan 28, 2024 at 7:22 PM Matthew Wilcox [off-list ref] wrote:

quoted

On Sun, Jan 28, 2024 at 06:12:29PM -0500, Mike Snitzer wrote:

quoted

On Sun, Jan 28 2024 at  5:02P -0500,
Matthew Wilcox [off-list ref] wrote:

Understood.  But ... the application is asking for as much readahead as
possible, and the sysadmin has said "Don't readahead more than 64kB at
a time".  So why will we not get a bug report in 1-15 years time saying
"I put a limit on readahead and the kernel is ignoring it"?  I think
typically we allow the sysadmin to override application requests,
don't we?

The application isn't knowingly asking for readahead.  It is asking to
mmap the file (and reporter wants it done as quickly as possible..
like occurred before).

... which we do within the constraints of the given configuration.

quoted

This fix is comparable to Jens' commit 9491ae4aade6 ("mm: don't cap
request size based on read-ahead setting") -- same logic, just applied
to callchain that ends up using madvise(MADV_WILLNEED).

Not really. There is a difference between performing a synchronous
read IO here that we must complete, compared to optimistic
asynchronous read-ahead which we can fail or toss away without the
user ever seeing the data the IO returned.

Yeah, the big readahead in this patch happens when user starts to read
over mmaped buffer instead of madvise().

Yes, that's how it is intended to work :/

quoted

quoted

We want required IO to be done in as few, larger IOs as possible,
and not be limited by constraints placed on background optimistic
IOs.

madvise(WILLNEED) is optimistic IO - there is no requirement that it
complete the data reads successfully. If the data is actually
required, we'll guarantee completion when the user accesses it, not
when madvise() is called.  IOWs, madvise is async readahead, and so
really should be constrained by readahead bounds and not user IO
bounds.

We could change this behaviour for madvise of large ranges that we
force into the page cache by ignoring device readahead bounds, but
I'm not sure we want to do this in general.

Perhaps fadvise/madvise(willneed) can fiddle the file f_ra.ra_pages
value in this situation to override the device limit for large
ranges (for some definition of large - say 10x bdi->ra_pages) and
restore it once the readahead operation is done. This would make it
behave less like readahead and more like a user read from an IO
perspective...

->ra_pages is just one hint, which is 128KB at default, and either
device or userspace can override it.

fadvise/madvise(willneed) already readahead bytes from bdi->io_pages which
is the max device sector size(often 10X of ->ra_pages), please see
force_page_cache_ra().

Yes, but if we also change vma->file->f_ra->ra_pages during the
WILLNEED operation (as we do for FADV_SEQUENTIAL) then we get a
larger readahead window for the demand-paged access portion of the
WILLNEED access...

quoted

Follows the current report:

1) usersapce call madvise(willneed, 1G)

2) only the 1st part(size is from bdi->io_pages, suppose it is 2MB) is
readahead in madvise(willneed, 1G) since commit 6d2be915e589

3) the other parts(2M ~ 1G) is readahead by unit of bdi->ra_pages which is
set as 64KB by userspace when userspace reads the mmaped buffer, then
the whole application becomes slower.

It gets limited by file->f_ra->ra_pages being initialised to
bdi->ra_pages and then never changed as the advice for access
methods to the file are changed.

But the problem here is *not the readahead code*. The problem is
that the user has configured the device readahead window to be far
smaller than is optimal for the storage. Hence readahead is slow.
The fix for that is to either increase the device readahead windows,
or to change the specific readahead window for the file that has
sequential access patterns.

Indeed, we already have that - FADV_SEQUENTIAL will set
file->f_ra.ra_pages to 2 * bdi->ra_pages so that readahead uses
larger IOs for that access.

That's what should happen here - MADV_WILLNEED does not imply a
specific access pattern so the application should be running
MADV_SEQUENTIAL (triggers aggressive readahead) then MADV_WILLNEED
to start the readahead, and then the rest of the on-demand readahead
will get the higher readahead limits.

quoted

This patch changes 3) to use bdi->io_pages as readahead unit.

I think it really should be changing MADV/FADV_SEQUENTIAL to set
file->f_ra.ra_pages to bdi->io_pages, not bdi->ra_pages * 2, and the
mem.load() implementation in the application converted to use
MADV_SEQUENTIAL to properly indicate it's access pattern to the
readahead algorithm.

Here the single .ra_pages may not work, that is why this patch stores
the willneed range in maple tree, please see the following words from
the original RH report:

"
Increasing read ahead is not an option as it has a mixed I/O workload of
random I/O and sequential I/O, so that a large read ahead is very counterproductive
to the random I/O and is unacceptable.
"

Also almost all these advises(SEQUENTIA, WILLNEED, NORMAL, RANDOM)
ignore the passed range, and the behavior becomes all or nothing,
instead of something only for the specified range, which may not
match with man, please see 'man posix_fadvise':

       POSIX_FADV_NORMAL
              Indicates that the application has no advice to give about its access
			  pattern for the specified data.  If no advice is given for an open file,
			  this is the default assumption.

       POSIX_FADV_SEQUENTIAL
              The application expects to access the specified data sequentially (with
			  lower offsets read before higher ones).

       POSIX_FADV_RANDOM
              The specified data will be accessed in random order.

       POSIX_FADV_NOREUSE
              The specified data will be accessed only once.

              In kernels before 2.6.18, POSIX_FADV_NOREUSE had the same semantics as
			  POSIX_FADV_WILLNEED.  This was probably a bug; since kernel 2.6.18, this
			  flag is a no-op.

       POSIX_FADV_WILLNEED
              The specified data will be accessed in the near future.

It is even worse for readahead() syscall:

	DESCRIPTION
	       readahead()  initiates readahead on a file so that subsequent reads from that
		   file will be satisfied from the cache, and not block on disk I/O (assuming the
		   readahead was initiated early enough and that other activity on the system did
		   not in the meantime flush pages from the cache).

Thanks,
Ming