| Message ID | 20211208042256.1923824-26-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Folios for 5.17 | expand |
> thn makes up for the extra 100 bytes of text added to the various s/thn/ > return read_cache_page(mapping, index, NULL, data); > } > > +static inline struct folio *read_mapping_folio(struct address_space *mapping, > + pgoff_t index, void *data) > +{ > + return read_cache_folio(mapping, index, NULL, data); > +} Is there much of a point in this wrapper? > +static struct page *do_read_cache_page(struct address_space *mapping, > + pgoff_t index, filler_t *filler, void *data, gfp_t gfp) > +{ > + struct folio *folio = read_cache_folio(mapping, index, filler, data); > + if (IS_ERR(folio)) > + return &folio->page; > + return folio_file_page(folio, index); > +} This drops the gfp on the floor.
On Thu, Dec 23, 2021 at 08:39:59AM +0100, Christoph Hellwig wrote: > > thn makes up for the extra 100 bytes of text added to the various > > s/thn/ > > > return read_cache_page(mapping, index, NULL, data); > > } > > > > +static inline struct folio *read_mapping_folio(struct address_space *mapping, > > + pgoff_t index, void *data) > > +{ > > + return read_cache_folio(mapping, index, NULL, data); > > +} > > Is there much of a point in this wrapper? Comparing read_mapping_page() vs read_cache_page(), 4 callers vs ~50. It's definitely the preferred form, so I'd rather keep it. > > +static struct page *do_read_cache_page(struct address_space *mapping, > > + pgoff_t index, filler_t *filler, void *data, gfp_t gfp) > > +{ > > + struct folio *folio = read_cache_folio(mapping, index, filler, data); > > + if (IS_ERR(folio)) > > + return &folio->page; > > + return folio_file_page(folio, index); > > +} > > This drops the gfp on the floor. Oops. Will fix.
On Thu, Dec 23, 2021 at 03:18:34PM +0000, Matthew Wilcox wrote: > > > +static struct page *do_read_cache_page(struct address_space *mapping, > > > + pgoff_t index, filler_t *filler, void *data, gfp_t gfp) > > > +{ > > > + struct folio *folio = read_cache_folio(mapping, index, filler, data); > > > + if (IS_ERR(folio)) > > > + return &folio->page; > > > + return folio_file_page(folio, index); > > > +} > > > > This drops the gfp on the floor. > > Oops. Will fix. For the record, this fix: - struct folio *folio = read_cache_folio(mapping, index, filler, data); + struct folio *folio; + + folio = do_read_cache_folio(mapping, index, filler, data, gfp);
On Wed, Dec 08, 2021 at 04:22:33AM +0000, Matthew Wilcox (Oracle) wrote: > @@ -3503,23 +3491,23 @@ static struct page *do_read_cache_page(struct address_space *mapping, > * avoid spurious serialisations and wakeups when multiple processes > * wait on the same page for IO to complete. > */ > - wait_on_page_locked(page); > - if (PageUptodate(page)) > + folio_wait_locked(folio); > + if (folio_test_uptodate(folio)) > goto out; > > /* Distinguish between all the cases under the safety of the lock */ > - lock_page(page); > + folio_lock(folio); > > /* Case c or d, restart the operation */ > - if (!page->mapping) { > - unlock_page(page); > - put_page(page); > + if (!folio->mapping) { > + folio_unlock(folio); > + folio_put(folio); > goto repeat; > } Re-reviewing this patch after Christoph's feedback, I believe it is wrong to sleep with the refcount elevated, waiting for someone else to unlock the page. Doing that prevents anyone from splitting the folio, which can be annoying. So I'm thinking about adding this patch (as a follow-on). It brings the code closer to the read_iter path, which is always a good thing. The comment was going to be made untrue by this patch, and I tried rewriting it, but concluded ultimately that it was more distracting than informative. diff --git a/mm/filemap.c b/mm/filemap.c index 6f541d931a4c..33077c264d79 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3451,45 +3451,12 @@ static struct folio *do_read_cache_folio(struct address_space *mapping, if (folio_test_uptodate(folio)) goto out; - /* - * Page is not up to date and may be locked due to one of the following - * case a: Page is being filled and the page lock is held - * case b: Read/write error clearing the page uptodate status - * case c: Truncation in progress (page locked) - * case d: Reclaim in progress - * - * Case a, the page will be up to date when the page is unlocked. - * There is no need to serialise on the page lock here as the page - * is pinned so the lock gives no additional protection. Even if the - * page is truncated, the data is still valid if PageUptodate as - * it's a race vs truncate race. - * Case b, the page will not be up to date - * Case c, the page may be truncated but in itself, the data may still - * be valid after IO completes as it's a read vs truncate race. The - * operation must restart if the page is not uptodate on unlock but - * otherwise serialising on page lock to stabilise the mapping gives - * no additional guarantees to the caller as the page lock is - * released before return. - * Case d, similar to truncation. If reclaim holds the page lock, it - * will be a race with remove_mapping that determines if the mapping - * is valid on unlock but otherwise the data is valid and there is - * no need to serialise with page lock. - * - * As the page lock gives no additional guarantee, we optimistically - * wait on the page to be unlocked and check if it's up to date and - * use the page if it is. Otherwise, the page lock is required to - * distinguish between the different cases. The motivation is that we - * avoid spurious serialisations and wakeups when multiple processes - * wait on the same page for IO to complete. - */ - folio_wait_locked(folio); - if (folio_test_uptodate(folio)) - goto out; - - /* Distinguish between all the cases under the safety of the lock */ - folio_lock(folio); + if (!folio_trylock(folio)) { + folio_put_wait_locked(folio, TASK_UNINTERRUPTIBLE); + goto repeat; + } - /* Case c or d, restart the operation */ + /* Folio was truncated from mapping */ if (!folio->mapping) { folio_unlock(folio); folio_put(folio);
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index 30302be6977f..7bef50ea5435 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -629,8 +629,10 @@ static inline struct page *grab_cache_page(struct address_space *mapping, return find_or_create_page(mapping, index, mapping_gfp_mask(mapping)); } -extern struct page * read_cache_page(struct address_space *mapping, - pgoff_t index, filler_t *filler, void *data); +struct folio *read_cache_folio(struct address_space *, pgoff_t index, + filler_t *filler, void *data); +struct page *read_cache_page(struct address_space *, pgoff_t index, + filler_t *filler, void *data); extern struct page * read_cache_page_gfp(struct address_space *mapping, pgoff_t index, gfp_t gfp_mask); extern int read_cache_pages(struct address_space *mapping, @@ -642,6 +644,12 @@ static inline struct page *read_mapping_page(struct address_space *mapping, return read_cache_page(mapping, index, NULL, data); } +static inline struct folio *read_mapping_folio(struct address_space *mapping, + pgoff_t index, void *data) +{ + return read_cache_folio(mapping, index, NULL, data); +} + /* * Get index of the page within radix-tree (but not for hugetlb pages). * (TODO: remove once hugetlb pages will have ->index in PAGE_SIZE) diff --git a/mm/filemap.c b/mm/filemap.c index fc0f1d9904d2..f34dda0a7627 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3418,35 +3418,20 @@ EXPORT_SYMBOL(filemap_page_mkwrite); EXPORT_SYMBOL(generic_file_mmap); EXPORT_SYMBOL(generic_file_readonly_mmap); -static struct page *wait_on_page_read(struct page *page) +static struct folio *do_read_cache_folio(struct address_space *mapping, + pgoff_t index, filler_t filler, void *data, gfp_t gfp) { - if (!IS_ERR(page)) { - wait_on_page_locked(page); - if (!PageUptodate(page)) { - put_page(page); - page = ERR_PTR(-EIO); - } - } - return page; -} - -static struct page *do_read_cache_page(struct address_space *mapping, - pgoff_t index, - int (*filler)(void *, struct page *), - void *data, - gfp_t gfp) -{ - struct page *page; + struct folio *folio; int err; repeat: - page = find_get_page(mapping, index); - if (!page) { - page = __page_cache_alloc(gfp); - if (!page) + folio = filemap_get_folio(mapping, index); + if (!folio) { + folio = filemap_alloc_folio(gfp, 0); + if (!folio) return ERR_PTR(-ENOMEM); - err = add_to_page_cache_lru(page, mapping, index, gfp); + err = filemap_add_folio(mapping, folio, index, gfp); if (unlikely(err)) { - put_page(page); + folio_put(folio); if (err == -EEXIST) goto repeat; /* Presumably ENOMEM for xarray node */ @@ -3455,21 +3440,24 @@ static struct page *do_read_cache_page(struct address_space *mapping, filler: if (filler) - err = filler(data, page); + err = filler(data, &folio->page); else - err = mapping->a_ops->readpage(data, page); + err = mapping->a_ops->readpage(data, &folio->page); if (err < 0) { - put_page(page); + folio_put(folio); return ERR_PTR(err); } - page = wait_on_page_read(page); - if (IS_ERR(page)) - return page; + folio_wait_locked(folio); + if (!folio_test_uptodate(folio)) { + folio_put(folio); + return ERR_PTR(-EIO); + } + goto out; } - if (PageUptodate(page)) + if (folio_test_uptodate(folio)) goto out; /* @@ -3503,23 +3491,23 @@ static struct page *do_read_cache_page(struct address_space *mapping, * avoid spurious serialisations and wakeups when multiple processes * wait on the same page for IO to complete. */ - wait_on_page_locked(page); - if (PageUptodate(page)) + folio_wait_locked(folio); + if (folio_test_uptodate(folio)) goto out; /* Distinguish between all the cases under the safety of the lock */ - lock_page(page); + folio_lock(folio); /* Case c or d, restart the operation */ - if (!page->mapping) { - unlock_page(page); - put_page(page); + if (!folio->mapping) { + folio_unlock(folio); + folio_put(folio); goto repeat; } /* Someone else locked and filled the page in a very small window */ - if (PageUptodate(page)) { - unlock_page(page); + if (folio_test_uptodate(folio)) { + folio_unlock(folio); goto out; } @@ -3529,16 +3517,16 @@ static struct page *do_read_cache_page(struct address_space *mapping, * Clear page error before actual read, PG_error will be * set again if read page fails. */ - ClearPageError(page); + folio_clear_error(folio); goto filler; out: - mark_page_accessed(page); - return page; + folio_mark_accessed(folio); + return folio; } /** - * read_cache_page - read into page cache, fill it if needed + * read_cache_folio - read into page cache, fill it if needed * @mapping: the page's address_space * @index: the page index * @filler: function to perform the read @@ -3553,10 +3541,25 @@ static struct page *do_read_cache_page(struct address_space *mapping, * * Return: up to date page on success, ERR_PTR() on failure. */ +struct folio *read_cache_folio(struct address_space *mapping, pgoff_t index, + filler_t filler, void *data) +{ + return do_read_cache_folio(mapping, index, filler, data, + mapping_gfp_mask(mapping)); +} +EXPORT_SYMBOL(read_cache_folio); + +static struct page *do_read_cache_page(struct address_space *mapping, + pgoff_t index, filler_t *filler, void *data, gfp_t gfp) +{ + struct folio *folio = read_cache_folio(mapping, index, filler, data); + if (IS_ERR(folio)) + return &folio->page; + return folio_file_page(folio, index); +} + struct page *read_cache_page(struct address_space *mapping, - pgoff_t index, - int (*filler)(void *, struct page *), - void *data) + pgoff_t index, filler_t *filler, void *data) { return do_read_cache_page(mapping, index, filler, data, mapping_gfp_mask(mapping));
Reimplement read_cache_page() as a wrapper around read_cache_folio(). Saves over 400 bytes of text from do_read_cache_folio() which more thn makes up for the extra 100 bytes of text added to the various wrapper functions. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- include/linux/pagemap.h | 12 +++++- mm/filemap.c | 95 +++++++++++++++++++++-------------------- 2 files changed, 59 insertions(+), 48 deletions(-)