Re: [PATCH 15/17] mm/filemap: Don't relock the page after calling readpage

[Matthew Wilcox <willy@infradead.org>]

On Tue, Nov 03, 2020 at 06:13:56PM +0100, Christoph Hellwig wrote:
> On Tue, Nov 03, 2020 at 03:24:36PM +0000, Matthew Wilcox wrote:
> > On Tue, Nov 03, 2020 at 09:00:45AM +0100, Christoph Hellwig wrote:
> > > On Mon, Nov 02, 2020 at 06:43:10PM +0000, Matthew Wilcox (Oracle) wrote:
> > > > We don't need to get the page lock again; we just need to wait for
> > > > the I/O to finish, so use wait_on_page_locked_killable() like the
> > > > other callers of ->readpage.
> > > 
> > > As that isn't entirely obvious, what about adding a comment next to
> > > the wait_on_page_locked_killable call to document it?
> > 
> > The other callers of ->readpage don't document that, so not sure why
> > we should here?
> 
> The callers of ->readpage are a mess :(
> 
> Many use lock_page or trylock_page, one uses wait_on_page_locked directly,
> another one uses the wait_on_page_read helper.  I think we need a good
> helper here, and I think it looks a lot like filemap_read_page in your
> tree..

Oh, heh.  Turns out I wrote this in a patch series the other day ...

static int mapping_readpage(struct file *file, struct address_space *mapping,
                struct page *page, bool synchronous)
{
        struct readahead_control ractl = {
                .file = file,
                .mapping = mapping,
                ._index = page->index,
                ._nr_pages = 1,
        };
        int ret;

        if (!synchronous && mapping->a_ops->readahead) {
                mapping->a_ops->readahead(&ractl);
                return 0;
        }

        ret = mapping->a_ops->readpage(file, page);
        if (ret != AOP_UPDATED_PAGE)
                return ret;
        unlock_page(page);
        return 0;
}

(it's for swap_readpage, which is generally called in an async manner ...
and really doesn't handle errors at all well)

This does illustrate that we need the mapping argument.  Some of the
callers of ->readpage need to pass a NULL file pointer, so we can't
get it from file->f_mapping.