| Message ID | 20201031090004.452516-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] mm: simplify generic_file_buffered_read_readpage | expand |
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;
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.
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 ...
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.
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.
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.
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
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.
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 --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; } }
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(-)