| Message ID | 20201102184312.25926-16-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor generic_file_buffered_read | expand |
On Mon, Nov 02, 2020 at 06:43:10PM +0000, Matthew Wilcox (Oracle) wrote: > We don't need to get the page lock again; we just need to wait for > the I/O to finish, so use wait_on_page_locked_killable() like the > other callers of ->readpage. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Kent Overstreet <kent.overstreet@gmail.com>
On Mon, Nov 02, 2020 at 06:43:10PM +0000, Matthew Wilcox (Oracle) wrote: > We don't need to get the page lock again; we just need to wait for > the I/O to finish, so use wait_on_page_locked_killable() like the > other callers of ->readpage. As that isn't entirely obvious, what about adding a comment next to the wait_on_page_locked_killable call to document it? > > + error = wait_on_page_locked_killable(page); > if (error) > return error; > + if (PageUptodate(page)) > + return 0; > + if (!page->mapping) /* page truncated */ > + return AOP_TRUNCATED_PAGE; > + shrink_readahead_size_eio(&file->f_ra); > + return -EIO; You might notice a theme here, but I hate having the fast path exit early without a good reason, so I'd be much happier with: if (!PageUptodate(page)) { if (!page->mapping) /* page truncated */ return AOP_TRUNCATED_PAGE; shrink_readahead_size_eio(&file->f_ra); return -EIO; } return 0;
On Tue, Nov 03, 2020 at 09:00:45AM +0100, Christoph Hellwig wrote: > On Mon, Nov 02, 2020 at 06:43:10PM +0000, Matthew Wilcox (Oracle) wrote: > > We don't need to get the page lock again; we just need to wait for > > the I/O to finish, so use wait_on_page_locked_killable() like the > > other callers of ->readpage. > > As that isn't entirely obvious, what about adding a comment next to > the wait_on_page_locked_killable call to document it? The other callers of ->readpage don't document that, so not sure why we should here? > > + error = wait_on_page_locked_killable(page); > > if (error) > > return error; > > + if (PageUptodate(page)) > > + return 0; > > + if (!page->mapping) /* page truncated */ > > + return AOP_TRUNCATED_PAGE; > > + shrink_readahead_size_eio(&file->f_ra); > > + return -EIO; > > You might notice a theme here, but I hate having the fast path exit > early without a good reason, so I'd be much happier with: > > if (!PageUptodate(page)) { > if (!page->mapping) /* page truncated */ > return AOP_TRUNCATED_PAGE; > shrink_readahead_size_eio(&file->f_ra); > return -EIO; > } > return 0; I'm just not a fan of unnecessary indentation.
On Tue, Nov 03, 2020 at 03:24:36PM +0000, Matthew Wilcox wrote: > On Tue, Nov 03, 2020 at 09:00:45AM +0100, Christoph Hellwig wrote: > > On Mon, Nov 02, 2020 at 06:43:10PM +0000, Matthew Wilcox (Oracle) wrote: > > > We don't need to get the page lock again; we just need to wait for > > > the I/O to finish, so use wait_on_page_locked_killable() like the > > > other callers of ->readpage. > > > > As that isn't entirely obvious, what about adding a comment next to > > the wait_on_page_locked_killable call to document it? > > The other callers of ->readpage don't document that, so not sure why > we should here? The callers of ->readpage are a mess :( Many use lock_page or trylock_page, one uses wait_on_page_locked directly, another one uses the wait_on_page_read helper. I think we need a good helper here, and I think it looks a lot like filemap_read_page in your tree..
On Tue, Nov 03, 2020 at 06:13:56PM +0100, Christoph Hellwig wrote: > On Tue, Nov 03, 2020 at 03:24:36PM +0000, Matthew Wilcox wrote: > > On Tue, Nov 03, 2020 at 09:00:45AM +0100, Christoph Hellwig wrote: > > > On Mon, Nov 02, 2020 at 06:43:10PM +0000, Matthew Wilcox (Oracle) wrote: > > > > We don't need to get the page lock again; we just need to wait for > > > > the I/O to finish, so use wait_on_page_locked_killable() like the > > > > other callers of ->readpage. > > > > > > As that isn't entirely obvious, what about adding a comment next to > > > the wait_on_page_locked_killable call to document it? > > > > The other callers of ->readpage don't document that, so not sure why > > we should here? > > The callers of ->readpage are a mess :( > > Many use lock_page or trylock_page, one uses wait_on_page_locked directly, > another one uses the wait_on_page_read helper. I think we need a good > helper here, and I think it looks a lot like filemap_read_page in your > tree.. Oh, heh. Turns out I wrote this in a patch series the other day ... static int mapping_readpage(struct file *file, struct address_space *mapping, struct page *page, bool synchronous) { struct readahead_control ractl = { .file = file, .mapping = mapping, ._index = page->index, ._nr_pages = 1, }; int ret; if (!synchronous && mapping->a_ops->readahead) { mapping->a_ops->readahead(&ractl); return 0; } ret = mapping->a_ops->readpage(file, page); if (ret != AOP_UPDATED_PAGE) return ret; unlock_page(page); return 0; } (it's for swap_readpage, which is generally called in an async manner ... and really doesn't handle errors at all well) This does illustrate that we need the mapping argument. Some of the callers of ->readpage need to pass a NULL file pointer, so we can't get it from file->f_mapping.
diff --git a/mm/filemap.c b/mm/filemap.c index f16b1eb03bca..f2de97d51441 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2209,23 +2209,16 @@ static int filemap_read_page(struct file *file, struct address_space *mapping, error = mapping->a_ops->readpage(file, page); if (error) return error; - if (PageUptodate(page)) - return 0; - error = lock_page_killable(page); + error = wait_on_page_locked_killable(page); if (error) return error; - if (!PageUptodate(page)) { - if (page->mapping == NULL) { - /* page truncated */ - error = AOP_TRUNCATED_PAGE; - } else { - shrink_readahead_size_eio(&file->f_ra); - error = -EIO; - } - } - unlock_page(page); - return error; + if (PageUptodate(page)) + return 0; + if (!page->mapping) /* page truncated */ + return AOP_TRUNCATED_PAGE; + shrink_readahead_size_eio(&file->f_ra); + return -EIO; } static bool filemap_range_uptodate(struct kiocb *iocb,
We don't need to get the page lock again; we just need to wait for the I/O to finish, so use wait_on_page_locked_killable() like the other callers of ->readpage. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/filemap.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)