On Mon, Jan 29 2024 at 8:26P -0500, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> On Mon, Jan 29, 2024 at 04:25:41PM +0800, Ming Lei wrote:
> > 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.
> > "
>
> It is really frustrating having to drag this important information out of
> you. Please send the full bug report (stripping identifying information
> if that's what the customer needs). We seem to be covering the same
> ground again in public that apparently you've already done in private.
> This is no way to work with upstream.
We are all upstream developers here. And Ming is among the best of us,
so please try to stay constructive.
You now have the full bug report (I provided the reproducer and
additonal context, and now Ming shared the "Increasing read ahead is
not an option..." detail from the original report).
BUT Ming did mention the potential for mixed workload in his original
RFC patch header:
On Sun, Jan 28 2024 at 9:25P -0500, Ming Lei <ming.lei@xxxxxxxxxx> wrote:
> Since commit 6d2be915e589 ("mm/readahead.c: fix readahead failure for
> memoryless NUMA nodes and limit readahead max_pages"), ADV_WILLNEED
> only tries to readahead 512 pages, and the remained part in the advised
> range fallback on normal readahead.
>
> If bdi->ra_pages is set as small, readahead will perform not efficient
> enough. Increasing read ahead may not be an option since workload may
> have mixed random and sequential I/O.
And while both you and Dave rightly seized on the seemingly "Doctor it
hurts when I clamp readahead to be small but then issue larger
sequential reads and want it to use larger readahead" aspect of this
report...
Dave was helpful with his reasoned follow-up responses, culminating
with this one (where the discussion evolved to clearly consider the
fact that an integral part of addressing the reported issue is the
need to allow for mixed workloads not stomping on each other when
issuing IO to the same backing block device):
On Mon, Jan 29 2024 at 12:15P -0500,
Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> On Mon, Jan 29, 2024 at 11:57:45AM +0800, Ming Lei wrote:
> > On Mon, Jan 29, 2024 at 12:47:41PM +1100, Dave Chinner wrote:
> > > On Sun, Jan 28, 2024 at 07:39:49PM -0500, Mike Snitzer wrote:
> > > > On Sun, Jan 28, 2024 at 7:22 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Sun, Jan 28, 2024 at 06:12:29PM -0500, Mike Snitzer wrote:
> > > > > > On Sun, Jan 28 2024 at 5:02P -0500,
> > > > > > Matthew Wilcox <willy@xxxxxxxxxxxxx> 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.
> > >
> > > > 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 :/
>
> > > 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...
>
> >
> > 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.
>
> > 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.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
Anyway, if we could all keep cool and just finish the discussion about
how to address the reported issue in a long-term supported way that'd
be _awesome_.
While I'm sure this legacy application would love to not have to
change its code at all, I think we can all agree that we need to just
focus on how best to advise applications that have mixed workloads
accomplish efficient mmap+read of both sequential and random.
To that end, I heard Dave clearly suggest 2 things:
1) update MADV/FADV_SEQUENTIAL to set file->f_ra.ra_pages to
bdi->io_pages, not bdi->ra_pages * 2
2) Have the application first issue MADV_SEQUENTIAL to convey that for
the following MADV_WILLNEED is for sequential file load (so it is
desirable to use larger ra_pages)
This overrides the default of bdi->io_pages and _should_ provide the
required per-file duality of control for readahead, correct?
Thanks,
Mike
