Re: [PATCH v3 6/8] filemap: Allow __filemap_get_folio to allocate large folios

[Matthew Wilcox <willy@infradead.org>]

On Tue, Jun 13, 2023 at 05:54:35PM +1000, Dave Chinner wrote:
> On Tue, Jun 13, 2023 at 03:00:14AM +0100, Matthew Wilcox wrote:
> > On Tue, Jun 13, 2023 at 11:30:13AM +1000, Dave Chinner wrote:
> > > I think this ends up being sub-optimal and fairly non-obvious
> > > non-obvious behaviour from the iomap side of the fence which is
> > > clearly asking for high-order folios to be allocated. i.e. a small
> > > amount of allocate-around to naturally align large folios when the
> > > page cache is otherwise empty would make a big difference to the
> > > efficiency of non-large-folio-aligned sequential writes...
> > 
> > At this point we're arguing about what I/O pattern to optimise for.
> > I'm going for a "do no harm" approach where we only allocate exactly as
> > much memory as we did before.  You're advocating for a
> > higher-risk/higher-reward approach.
> 
> Not really - I'm just trying to understand the behaviour the change
> will result in, compared to what would be considered optimal as it's
> not clearly spelled out in either the code or the commit messages.

I suppose it depends what you think we're optimising for.  If it's
minimum memory consumption, then the current patchset is optimal ;-) If
it's minimum number of folios allocated for a particular set of writes,
then your proposal makes sense.  I do think we should end up doing
something along the lines of your sketch; it just doesn't need to be now.

> > I'd like to see some amount of per-fd write history (as we have per-fd
> > readahead history) to decide whether to allocate large folios ahead of
> > the current write position.  As with readahead, I'd like to see that even
> > doing single-byte writes can result in the allocation of large folios,
> > as long as the app has done enough of them.
> 
> *nod*
> 
> We already have some hints in the iomaps that can tell you this sort
> of thing. e.g. if ->iomap_begin() returns a delalloc iomap that
> extends beyond the current write, we're performing a sequence of
> multiple sequential writes.....

Well, if this is something the FS is already tracking, then we can
either try to lift that functionality into the page cache, or just take
advantage of the FS knowledge.  In iomap_write_begin(), we have access
to the srcmap and the iomap, and we can pass in something other than
the length of the write as the hint to __iomap_get_folio() for the
size of the folio we would like.

I should probably clean up the kernel-doc for iomap_get_folio() ...

- * @len: length of write
+ * @len: Suggested size of folio to create.