clean up the generic pagecache read helpers

clean up the generic pagecache read helpers

[Christoph Hellwig]

Hi Andrew,

this series cleans up refactors our generic read from page cache helpers.
The recent usage of the gang lookups helped a fair amount compared to
the previous state, but left some of the old mess and actually introduced
some new rather odd calling conventions as well.

Matthew: I think this should actually help with your THP work even if it
means another rebase.  I was a bit surprised how quickly the gang lookups
went in as well.

Diffstat:
 fs/btrfs/file.c    |    2 
 include/linux/fs.h |    4 
 mm/filemap.c       |  337 ++++++++++++++++++++++-------------------------------
 3 files changed, 149 insertions(+), 194 deletions(-)

[PATCH 01/13] mm: simplify generic_file_buffered_read_readpage

[Christoph Hellwig]

Stop passing pointless arguments, and return an int instead of a page
struct that contains either the passed in page, an ERR_PTR or NULL and
use goto labels to share common code.  Also rename the function to
filemap_readpage as it is a fairly generic wrapper around ->readpage
that isn't really specific to generic_file_buffered_read.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 66 ++++++++++++++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 30 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index d90614f501daa5..2e997890cc81c2 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2168,59 +2168,53 @@ static int lock_page_for_iocb(struct kiocb *iocb, struct page *page)
 		return lock_page_killable(page);
 }
 
-static struct page *
-generic_file_buffered_read_readpage(struct kiocb *iocb,
-				    struct file *filp,
-				    struct address_space *mapping,
-				    struct page *page)
+static int filemap_readpage(struct kiocb *iocb, struct page *page)
 {
-	struct file_ra_state *ra = &filp->f_ra;
-	int error;
+	struct file *file = iocb->ki_filp;
+	int error = -EAGAIN;
 
 	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT)) {
 		unlock_page(page);
-		put_page(page);
-		return ERR_PTR(-EAGAIN);
+		goto out_put_page;
 	}
 
 	/*
-	 * A previous I/O error may have been due to temporary
-	 * failures, eg. multipath errors.
-	 * PG_error will be set again if readpage fails.
+	 * A previous I/O error may have been due to temporary failures, e.g.
+	 * multipath errors.  PG_error will be set again if readpage fails.
 	 */
 	ClearPageError(page);
 	/* Start the actual read. The read will unlock the page. */
-	error = mapping->a_ops->readpage(filp, page);
-
-	if (unlikely(error)) {
-		put_page(page);
-		return error != AOP_TRUNCATED_PAGE ? ERR_PTR(error) : NULL;
-	}
+	error = file->f_mapping->a_ops->readpage(file, page);
+	if (unlikely(error))
+		goto out_put_page;
 
 	if (!PageUptodate(page)) {
 		error = lock_page_for_iocb(iocb, page);
-		if (unlikely(error)) {
-			put_page(page);
-			return ERR_PTR(error);
-		}
+		if (unlikely(error))
+			goto out_put_page;
+
 		if (!PageUptodate(page)) {
 			if (page->mapping == NULL) {
 				/*
 				 * invalidate_mapping_pages got it
 				 */
 				unlock_page(page);
-				put_page(page);
-				return NULL;
+				error = AOP_TRUNCATED_PAGE;
+				goto out_put_page;
 			}
 			unlock_page(page);
-			shrink_readahead_size_eio(ra);
-			put_page(page);
-			return ERR_PTR(-EIO);
+			shrink_readahead_size_eio(&file->f_ra);
+			error = -EIO;
+			goto out_put_page;
 		}
+
 		unlock_page(page);
 	}
+	return 0;
 
-	return page;
+out_put_page:
+	put_page(page);
+	return error;
 }
 
 static struct page *
@@ -2291,7 +2285,13 @@ generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
 		return page;
 	}
 
-	return generic_file_buffered_read_readpage(iocb, filp, mapping, page);
+	error = filemap_readpage(iocb, page);
+	if (error) {
+		if (error == AOP_TRUNCATED_PAGE)
+			return NULL;
+		return ERR_PTR(error);
+	}
+	return page;
 }
 
 static struct page *
@@ -2322,7 +2322,13 @@ generic_file_buffered_read_no_cached_page(struct kiocb *iocb,
 		return error != -EEXIST ? ERR_PTR(error) : NULL;
 	}
 
-	return generic_file_buffered_read_readpage(iocb, filp, mapping, page);
+	error = filemap_readpage(iocb, page);
+	if (error) {
+		if (error == AOP_TRUNCATED_PAGE)
+			return NULL;
+		return ERR_PTR(error);
+	}
+	return page;
 }
 
 static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
-- 
2.28.0

[PATCH 02/13] mm: simplify generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

Stop passing pointless arguments, and return an int instead of a page
struct that contains either the passed in page, an ERR_PTR or NULL and
use goto labels to share common code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 65 ++++++++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 37 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 2e997890cc81c2..c717cfe35cc72a 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2217,15 +2217,11 @@ static int filemap_readpage(struct kiocb *iocb, struct page *page)
 	return error;
 }
 
-static struct page *
-generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
-					   struct file *filp,
-					   struct iov_iter *iter,
-					   struct page *page,
-					   loff_t pos, loff_t count)
+static int generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
+		struct iov_iter *iter, struct page *page, loff_t pos,
+		loff_t count)
 {
-	struct address_space *mapping = filp->f_mapping;
-	struct inode *inode = mapping->host;
+	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	int error;
 
 	/*
@@ -2239,15 +2235,14 @@ generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
 	} else {
 		error = wait_on_page_locked_killable(page);
 	}
-	if (unlikely(error)) {
-		put_page(page);
-		return ERR_PTR(error);
-	}
+	if (unlikely(error))
+		goto put_page;
+
 	if (PageUptodate(page))
-		return page;
+		return 0;
 
-	if (inode->i_blkbits == PAGE_SHIFT ||
-			!mapping->a_ops->is_partially_uptodate)
+	if (mapping->host->i_blkbits == PAGE_SHIFT ||
+	    !mapping->a_ops->is_partially_uptodate)
 		goto page_not_up_to_date;
 	/* pipes can't handle partially uptodate pages */
 	if (unlikely(iov_iter_is_pipe(iter)))
@@ -2260,38 +2255,33 @@ generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
 	if (!mapping->a_ops->is_partially_uptodate(page,
 				pos & ~PAGE_MASK, count))
 		goto page_not_up_to_date_locked;
+
+unlock_page:
 	unlock_page(page);
-	return page;
+	return 0;
 
 page_not_up_to_date:
 	/* Get exclusive access to the page ... */
 	error = lock_page_for_iocb(iocb, page);
-	if (unlikely(error)) {
-		put_page(page);
-		return ERR_PTR(error);
-	}
+	if (unlikely(error))
+		goto put_page;
 
 page_not_up_to_date_locked:
 	/* Did it get truncated before we got the lock? */
 	if (!page->mapping) {
 		unlock_page(page);
-		put_page(page);
-		return NULL;
+		error = AOP_TRUNCATED_PAGE;
+		goto put_page;
 	}
 
 	/* Did somebody else fill it already? */
-	if (PageUptodate(page)) {
-		unlock_page(page);
-		return page;
-	}
+	if (PageUptodate(page))
+		goto unlock_page;
+	return filemap_readpage(iocb, page);
 
-	error = filemap_readpage(iocb, page);
-	if (error) {
-		if (error == AOP_TRUNCATED_PAGE)
-			return NULL;
-		return ERR_PTR(error);
-	}
-	return page;
+put_page:
+	put_page(page);
+	return error;
 }
 
 static struct page *
@@ -2395,13 +2385,14 @@ static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 				break;
 			}
 
-			page = generic_file_buffered_read_pagenotuptodate(iocb,
-					filp, iter, page, pg_pos, pg_count);
-			if (IS_ERR_OR_NULL(page)) {
+			err = generic_file_buffered_read_pagenotuptodate(iocb,
+					iter, page, pg_pos, pg_count);
+			if (err) {
+				if (err == AOP_TRUNCATED_PAGE)
+					err = 0;
 				for (j = i + 1; j < nr_got; j++)
 					put_page(pages[j]);
 				nr_got = i;
-				err = PTR_ERR_OR_ZERO(page);
 				break;
 			}
 		}
-- 
2.28.0

[PATCH 03/13] mm: lift the nowait checks into generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

Move the checks for IOCB_NOWAIT and IOCB_WAITQ from the only caller into
generic_file_buffered_read_pagenotuptodate, which simplifies the error
unwinding.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index c717cfe35cc72a..bae5b905aa7bdc 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2219,19 +2219,22 @@ static int filemap_readpage(struct kiocb *iocb, struct page *page)
 
 static int generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
 		struct iov_iter *iter, struct page *page, loff_t pos,
-		loff_t count)
+		loff_t count, bool first)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
-	int error;
+	int error = -EAGAIN;
+
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		goto put_page;
 
 	/*
-	 * See comment in do_read_cache_page on why
-	 * wait_on_page_locked is used to avoid unnecessarily
-	 * serialisations and why it's safe.
+	 * See comment in do_read_cache_page on why wait_on_page_locked is used
+	 * to avoid unnecessarily serialisations and why it's safe.
 	 */
 	if (iocb->ki_flags & IOCB_WAITQ) {
-		error = wait_on_page_locked_async(page,
-						iocb->ki_waitq);
+		if (!first)
+			goto put_page;
+		error = wait_on_page_locked_async(page, iocb->ki_waitq);
 	} else {
 		error = wait_on_page_locked_killable(page);
 	}
@@ -2376,17 +2379,8 @@ static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 		}
 
 		if (!PageUptodate(page)) {
-			if ((iocb->ki_flags & IOCB_NOWAIT) ||
-			    ((iocb->ki_flags & IOCB_WAITQ) && i)) {
-				for (j = i; j < nr_got; j++)
-					put_page(pages[j]);
-				nr_got = i;
-				err = -EAGAIN;
-				break;
-			}
-
 			err = generic_file_buffered_read_pagenotuptodate(iocb,
-					iter, page, pg_pos, pg_count);
+					iter, page, pg_pos, pg_count, i == 0);
 			if (err) {
 				if (err == AOP_TRUNCATED_PAGE)
 					err = 0;
-- 
2.28.0

[PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

Move the calculation of the per-page variables and the readahead handling
from the only caller into generic_file_buffered_read_pagenotuptodate,
which now becomes a routine to handle everything related to bringing
one page uptodate and thus is renamed to filemap_read_one_page.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 63 +++++++++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index bae5b905aa7bdc..5cdf8090d4e12c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2217,13 +2217,26 @@ static int filemap_readpage(struct kiocb *iocb, struct page *page)
 	return error;
 }
 
-static int generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
-		struct iov_iter *iter, struct page *page, loff_t pos,
-		loff_t count, bool first)
+static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
+		struct page *page, pgoff_t pg_index, bool first)
 {
-	struct address_space *mapping = iocb->ki_filp->f_mapping;
+	struct file *file = iocb->ki_filp;
+	struct address_space *mapping = file->f_mapping;
+	loff_t last = iocb->ki_pos + iter->count;
+	pgoff_t last_index = (last + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);
 	int error = -EAGAIN;
 
+	if (PageReadahead(page)) {
+		if (iocb->ki_flags & IOCB_NOIO)
+			goto put_page;
+		page_cache_async_readahead(mapping, &file->f_ra, file, page,
+					   pg_index, last_index - pg_index);
+	}
+
+	if (PageUptodate(page))
+		return 0;
+
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		goto put_page;
 
@@ -2255,8 +2268,8 @@ static int generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
 	/* Did it get truncated before we got the lock? */
 	if (!page->mapping)
 		goto page_not_up_to_date_locked;
-	if (!mapping->a_ops->is_partially_uptodate(page,
-				pos & ~PAGE_MASK, count))
+	if (!mapping->a_ops->is_partially_uptodate(page, pos & ~PAGE_MASK,
+			last - pos))
 		goto page_not_up_to_date_locked;
 
 unlock_page:
@@ -2360,35 +2373,15 @@ static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 		nr_got = 1;
 got_pages:
 	for (i = 0; i < nr_got; i++) {
-		struct page *page = pages[i];
-		pgoff_t pg_index = index + i;
-		loff_t pg_pos = max(iocb->ki_pos,
-				    (loff_t) pg_index << PAGE_SHIFT);
-		loff_t pg_count = iocb->ki_pos + iter->count - pg_pos;
-
-		if (PageReadahead(page)) {
-			if (iocb->ki_flags & IOCB_NOIO) {
-				for (j = i; j < nr_got; j++)
-					put_page(pages[j]);
-				nr_got = i;
-				err = -EAGAIN;
-				break;
-			}
-			page_cache_async_readahead(mapping, ra, filp, page,
-					pg_index, last_index - pg_index);
-		}
-
-		if (!PageUptodate(page)) {
-			err = generic_file_buffered_read_pagenotuptodate(iocb,
-					iter, page, pg_pos, pg_count, i == 0);
-			if (err) {
-				if (err == AOP_TRUNCATED_PAGE)
-					err = 0;
-				for (j = i + 1; j < nr_got; j++)
-					put_page(pages[j]);
-				nr_got = i;
-				break;
-			}
+		err = filemap_make_page_uptodate(iocb, iter, pages[i],
+				index + i, i == 0);
+		if (err) {
+			if (err == AOP_TRUNCATED_PAGE)
+				err = 0;
+			for (j = i + 1; j < nr_got; j++)
+				put_page(pages[j]);
+			nr_got = i;
+			break;
 		}
 	}
 
-- 
2.28.0

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Matthew Wilcox]

On Sat, Oct 31, 2020 at 09:59:55AM +0100, Christoph Hellwig wrote:
> Move the calculation of the per-page variables and the readahead handling
> from the only caller into generic_file_buffered_read_pagenotuptodate,
> which now becomes a routine to handle everything related to bringing
> one page uptodate and thus is renamed to filemap_read_one_page.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  mm/filemap.c | 63 +++++++++++++++++++++++-----------------------------
>  1 file changed, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index bae5b905aa7bdc..5cdf8090d4e12c 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2217,13 +2217,26 @@ static int filemap_readpage(struct kiocb *iocb, struct page *page)
>  	return error;
>  }
>  
> -static int generic_file_buffered_read_pagenotuptodate(struct kiocb *iocb,
> -		struct iov_iter *iter, struct page *page, loff_t pos,
> -		loff_t count, bool first)
> +static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
> +		struct page *page, pgoff_t pg_index, bool first)

I prefer "filemap_update_page".

I don't understand why you pass in pg_index instead of using page->index.
We dereferenced the page pointer already to check PageReadahead(), so
there's no performance issue here.

Also, if filemap_find_get_pages() stops on the first !Uptodate or
Readahead page, as I had it in my patchset, then we don't need the loop
at all -- filemap_read_pages() looks like:

        nr_pages = filemap_find_get_pages(iocb, iter, pages, nr);
	if (!nr_pages) {
		pages[0] = filemap_create_page(iocb, iter);
		if (!IS_ERR_OR_NULL(pages[0]))
			return 1;
		if (!pages[0])
			goto retry;
		return PTR_ERR(pages[0]);
	}

	page = pages[nr_pages - 1];
	if (PageUptodate(page) && !PageReadahead(page))
		return nr_pages;
	err = filemap_update_page(iocb, iter, page);
	if (!err)
		return nr_pages;
	nr_pages -= 1;
	if (nr_pages)
		return nr_pages;
	return err;

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

On Sat, Oct 31, 2020 at 05:06:46PM +0000, Matthew Wilcox wrote:
> > +static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
> > +		struct page *page, pgoff_t pg_index, bool first)
> 
> I prefer "filemap_update_page".

That has the advantage of being shorter, while I find it less descriptive.
I can updated it and add a comment explaining what it does, as that is
probably warranted anyway.

> I don't understand why you pass in pg_index instead of using page->index.
> We dereferenced the page pointer already to check PageReadahead(), so
> there's no performance issue here.

Yes, we should do that.

> Also, if filemap_find_get_pages() stops on the first !Uptodate or
> Readahead page, as I had it in my patchset, then we don't need the loop
> at all -- filemap_read_pages() looks like:
> 
>         nr_pages = filemap_find_get_pages(iocb, iter, pages, nr);
> 	if (!nr_pages) {
> 		pages[0] = filemap_create_page(iocb, iter);
> 		if (!IS_ERR_OR_NULL(pages[0]))
> 			return 1;
> 		if (!pages[0])
> 			goto retry;
> 		return PTR_ERR(pages[0]);
> 	}
> 
> 	page = pages[nr_pages - 1];
> 	if (PageUptodate(page) && !PageReadahead(page))
> 		return nr_pages;
> 	err = filemap_update_page(iocb, iter, page);
> 	if (!err)
> 		return nr_pages;
> 	nr_pages -= 1;
> 	if (nr_pages)
> 		return nr_pages;
> 	return err;

This looks sensible, but goes beyond the simple refactoring I had
intended.  Let me take a more detailed look at your series (I had just
updated my existing series to to the latest linux-next) and see how
it can nicely fit in.

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Matthew Wilcox]

On Sun, Nov 01, 2020 at 11:31:44AM +0100, Christoph Hellwig wrote:
> This looks sensible, but goes beyond the simple refactoring I had
> intended.  Let me take a more detailed look at your series (I had just
> updated my existing series to to the latest linux-next) and see how
> it can nicely fit in.

I'm also working on merging our two series.  I'm just about there ...

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

On Sun, Nov 01, 2020 at 10:49:58AM +0000, Matthew Wilcox wrote:
> On Sun, Nov 01, 2020 at 11:31:44AM +0100, Christoph Hellwig wrote:
> > This looks sensible, but goes beyond the simple refactoring I had
> > intended.  Let me take a more detailed look at your series (I had just
> > updated my existing series to to the latest linux-next) and see how
> > it can nicely fit in.
> 
> I'm also working on merging our two series.  I'm just about there ...

Heh, same here.

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

On Sun, Nov 01, 2020 at 11:51:12AM +0100, Christoph Hellwig wrote:
> On Sun, Nov 01, 2020 at 10:49:58AM +0000, Matthew Wilcox wrote:
> > On Sun, Nov 01, 2020 at 11:31:44AM +0100, Christoph Hellwig wrote:
> > > This looks sensible, but goes beyond the simple refactoring I had
> > > intended.  Let me take a more detailed look at your series (I had just
> > > updated my existing series to to the latest linux-next) and see how
> > > it can nicely fit in.
> > 
> > I'm also working on merging our two series.  I'm just about there ...
> 
> Heh, same here.

I'll stop for now.

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Matthew Wilcox]

On Sun, Nov 01, 2020 at 11:51:58AM +0100, Christoph Hellwig wrote:
> On Sun, Nov 01, 2020 at 11:51:12AM +0100, Christoph Hellwig wrote:
> > On Sun, Nov 01, 2020 at 10:49:58AM +0000, Matthew Wilcox wrote:
> > > On Sun, Nov 01, 2020 at 11:31:44AM +0100, Christoph Hellwig wrote:
> > > > This looks sensible, but goes beyond the simple refactoring I had
> > > > intended.  Let me take a more detailed look at your series (I had just
> > > > updated my existing series to to the latest linux-next) and see how
> > > > it can nicely fit in.
> > > 
> > > I'm also working on merging our two series.  I'm just about there ...
> > 
> > Heh, same here.
> 
> I'll stop for now.

http://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/next

is what i currently have.  Haven't pulled in everything from you; certainly not renaming generic_file_buffered_read to filemap_read(), which is awesome.
i'm about 500 seconds into an xfstests run.

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

On Sun, Nov 01, 2020 at 11:04:06AM +0000, Matthew Wilcox wrote:
> > I'll stop for now.
> 
> http://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/next
> 
> is what i currently have.  Haven't pulled in everything from you; certainly not renaming generic_file_buffered_read to filemap_read(), which is awesome.
> i'm about 500 seconds into an xfstests run.

A bunch of comments from a very quick look:

mm/filemap: Rename generic_file_buffered_read subfunctions

 - the commit log still mentions the old names

mm/filemap: Change calling convention for buffered read functions

 - please also drop the mapping argument to the various functions while
   you're at it

mm/filemap: Reduce indentation in gfbr_read_page

 - still mentionds the old function name

mm/filemap: Support readpage splitting a page

 - nitpick, I find this a little hard to read:

+	} else if (!trylock_page(page)) {
+		put_and_wait_on_page_locked(page, TASK_KILLABLE);
+		return NULL;
 	}

and would write this a little more coarse as:

	} else {
		if (!trylock_page(page)) {
			put_and_wait_on_page_locked(page, TASK_KILLABLE);
			return NULL;
		}
	}

Also I think the messy list of uptodate checks could now be simplified
down to:

	if (!PageUptodate(page)) {
		if (inode->i_blkbits <= PAGE_SHIFT &&
		    mapping->a_ops->is_partially_uptodate &&
		    !iov_iter_is_pipe(iter)) {
			if (!page->mapping)
				goto truncated;
			if (mapping->a_ops->is_partially_uptodate(page,
					pos & (thp_size(page) - 1), count))
				goto uptodate;
		}

		return filemap_read_page(iocb, mapping, page);
	}

mm/filemap: Convert filemap_read_page to return an errno:

 - using a goto label for the put_page + return error case like in my
   patch would be cleaner I think

mm/filemap: Restructure filemap_get_pages

 - I have to say I still like my little helper here for
   the two mapping_get_read_thps calls plus page_cache_sync_readahead

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Matthew Wilcox]

On Sun, Nov 01, 2020 at 12:52:17PM +0100, Christoph Hellwig wrote:
> On Sun, Nov 01, 2020 at 11:04:06AM +0000, Matthew Wilcox wrote:
> > > I'll stop for now.
> > 
> > http://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/next
> > 
> > is what i currently have.  Haven't pulled in everything from you; certainly not renaming generic_file_buffered_read to filemap_read(), which is awesome.
> > i'm about 500 seconds into an xfstests run.
> 
> A bunch of comments from a very quick look:
> 
> mm/filemap: Rename generic_file_buffered_read subfunctions
> 
>  - the commit log still mentions the old names

Hm?  I have this:

    mm/filemap: Rename generic_file_buffered_read subfunctions
    
    The recent split of generic_file_buffered_read() created some very
    long function names which are hard to distinguish from each other.
    Rename as follows:
    
    generic_file_buffered_read_readpage -> filemap_read_page
    generic_file_buffered_read_pagenotuptodate -> filemap_update_page
    generic_file_buffered_read_no_cached_page -> filemap_create_page
    generic_file_buffered_read_get_pages -> filemap_get_pages

> mm/filemap: Change calling convention for buffered read functions
> 
>  - please also drop the mapping argument to the various functions while
>    you're at it

Not sure I see the point to it.  Sure, they _can_ retrieve it with
iocb->ki_filp->f_mapping, but usually we like to pass the mapping
argument to functions which do something with the mapping.

> mm/filemap: Reduce indentation in gfbr_read_page
> 
>  - still mentionds the old function name

Fixed.

> mm/filemap: Support readpage splitting a page
> 
>  - nitpick, I find this a little hard to read:
> 
> +	} else if (!trylock_page(page)) {
> +		put_and_wait_on_page_locked(page, TASK_KILLABLE);
> +		return NULL;
>  	}
> 
> and would write this a little more coarse as:
> 
> 	} else {
> 		if (!trylock_page(page)) {
> 			put_and_wait_on_page_locked(page, TASK_KILLABLE);
> 			return NULL;
> 		}
> 	}

No strong feeling here.  I'll change it.

> Also I think the messy list of uptodate checks could now be simplified
> down to:
> 
> 	if (!PageUptodate(page)) {
> 		if (inode->i_blkbits <= PAGE_SHIFT &&

I've been wondering about this test's usefulness in the presence
of THP.  Do we want to make it 'if (inode->i_blkbits < (thp_order(page)
+ PAGE_SHIFT)'?  It doesn't make sense to leave it as it is because then
a 1kB and 4kB blocksize filesystem will behave differently.

> 		    mapping->a_ops->is_partially_uptodate &&
> 		    !iov_iter_is_pipe(iter)) {
> 			if (!page->mapping)
> 				goto truncated;
> 			if (mapping->a_ops->is_partially_uptodate(page,
> 					pos & (thp_size(page) - 1), count))
> 				goto uptodate;
> 		}

Now that you've rearranged it like this, it's obvious that the truncated
check is in the wrong place.  We don't want to call filemap_read_page()
if the page has been truncated either.

> 		return filemap_read_page(iocb, mapping, page);
> 	}
> 
> mm/filemap: Convert filemap_read_page to return an errno:
> 
>  - using a goto label for the put_page + return error case like in my
>    patch would be cleaner I think

A later patch hoists the put_page to the caller, so I think you'll like
where it ends up.

> mm/filemap: Restructure filemap_get_pages
> 
>  - I have to say I still like my little helper here for
>    the two mapping_get_read_thps calls plus page_cache_sync_readahead

I'll take a look at that.

Looking at all this again, I think I want to pull the IOCB checks out
of filemap_read_page() and just pass a struct file to it.  It'll make
it more clear that NOIO, NOWAIT and WAITQ can't get to calling ->readpage.

I'll send another rev tomorrow.

Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig]

On Sun, Nov 01, 2020 at 02:55:07PM +0000, Matthew Wilcox wrote:
> Hm?  I have this:

Yes, this looks fine.  Not sure if I saw an earlier version or was
just confused.

> > mm/filemap: Change calling convention for buffered read functions
> > 
> >  - please also drop the mapping argument to the various functions while
> >    you're at it
> 
> Not sure I see the point to it.  Sure, they _can_ retrieve it with
> iocb->ki_filp->f_mapping, but usually we like to pass the mapping
> argument to functions which do something with the mapping.

There really isn't any point in passing an extra argument that can
trivially be derived.

> > Also I think the messy list of uptodate checks could now be simplified
> > down to:
> > 
> > 	if (!PageUptodate(page)) {
> > 		if (inode->i_blkbits <= PAGE_SHIFT &&
> 
> I've been wondering about this test's usefulness in the presence
> of THP.  Do we want to make it 'if (inode->i_blkbits < (thp_order(page)
> + PAGE_SHIFT)'?  It doesn't make sense to leave it as it is because then
> a 1kB and 4kB blocksize filesystem will behave differently.

Yeah, the partially uptodate checks would make sense for huge pages.
Just make sure that the iomap version does the right thing for this
case first.

> 
> > 		    mapping->a_ops->is_partially_uptodate &&
> > 		    !iov_iter_is_pipe(iter)) {
> > 			if (!page->mapping)
> > 				goto truncated;
> > 			if (mapping->a_ops->is_partially_uptodate(page,
> > 					pos & (thp_size(page) - 1), count))
> > 				goto uptodate;
> > 		}
> 
> Now that you've rearranged it like this, it's obvious that the truncated
> check is in the wrong place.  We don't want to call filemap_read_page()
> if the page has been truncated either.

True.

> A later patch hoists the put_page to the caller, so I think you'll like
> where it ends up.

I still find the result in the callers a little weird as it doesn't
follow the normal jump to the end for exceptions flow, but that is
just another tiny nitpick.

> 
> > mm/filemap: Restructure filemap_get_pages
> > 
> >  - I have to say I still like my little helper here for
> >    the two mapping_get_read_thps calls plus page_cache_sync_readahead
> 
> I'll take a look at that.
> 
> Looking at all this again, I think I want to pull the IOCB checks out
> of filemap_read_page() and just pass a struct file to it.  It'll make
> it more clear that NOIO, NOWAIT and WAITQ can't get to calling ->readpage.

filemap_update_page alread exits early for NOWAIT, so it would just
need the NOIO check.  filemap_create_page checks NOIO early, but
allocating the page for NOWAIT also seems rather pointless.  So yes,
I think this would be an improvement.

[PATCH 05/13] mm: simplify generic_file_buffered_read_no_cached_page

[Christoph Hellwig]

Return an errno and add a new by reference argument for the allocated
page, which allows to cleanup the error unwindining in the function
and the caller.  Also rename the function to filemap_new_page which is
both shorter and more descriptive.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 53 ++++++++++++++++------------------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 5cdf8090d4e12c..9e1cc18afe1134 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2300,41 +2300,27 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	return error;
 }
 
-static struct page *
-generic_file_buffered_read_no_cached_page(struct kiocb *iocb,
-					  struct iov_iter *iter)
+static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
+		struct page **page)
 {
-	struct file *filp = iocb->ki_filp;
-	struct address_space *mapping = filp->f_mapping;
+	struct address_space *mapping = iocb->ki_filp->f_mapping;
+	gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
-	struct page *page;
 	int error;
 
 	if (iocb->ki_flags & IOCB_NOIO)
-		return ERR_PTR(-EAGAIN);
+		return -EAGAIN;
 
-	/*
-	 * Ok, it wasn't cached, so we need to create a new
-	 * page..
-	 */
-	page = page_cache_alloc(mapping);
+	*page = page_cache_alloc(mapping);
 	if (!page)
-		return ERR_PTR(-ENOMEM);
-
-	error = add_to_page_cache_lru(page, mapping, index,
-				      mapping_gfp_constraint(mapping, GFP_KERNEL));
+		return -ENOMEM;
+	error = add_to_page_cache_lru(*page, mapping, index, gfp);
 	if (error) {
-		put_page(page);
-		return error != -EEXIST ? ERR_PTR(error) : NULL;
+		put_page(*page);
+		return error;
 	}
 
-	error = filemap_readpage(iocb, page);
-	if (error) {
-		if (error == AOP_TRUNCATED_PAGE)
-			return NULL;
-		return ERR_PTR(error);
-	}
-	return page;
+	return filemap_readpage(iocb, *page);
 }
 
 static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
@@ -2366,18 +2352,14 @@ static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 	nr_got = find_get_pages_contig(mapping, index, nr, pages);
 	if (nr_got)
 		goto got_pages;
-
-	pages[0] = generic_file_buffered_read_no_cached_page(iocb, iter);
-	err = PTR_ERR_OR_ZERO(pages[0]);
-	if (!IS_ERR_OR_NULL(pages[0]))
+	err = filemap_new_page(iocb, iter, &pages[0]);
+	if (!err)
 		nr_got = 1;
 got_pages:
 	for (i = 0; i < nr_got; i++) {
 		err = filemap_make_page_uptodate(iocb, iter, pages[i],
 				index + i, i == 0);
 		if (err) {
-			if (err == AOP_TRUNCATED_PAGE)
-				err = 0;
 			for (j = i + 1; j < nr_got; j++)
 				put_page(pages[j]);
 			nr_got = i;
@@ -2387,12 +2369,9 @@ static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 
 	if (likely(nr_got))
 		return nr_got;
-	if (err)
-		return err;
-	/*
-	 * No pages and no error means we raced and should retry:
-	 */
-	goto find_page;
+	if (err == -EEXIST || err == AOP_TRUNCATED_PAGE)
+		goto find_page;
+	return err;
 }
 
 /**
-- 
2.28.0

Re: [PATCH 05/13] mm: simplify generic_file_buffered_read_no_cached_page

[Matthew Wilcox]

On Sat, Oct 31, 2020 at 09:59:56AM +0100, Christoph Hellwig wrote:
> -	page = page_cache_alloc(mapping);
> +	*page = page_cache_alloc(mapping);
>  	if (!page)

	if (!*page)

Re: [PATCH 05/13] mm: simplify generic_file_buffered_read_no_cached_page

[Matthew Wilcox]

On Sat, Oct 31, 2020 at 09:59:56AM +0100, Christoph Hellwig wrote:
> +static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
> +		struct page **page)

I don't like this calling convention.  It's too easy to get it wrong,
as you demonstrated.  I preferred the way Kent had it with returning
an ERR_PTR.

Re: [PATCH 05/13] mm: simplify generic_file_buffered_read_no_cached_page

[Christoph Hellwig]

On Sat, Oct 31, 2020 at 04:28:13PM +0000, Matthew Wilcox wrote:
> On Sat, Oct 31, 2020 at 09:59:56AM +0100, Christoph Hellwig wrote:
> > +static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
> > +		struct page **page)
> 
> I don't like this calling convention.  It's too easy to get it wrong,
> as you demonstrated.  I preferred the way Kent had it with returning
> an ERR_PTR.

I guess for this function we can do that, here is the untested patch
on top of my series to show what we'd end up with:

diff --git a/mm/filemap.c b/mm/filemap.c
index b45f0bafdbaebf..1ac1fcd0067bf1 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2281,38 +2281,40 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	return 0;
 }
 
-static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
-		struct page **page)
+static struct page *filemap_new_page(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	gfp_t gfp = mapping_gfp_constraint(mapping, GFP_KERNEL);
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
+	struct page *page;
 	int error;
 
 	if (iocb->ki_flags & IOCB_NOIO)
-		return -EAGAIN;
+		return ERR_PTR(-EAGAIN);
 
-	*page = page_cache_alloc(mapping);
+	page = page_cache_alloc(mapping);
 	if (!page)
-		return -ENOMEM;
-	error = add_to_page_cache_lru(*page, mapping, index, gfp);
+		return ERR_PTR(-ENOMEM);
+	error = add_to_page_cache_lru(page, mapping, index, gfp);
 	if (error)
 		goto put_page;
-	error = filemap_readpage(iocb, *page);
+	error = filemap_readpage(iocb, page);
 	if (error)
 		goto put_page;
-	if (PageReadahead(*page)) {
+	if (PageReadahead(page)) {
 		error = -EAGAIN;
 		if (iocb->ki_flags & IOCB_NOIO)
 			goto put_page;
 		page_cache_async_readahead(mapping, &iocb->ki_filp->f_ra,
-				iocb->ki_filp, *page, index,
+				iocb->ki_filp, page, index,
 				(iter->count + PAGE_SIZE - 1) >> PAGE_SHIFT);
 	}
 	return 0;
 put_page:
-	put_page(*page);
-	return error;
+	put_page(page);
+	if (error == -EEXIST || error == AOP_TRUNCATED_PAGE)
+		return NULL;
+	return ERR_PTR(error);
 }
 
 static int filemap_find_get_pages(struct kiocb *iocb, struct iov_iter *iter,
@@ -2347,28 +2349,30 @@ static int filemap_read_pages(struct kiocb *iocb, struct iov_iter *iter,
 		return -EINTR;
 
 	nr_pages = filemap_find_get_pages(iocb, iter, pages, nr);
-	if (nr_pages) {
-		for (i = 0; i < nr_pages; i++) {
-			err = filemap_make_page_uptodate(iocb, iter, pages[i],
-					index + i, i == 0);
-			if (err) {
-				for (j = i; j < nr_pages; j++)
-					put_page(pages[j]);
-				nr_pages = i;
-				break;
-			}
+	if (!nr_pages) {
+		pages[0] = filemap_new_page(iocb, iter);
+		if (pages[0] == NULL)
+			goto retry;
+		if (IS_ERR(pages[0]))
+			return PTR_ERR(pages[0]);
+		return 1;
+	}
+
+	for (i = 0; i < nr_pages; i++) {
+		err = filemap_make_page_uptodate(iocb, iter, pages[i],
+				index + i, i == 0);
+		if (err) {
+			for (j = i; j < nr_pages; j++)
+				put_page(pages[j]);
+			if (likely(i > 0))
+				return i;
+			if (err == AOP_TRUNCATED_PAGE)
+				goto retry;
+			return err;
 		}
-	} else {
-		err = filemap_new_page(iocb, iter, &pages[0]);
-		if (!err)
-			nr_pages = 1;
 	}
 
-	if (likely(nr_pages))
-		return nr_pages;
-	if (err == -EEXIST || err == AOP_TRUNCATED_PAGE)
-		goto retry;
-	return err;
+	return nr_pages;
 }
 
 /**

[PATCH 06/13] mm: factor out a filemap_find_get_pages helper

[Christoph Hellwig]

Factor out a helper to lookup a range of contiguous pages from
generic_file_buffered_read_get_pages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 9e1cc18afe1134..0af7ddaa0fe7ba 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2323,39 +2323,46 @@ static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
 	return filemap_readpage(iocb, *page);
 }
 
+static int filemap_find_get_pages(struct kiocb *iocb, struct iov_iter *iter,
+		struct page **pages, unsigned int nr)
+{
+	struct address_space *mapping = iocb->ki_filp->f_mapping;
+	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
+	pgoff_t last_index = (iocb->ki_pos + iter->count + PAGE_SIZE - 1) >>
+			PAGE_SHIFT;
+	int nr_pages;
+
+	nr = min_t(unsigned long, last_index - index, nr);
+	nr_pages = find_get_pages_contig(mapping, index, nr, pages);
+	if (nr_pages)
+		return nr_pages;
+
+	if (iocb->ki_flags & IOCB_NOIO)
+		return -EAGAIN;
+	page_cache_sync_readahead(mapping, &iocb->ki_filp->f_ra, iocb->ki_filp,
+			index, last_index - index);
+	return find_get_pages_contig(mapping, index, nr, pages);
+}
+
 static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
 						struct iov_iter *iter,
 						struct page **pages,
 						unsigned int nr)
 {
-	struct file *filp = iocb->ki_filp;
-	struct address_space *mapping = filp->f_mapping;
-	struct file_ra_state *ra = &filp->f_ra;
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
-	pgoff_t last_index = (iocb->ki_pos + iter->count + PAGE_SIZE-1) >> PAGE_SHIFT;
 	int i, j, nr_got, err = 0;
 
-	nr = min_t(unsigned long, last_index - index, nr);
 find_page:
 	if (fatal_signal_pending(current))
 		return -EINTR;
 
-	nr_got = find_get_pages_contig(mapping, index, nr, pages);
-	if (nr_got)
-		goto got_pages;
-
-	if (iocb->ki_flags & IOCB_NOIO)
-		return -EAGAIN;
-
-	page_cache_sync_readahead(mapping, ra, filp, index, last_index - index);
+	nr_got = filemap_find_get_pages(iocb, iter, pages, nr);
+	if (!nr_got) {
+		err = filemap_new_page(iocb, iter, &pages[0]);
+		if (!err)
+			nr_got = 1;
+	}
 
-	nr_got = find_get_pages_contig(mapping, index, nr, pages);
-	if (nr_got)
-		goto got_pages;
-	err = filemap_new_page(iocb, iter, &pages[0]);
-	if (!err)
-		nr_got = 1;
-got_pages:
 	for (i = 0; i < nr_got; i++) {
 		err = filemap_make_page_uptodate(iocb, iter, pages[i],
 				index + i, i == 0);
-- 
2.28.0

[PATCH 07/13] mm: refactor generic_file_buffered_read_get_pages

[Christoph Hellwig]

Move the call to filemap_make_page_uptodate for a newly allocated page
into filemap_new_page, which turns the new vs lookup decision into a
plain if / else statement, rename two identifier to be more obvious
and the function itself to filemap_read_pages which describes
it a little better while being much shorter.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 52 ++++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 0af7ddaa0fe7ba..96855299247c56 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2320,7 +2320,10 @@ static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
 		return error;
 	}
 
-	return filemap_readpage(iocb, *page);
+	error = filemap_readpage(iocb, *page);
+	if (error)
+		return error;
+	return filemap_make_page_uptodate(iocb, iter, *page, index, true);
 }
 
 static int filemap_find_get_pages(struct kiocb *iocb, struct iov_iter *iter,
@@ -2344,40 +2347,38 @@ static int filemap_find_get_pages(struct kiocb *iocb, struct iov_iter *iter,
 	return find_get_pages_contig(mapping, index, nr, pages);
 }
 
-static int generic_file_buffered_read_get_pages(struct kiocb *iocb,
-						struct iov_iter *iter,
-						struct page **pages,
-						unsigned int nr)
+static int filemap_read_pages(struct kiocb *iocb, struct iov_iter *iter,
+		struct page **pages, unsigned int nr)
 {
 	pgoff_t index = iocb->ki_pos >> PAGE_SHIFT;
-	int i, j, nr_got, err = 0;
+	int nr_pages, err = 0, i, j;
 
-find_page:
+retry:
 	if (fatal_signal_pending(current))
 		return -EINTR;
 
-	nr_got = filemap_find_get_pages(iocb, iter, pages, nr);
-	if (!nr_got) {
+	nr_pages = filemap_find_get_pages(iocb, iter, pages, nr);
+	if (nr_pages) {
+		for (i = 0; i < nr_pages; i++) {
+			err = filemap_make_page_uptodate(iocb, iter, pages[i],
+					index + i, i == 0);
+			if (err) {
+				for (j = i + 1; j < nr_pages; j++)
+					put_page(pages[j]);
+				nr_pages = i;
+				break;
+			}
+		}
+	} else {
 		err = filemap_new_page(iocb, iter, &pages[0]);
 		if (!err)
-			nr_got = 1;
+			nr_pages = 1;
 	}
 
-	for (i = 0; i < nr_got; i++) {
-		err = filemap_make_page_uptodate(iocb, iter, pages[i],
-				index + i, i == 0);
-		if (err) {
-			for (j = i + 1; j < nr_got; j++)
-				put_page(pages[j]);
-			nr_got = i;
-			break;
-		}
-	}
-
-	if (likely(nr_got))
-		return nr_got;
+	if (likely(nr_pages))
+		return nr_pages;
 	if (err == -EEXIST || err == AOP_TRUNCATED_PAGE)
-		goto find_page;
+		goto retry;
 	return err;
 }
 
@@ -2436,8 +2437,7 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
 			iocb->ki_flags |= IOCB_NOWAIT;
 
 		i = 0;
-		pg_nr = generic_file_buffered_read_get_pages(iocb, iter,
-							     pages, nr_pages);
+		pg_nr = filemap_read_pages(iocb, iter, pages, nr_pages);
 		if (pg_nr < 0) {
 			error = pg_nr;
 			break;
-- 
2.28.0

Re: [PATCH 07/13] mm: refactor generic_file_buffered_read_get_pages

[Matthew Wilcox]

On Sat, Oct 31, 2020 at 09:59:58AM +0100, Christoph Hellwig wrote:
> Move the call to filemap_make_page_uptodate for a newly allocated page
> into filemap_new_page, which turns the new vs lookup decision into a
> plain if / else statement, rename two identifier to be more obvious
> and the function itself to filemap_read_pages which describes
> it a little better while being much shorter.

We don't need to do this -- filemap_read_page waits for the page to
become unlocked, so we already know that it's uptodate (or an error!)
and we know it isn't going to have the readahead bit set.

[PATCH 08/13] mm: move putting the page on error out of filemap_readpage

[Christoph Hellwig]

Move the put_page on error from filemap_readpage into the callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 96855299247c56..6089f1d9dd429f 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2171,11 +2171,11 @@ static int lock_page_for_iocb(struct kiocb *iocb, struct page *page)
 static int filemap_readpage(struct kiocb *iocb, struct page *page)
 {
 	struct file *file = iocb->ki_filp;
-	int error = -EAGAIN;
+	int error;
 
 	if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT)) {
 		unlock_page(page);
-		goto out_put_page;
+		return -EAGAIN;
 	}
 
 	/*
@@ -2186,12 +2186,12 @@ static int filemap_readpage(struct kiocb *iocb, struct page *page)
 	/* Start the actual read. The read will unlock the page. */
 	error = file->f_mapping->a_ops->readpage(file, page);
 	if (unlikely(error))
-		goto out_put_page;
+		return error;
 
 	if (!PageUptodate(page)) {
 		error = lock_page_for_iocb(iocb, page);
 		if (unlikely(error))
-			goto out_put_page;
+			return error;
 
 		if (!PageUptodate(page)) {
 			if (page->mapping == NULL) {
@@ -2199,22 +2199,16 @@ static int filemap_readpage(struct kiocb *iocb, struct page *page)
 				 * invalidate_mapping_pages got it
 				 */
 				unlock_page(page);
-				error = AOP_TRUNCATED_PAGE;
-				goto out_put_page;
+				return AOP_TRUNCATED_PAGE;
 			}
 			unlock_page(page);
 			shrink_readahead_size_eio(&file->f_ra);
-			error = -EIO;
-			goto out_put_page;
+			return -EIO;
 		}
 
 		unlock_page(page);
 	}
 	return 0;
-
-out_put_page:
-	put_page(page);
-	return error;
 }
 
 static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
@@ -2293,7 +2287,10 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	/* Did somebody else fill it already? */
 	if (PageUptodate(page))
 		goto unlock_page;
-	return filemap_readpage(iocb, page);
+	error = filemap_readpage(iocb, page);
+	if (error)
+		goto put_page;
+	return 0;
 
 put_page:
 	put_page(page);
@@ -2315,15 +2312,15 @@ static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
 	if (!page)
 		return -ENOMEM;
 	error = add_to_page_cache_lru(*page, mapping, index, gfp);
-	if (error) {
-		put_page(*page);
-		return error;
-	}
-
+	if (error)
+		goto put_page;
 	error = filemap_readpage(iocb, *page);
 	if (error)
-		return error;
+		goto put_page;
 	return filemap_make_page_uptodate(iocb, iter, *page, index, true);
+put_page:
+	put_page(*page);
+	return error;
 }
 
 static int filemap_find_get_pages(struct kiocb *iocb, struct iov_iter *iter,
-- 
2.28.0

[PATCH 09/13] mm: move putting the page on error out of filemap_make_page_uptodate

[Christoph Hellwig]

Move the put_page on error from filemap_make_page_uptodate into the
callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 6089f1d9dd429f..5f4937715689e7 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2219,11 +2219,11 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	loff_t last = iocb->ki_pos + iter->count;
 	pgoff_t last_index = (last + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);
-	int error = -EAGAIN;
+	int error;
 
 	if (PageReadahead(page)) {
 		if (iocb->ki_flags & IOCB_NOIO)
-			goto put_page;
+			return -EAGAIN;
 		page_cache_async_readahead(mapping, &file->f_ra, file, page,
 					   pg_index, last_index - pg_index);
 	}
@@ -2232,7 +2232,7 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 		return 0;
 
 	if (iocb->ki_flags & IOCB_NOWAIT)
-		goto put_page;
+		return -EAGAIN;
 
 	/*
 	 * See comment in do_read_cache_page on why wait_on_page_locked is used
@@ -2240,13 +2240,13 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	 */
 	if (iocb->ki_flags & IOCB_WAITQ) {
 		if (!first)
-			goto put_page;
+			return -EAGAIN;
 		error = wait_on_page_locked_async(page, iocb->ki_waitq);
 	} else {
 		error = wait_on_page_locked_killable(page);
 	}
 	if (unlikely(error))
-		goto put_page;
+		return error;
 
 	if (PageUptodate(page))
 		return 0;
@@ -2274,27 +2274,19 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	/* Get exclusive access to the page ... */
 	error = lock_page_for_iocb(iocb, page);
 	if (unlikely(error))
-		goto put_page;
+		return error;
 
 page_not_up_to_date_locked:
 	/* Did it get truncated before we got the lock? */
 	if (!page->mapping) {
 		unlock_page(page);
-		error = AOP_TRUNCATED_PAGE;
-		goto put_page;
+		return AOP_TRUNCATED_PAGE;
 	}
 
 	/* Did somebody else fill it already? */
 	if (PageUptodate(page))
 		goto unlock_page;
-	error = filemap_readpage(iocb, page);
-	if (error)
-		goto put_page;
-	return 0;
-
-put_page:
-	put_page(page);
-	return error;
+	return filemap_readpage(iocb, page);
 }
 
 static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
@@ -2317,7 +2309,10 @@ static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
 	error = filemap_readpage(iocb, *page);
 	if (error)
 		goto put_page;
-	return filemap_make_page_uptodate(iocb, iter, *page, index, true);
+	error = filemap_make_page_uptodate(iocb, iter, *page, index, true);
+	if (error)
+		goto put_page;
+	return 0;
 put_page:
 	put_page(*page);
 	return error;
@@ -2360,7 +2355,7 @@ static int filemap_read_pages(struct kiocb *iocb, struct iov_iter *iter,
 			err = filemap_make_page_uptodate(iocb, iter, pages[i],
 					index + i, i == 0);
 			if (err) {
-				for (j = i + 1; j < nr_pages; j++)
+				for (j = i; j < nr_pages; j++)
 					put_page(pages[j]);
 				nr_pages = i;
 				break;
-- 
2.28.0

[PATCH 10/13] mm: open code readahead in filemap_new_page

[Christoph Hellwig]

Calling filemap_make_page_uptodate right after filemap_readpage in
filemap_new_page is rather counterintuitive.  The call is in fact
only needed to issue async readahead, and is guaranteed to return
just after that because the page is uptodate.  Just open code the
readahead related parts of filemap_make_page_uptodate instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 5f4937715689e7..000f75cd359d1c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2309,9 +2309,14 @@ static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
 	error = filemap_readpage(iocb, *page);
 	if (error)
 		goto put_page;
-	error = filemap_make_page_uptodate(iocb, iter, *page, index, true);
-	if (error)
-		goto put_page;
+	if (PageReadahead(*page)) {
+		error = -EAGAIN;
+		if (iocb->ki_flags & IOCB_NOIO)
+			goto put_page;
+		page_cache_async_readahead(mapping, &iocb->ki_filp->f_ra,
+				iocb->ki_filp, *page, index,
+				(iter->count + PAGE_SIZE - 1) >> PAGE_SHIFT);
+	}
 	return 0;
 put_page:
 	put_page(*page);
-- 
2.28.0

Re: [PATCH 10/13] mm: open code readahead in filemap_new_page

[Matthew Wilcox]

On Sat, Oct 31, 2020 at 10:00:01AM +0100, Christoph Hellwig wrote:
> Calling filemap_make_page_uptodate right after filemap_readpage in
> filemap_new_page is rather counterintuitive.  The call is in fact
> only needed to issue async readahead, and is guaranteed to return
> just after that because the page is uptodate.  Just open code the
> readahead related parts of filemap_make_page_uptodate instead.

Oh, you got rid of it again ;-)

It's still not possible for this page to have Readahead set on it --
it was only just created, and can't possibly have been created by
an earlier readahead call.

[PATCH 11/13] mm: streamline the partially uptodate checks in filemap_make_page_uptodate

[Christoph Hellwig]

Unwind the goto mess a bit.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 000f75cd359d1c..904b0a4fb9e008 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2218,7 +2218,6 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	struct address_space *mapping = file->f_mapping;
 	loff_t last = iocb->ki_pos + iter->count;
 	pgoff_t last_index = (last + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);
 	int error;
 
 	if (PageReadahead(page)) {
@@ -2251,32 +2250,22 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	if (PageUptodate(page))
 		return 0;
 
-	if (mapping->host->i_blkbits == PAGE_SHIFT ||
-	    !mapping->a_ops->is_partially_uptodate)
-		goto page_not_up_to_date;
-	/* pipes can't handle partially uptodate pages */
-	if (unlikely(iov_iter_is_pipe(iter)))
-		goto page_not_up_to_date;
-	if (!trylock_page(page))
-		goto page_not_up_to_date;
-	/* Did it get truncated before we got the lock? */
-	if (!page->mapping)
-		goto page_not_up_to_date_locked;
-	if (!mapping->a_ops->is_partially_uptodate(page, pos & ~PAGE_MASK,
-			last - pos))
-		goto page_not_up_to_date_locked;
-
-unlock_page:
-	unlock_page(page);
-	return 0;
+	if (mapping->host->i_blkbits <= PAGE_SHIFT &&
+	    mapping->a_ops->is_partially_uptodate &&
+	    !iov_iter_is_pipe(iter) &&
+	    trylock_page(page)) {
+		loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);
 
-page_not_up_to_date:
-	/* Get exclusive access to the page ... */
-	error = lock_page_for_iocb(iocb, page);
-	if (unlikely(error))
-		return error;
+		if (page->mapping &&
+		    mapping->a_ops->is_partially_uptodate(page,
+				pos & ~PAGE_MASK, last - pos))
+			goto unlock_page;
+	} else {
+		error = lock_page_for_iocb(iocb, page);
+		if (unlikely(error))
+			return error;
+	}
 
-page_not_up_to_date_locked:
 	/* Did it get truncated before we got the lock? */
 	if (!page->mapping) {
 		unlock_page(page);
@@ -2287,6 +2276,9 @@ static int filemap_make_page_uptodate(struct kiocb *iocb, struct iov_iter *iter,
 	if (PageUptodate(page))
 		goto unlock_page;
 	return filemap_readpage(iocb, page);
+unlock_page:
+	unlock_page(page);
+	return 0;
 }
 
 static int filemap_new_page(struct kiocb *iocb, struct iov_iter *iter,
-- 
2.28.0

Re: [PATCH 11/13] mm: streamline the partially uptodate checks in filemap_make_page_uptodate

[Matthew Wilcox]

On Sat, Oct 31, 2020 at 10:00:02AM +0100, Christoph Hellwig wrote:
> +	if (mapping->host->i_blkbits <= PAGE_SHIFT &&
> +	    mapping->a_ops->is_partially_uptodate &&
> +	    !iov_iter_is_pipe(iter) &&
> +	    trylock_page(page)) {
> +		loff_t pos = max(iocb->ki_pos, (loff_t)pg_index << PAGE_SHIFT);

I think the cure is worse than the disease here ;-)

Fortunately, I simplified this here:
https://git.infradead.org/users/willy/pagecache.git/commitdiff/c6b5b2540b6db91d3c8928c8ed1b5d72a402215a

by getting the page lock earlier (or dropping the reference to the page,
waiting for it to become unlocked and starting the lookup again)

[PATCH 12/13] mm: rename generic_file_buffered_read to filemap_read

[Christoph Hellwig]

Rename generic_file_buffered_read to match the naming of filemap_fault,
also update the written parameter to a more descriptive name and
improve the kerneldoc comment.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/file.c    |  2 +-
 include/linux/fs.h |  4 ++--
 mm/filemap.c       | 34 ++++++++++++++++------------------
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 87355a38a65470..1a4913e1fd1289 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3633,7 +3633,7 @@ static ssize_t btrfs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 			return ret;
 	}
 
-	return generic_file_buffered_read(iocb, to, ret);
+	return filemap_read(iocb, to, ret);
 }
 
 const struct file_operations btrfs_file_operations = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8d559d43f2af92..a79f65607236ae 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2948,8 +2948,8 @@ extern ssize_t generic_write_checks(struct kiocb *, struct iov_iter *);
 extern int generic_write_check_limits(struct file *file, loff_t pos,
 		loff_t *count);
 extern int generic_file_rw_checks(struct file *file_in, struct file *file_out);
-extern ssize_t generic_file_buffered_read(struct kiocb *iocb,
-		struct iov_iter *to, ssize_t already_read);
+ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *to,
+		ssize_t already_read);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);
diff --git a/mm/filemap.c b/mm/filemap.c
index 904b0a4fb9e008..743d764f3eab1c 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2372,23 +2372,21 @@ static int filemap_read_pages(struct kiocb *iocb, struct iov_iter *iter,
 }
 
 /**
- * generic_file_buffered_read - generic file read routine
- * @iocb:	the iocb to read
- * @iter:	data destination
- * @written:	already copied
+ * filemap_read - read data from the page cache
+ * @iocb:		the iocb to read
+ * @iter:		data destination
+ * @already_read:	number of bytes already read by the caller
  *
- * This is a generic file read routine, and uses the
- * mapping->a_ops->readpage() function for the actual low-level stuff.
- *
- * This is really ugly. But the goto's actually try to clarify some
- * of the logic when it comes to error handling etc.
+ * Read data from the pagecache using the ->readpage address space
+ * operation.
  *
  * Return:
- * * total number of bytes copied, including those the were already @written
- * * negative error code if nothing was copied
+ *  Total number of bytes copied, including those already read by the caller as
+ *  passed in the @already_read argument.  Negative error code if an error
+ *  happened before any bytes were copied.
  */
-ssize_t generic_file_buffered_read(struct kiocb *iocb,
-		struct iov_iter *iter, ssize_t written)
+ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
+		ssize_t already_read)
 {
 	struct file *filp = iocb->ki_filp;
 	struct file_ra_state *ra = &filp->f_ra;
@@ -2422,7 +2420,7 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
 		 * can no longer safely return -EIOCBQUEUED. Hence mark
 		 * an async read NOWAIT at that point.
 		 */
-		if ((iocb->ki_flags & IOCB_WAITQ) && written)
+		if ((iocb->ki_flags & IOCB_WAITQ) && already_read)
 			iocb->ki_flags |= IOCB_NOWAIT;
 
 		i = 0;
@@ -2482,7 +2480,7 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
 
 			copied = copy_page_to_iter(pages[i], offset, bytes, iter);
 
-			written += copied;
+			already_read += copied;
 			iocb->ki_pos += copied;
 			ra->prev_pos = iocb->ki_pos;
 
@@ -2501,9 +2499,9 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
 	if (pages != pages_onstack)
 		kfree(pages);
 
-	return written ? written : error;
+	return already_read ? already_read : error;
 }
-EXPORT_SYMBOL_GPL(generic_file_buffered_read);
+EXPORT_SYMBOL_GPL(filemap_read);
 
 /**
  * generic_file_read_iter - generic filesystem read routine
@@ -2577,7 +2575,7 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 			goto out;
 	}
 
-	retval = generic_file_buffered_read(iocb, iter, retval);
+	retval = filemap_read(iocb, iter, retval);
 out:
 	return retval;
 }
-- 
2.28.0

[PATCH 13/13] mm: simplify generic_file_read_iter

[Christoph Hellwig]

Avoid the pointless goto out just for returning retval.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 743d764f3eab1c..b45f0bafdbaebf 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2531,7 +2531,7 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	ssize_t retval = 0;
 
 	if (!count)
-		goto out; /* skip atime */
+		return 0; /* skip atime */
 
 	if (iocb->ki_flags & IOCB_DIRECT) {
 		struct file *file = iocb->ki_filp;
@@ -2549,7 +2549,7 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 						iocb->ki_pos,
 					        iocb->ki_pos + count - 1);
 			if (retval < 0)
-				goto out;
+				return retval;
 		}
 
 		file_accessed(file);
@@ -2572,12 +2572,10 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		 */
 		if (retval < 0 || !count || iocb->ki_pos >= size ||
 		    IS_DAX(inode))
-			goto out;
+			return retval;
 	}
 
-	retval = filemap_read(iocb, iter, retval);
-out:
-	return retval;
+	return filemap_read(iocb, iter, retval);
 }
 EXPORT_SYMBOL(generic_file_read_iter);
 
-- 
2.28.0

Re: clean up the generic pagecache read helpers

[Matthew Wilcox]

On Sat, Oct 31, 2020 at 09:59:51AM +0100, Christoph Hellwig wrote:
> Hi Andrew,
> 
> this series cleans up refactors our generic read from page cache helpers.
> The recent usage of the gang lookups helped a fair amount compared to
> the previous state, but left some of the old mess and actually introduced
> some new rather odd calling conventions as well.
> 
> Matthew: I think this should actually help with your THP work even if it
> means another rebase.  I was a bit surprised how quickly the gang lookups
> went in as well.

I think this is 90% the same direction I'm heading in.  I'll chime in
with a few suggestions on the individual patches.