Re: [PATCH 07/17] mm/filemap: Change filemap_read_page calling conventions

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

On Tue, Nov 03, 2020 at 08:34:27AM +0100, Christoph Hellwig wrote:
> > +static int filemap_read_page(struct file *file, struct address_space *mapping,
> > +		struct page *page)
> 
> I still think we should drop the mapping argument as well.

Feels a little weird to have it in the caller and not pass it in.

> > +	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
> > +		unlock_page(page);
> > +		put_page(page);
> > +		return ERR_PTR(-EAGAIN);
> > +	}
> > +	error = filemap_read_page(iocb->ki_filp, mapping, page);
> > +	if (!error)
> > +		return page;
> 
> I think a goto error for the error cases would be much more useful.
> That would allow to also share the error put_page for the flag check
> above and the truncated case below, but most importantly make the code
> flow obvious vs the early return for the success case.

In this patch, I'm trying to be obvious about the code motion between
the two functions.  This gets straightened out eventually:

        if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
                unlock_page(page);
                error = -EAGAIN;
        } else {
                error = filemap_read_page(iocb->ki_filp, mapping, page);
                if (!error)
                        return 0;
        }
error:
        put_page(page);
        return error;