Re: mm/madvise: set ra_pages as device max request size during ADV_POPULATE_READ

[Ming Lei <ming.lei@redhat.com>]

On Thu, Feb 01, 2024 at 11:43:11PM -0500, Mike Snitzer wrote:
> On Thu, Feb 01 2024 at  9:20P -0500,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > madvise(MADV_POPULATE_READ) tries to populate all page tables in the
> > specific range, so it is usually sequential IO if VMA is backed by
> > file.
> > 
> > Set ra_pages as device max request size for the involved readahead in
> > the ADV_POPULATE_READ, this way reduces latency of madvise(MADV_POPULATE_READ)
> > to 1/10 when running madvise(MADV_POPULATE_READ) over one 1GB file with
> > usual(default) 128KB of read_ahead_kb.
> > 
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Don Dutile <ddutile@redhat.com>
> > Cc: Rafael Aquini <raquini@redhat.com>
> > Cc: Dave Chinner <david@fromorbit.com>
> > Cc: Mike Snitzer <snitzer@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  mm/madvise.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 912155a94ed5..db5452c8abdd 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -900,6 +900,37 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> >  		return -EINVAL;
> >  }
> >  
> > +static void madvise_restore_ra_win(struct file **file, unsigned int ra_pages)
> > +{
> > +	if (*file) {
> > +		struct file *f = *file;
> > +
> > +		f->f_ra.ra_pages = ra_pages;
> > +		fput(f);
> > +		*file = NULL;
> > +	}
> > +}
> > +
> > +static struct file *madvise_override_ra_win(struct file *f,
> > +		unsigned long start, unsigned long end,
> > +		unsigned int *old_ra_pages)
> > +{
> > +	unsigned int io_pages;
> > +
> > +	if (!f || !f->f_mapping || !f->f_mapping->host)
> > +		return NULL;
> > +
> > +	io_pages = inode_to_bdi(f->f_mapping->host)->io_pages;
> > +	if (((end - start) >> PAGE_SHIFT) < io_pages)
> > +		return NULL;
> > +
> > +	f = get_file(f);
> > +	*old_ra_pages = f->f_ra.ra_pages;
> > +	f->f_ra.ra_pages = io_pages;
> > +
> > +	return f;
> > +}
> > +
> 
> Does this override imply that madvise_populate resorts to calling
> filemap_fault() and here you're just arming it to use the larger
> ->io_pages for the duration of all associated faulting?

Yes.

> 
> Wouldn't it be better to avoid faulting and build up larger page

How can we avoid the fault handling? which is needed to build VA->PA mapping.

> vectors that get sent down to the block layer in one go and let the

filemap_fault() already tries to allocate folio in big size(max order
is MAX_PAGECACHE_ORDER), see page_cache_ra_order() and ra_alloc_folio().

> block layer split using the device's limits? (like happens with
> force_page_cache_ra)

Here filemap code won't deal with block directly because there is VFS &
FS and io mapping is required, and it just calls aops->readahead() or
aops->read_folio(), but block plug & readahead_control are applied for
handling everything in batch.

> 
> I'm concerned that madvise_populate isn't so efficient with filemap

That is why this patch increases readahead window, then
madvise_populate() performance can be improved by X10 in big file-backed
popluate read.

> due to excessive faulting (*BUT* I haven't traced to know, I'm just
> inferring that is why twiddling f->f_ra.ra_pages helps improve
> madvise_populate by having it issue larger IO. Apologies if I'm way
> off base)

As mentioned, fault handling can't be avoided, but we can improve
involved readahead IO perf.



Thanks,
Ming