diff mbox series

[04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

Message ID 20201031090004.452516-5-hch@lst.de (mailing list archive)
State New
Headers show
Series [01/13] mm: simplify generic_file_buffered_read_readpage | expand

Commit Message

Christoph Hellwig Oct. 31, 2020, 8:59 a.m. UTC
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(-)

Comments

Matthew Wilcox Oct. 31, 2020, 5:06 p.m. UTC | #1
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;
Christoph Hellwig Nov. 1, 2020, 10:31 a.m. UTC | #2
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.
Matthew Wilcox Nov. 1, 2020, 10:49 a.m. UTC | #3
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 ...
Christoph Hellwig Nov. 1, 2020, 10:51 a.m. UTC | #4
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.
Christoph Hellwig Nov. 1, 2020, 10:51 a.m. UTC | #5
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.
Matthew Wilcox Nov. 1, 2020, 11:04 a.m. UTC | #6
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.
Christoph Hellwig Nov. 1, 2020, 11:52 a.m. UTC | #7
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
Matthew Wilcox Nov. 1, 2020, 2:55 p.m. UTC | #8
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.
Christoph Hellwig Nov. 2, 2020, 8:18 a.m. UTC | #9
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.
diff mbox series

Patch

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;
 		}
 	}