| Message ID | 20220214200017.3150590-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various fixes around invalidate_page() | expand |
On 2/14/22 12:00, Matthew Wilcox (Oracle) wrote: > invalidate_inode_page() is the only caller of invalidate_complete_page() > and inlining it reveals that the first check is unnecessary (because we > hold the page locked, and we just retrieved the mapping from the page). I just noticed this yesterday, when reviewing Rik's page poisoning fix. I had a patch for it squirreled away, but I missed the point about removing that extraneous mapping check. Glad you spotted it. ... > @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) It would be nice to retain some of the original comments. May I suggest this (it has an additional paragraph) for an updated version of comments above invalidate_inode_page(): /* * Safely invalidate one page from its pagecache mapping. * It only drops clean, unused pages. The page must be locked. * * This function can be called at any time, and is not supposed to throw away * dirty pages. But pages can be marked dirty at any time too, so use * remove_mapping(), which safely discards clean, unused pages. * * Returns 1 if the page is successfully invalidated, otherwise 0. */ Also, as long as you're there, a newline after the mapping declaration would bring this routine into compliance with that convention. hmmm, now I wonder why this isn't a boolean function. And I think the reason is that it's quite old. Either way, looks good: Reviewed-by: John Hubbard <jhubbard@nvidia.com> thanks,
On Mon, Feb 14, 2022 at 03:09:09PM -0800, John Hubbard wrote: > > @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) > > It would be nice to retain some of the original comments. May I suggest > this (it has an additional paragraph) for an updated version of comments > above invalidate_inode_page(): > > /* > * Safely invalidate one page from its pagecache mapping. > * It only drops clean, unused pages. The page must be locked. > * > * This function can be called at any time, and is not supposed to throw away > * dirty pages. But pages can be marked dirty at any time too, so use > * remove_mapping(), which safely discards clean, unused pages. > * > * Returns 1 if the page is successfully invalidated, otherwise 0. > */ By the end of this series, it becomes: /** * invalidate_inode_page() - Remove an unused page from the pagecache. * @page: The page to remove. * * Safely invalidate one page from its pagecache mapping. * It only drops clean, unused pages. * * Context: Page must be locked. * Return: The number of pages successfully removed. */ > Also, as long as you're there, a newline after the mapping declaration > would bring this routine into compliance with that convention. Again, by the end, we're at: struct folio *folio = page_folio(page); struct address_space *mapping = folio_mapping(folio); /* The page may have been truncated before it was locked */ if (!mapping) return 0; return mapping_shrink_folio(mapping, folio); > hmmm, now I wonder why this isn't a boolean function. And I think the > reason is that it's quite old. We could make this return a bool and have the one user that cares call folio_nr_pages(). I don't have a strong preference. > Either way, looks good: > > Reviewed-by: John Hubbard <jhubbard@nvidia.com> Thanks!
On 2/14/22 3:32 PM, Matthew Wilcox wrote: > On Mon, Feb 14, 2022 at 03:09:09PM -0800, John Hubbard wrote: >>> @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) >> >> It would be nice to retain some of the original comments. May I suggest >> this (it has an additional paragraph) for an updated version of comments >> above invalidate_inode_page(): >> >> /* >> * Safely invalidate one page from its pagecache mapping. >> * It only drops clean, unused pages. The page must be locked. >> * >> * This function can be called at any time, and is not supposed to throw away >> * dirty pages. But pages can be marked dirty at any time too, so use >> * remove_mapping(), which safely discards clean, unused pages. >> * >> * Returns 1 if the page is successfully invalidated, otherwise 0. >> */ > > By the end of this series, it becomes: > > /** > * invalidate_inode_page() - Remove an unused page from the pagecache. > * @page: The page to remove. > * > * Safely invalidate one page from its pagecache mapping. > * It only drops clean, unused pages. > * > * Context: Page must be locked. > * Return: The number of pages successfully removed. > */ OK. > >> Also, as long as you're there, a newline after the mapping declaration >> would bring this routine into compliance with that convention. > > Again, by the end, we're at: > > struct folio *folio = page_folio(page); > struct address_space *mapping = folio_mapping(folio); > > /* The page may have been truncated before it was locked */ > if (!mapping) > return 0; > return mapping_shrink_folio(mapping, folio); > Also good. >> hmmm, now I wonder why this isn't a boolean function. And I think the >> reason is that it's quite old. > > We could make this return a bool and have the one user that cares > call folio_nr_pages(). I don't have a strong preference. Neither do I. No need to add churn for that. thanks,
On Mon, Feb 14, 2022 at 08:00:09PM +0000, Matthew Wilcox (Oracle) wrote: > invalidate_inode_page() is the only caller of invalidate_complete_page() > and inlining it reveals that the first check is unnecessary (because we > hold the page locked, and we just retrieved the mapping from the page). > Actually, it does make a difference, in that tail pages no longer fail > at this check, so it's now possible to remove a tail page from a mapping. There is a comment that referneces invalidate_complete_page in kernel/futex/core.c which needs updating. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 2022/2/15 4:00, Matthew Wilcox (Oracle) wrote: > invalidate_inode_page() is the only caller of invalidate_complete_page() > and inlining it reveals that the first check is unnecessary (because we > hold the page locked, and we just retrieved the mapping from the page). > Actually, it does make a difference, in that tail pages no longer fail > at this check, so it's now possible to remove a tail page from a mapping. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/truncate.c | 28 +++++----------------------- > 1 file changed, 5 insertions(+), 23 deletions(-) > > diff --git a/mm/truncate.c b/mm/truncate.c > index 9dbf0b75da5d..e5e2edaa0b76 100644 > --- a/mm/truncate.c > +++ b/mm/truncate.c > @@ -193,27 +193,6 @@ static void truncate_cleanup_folio(struct folio *folio) > folio_clear_mappedtodisk(folio); > } > > -/* > - * This is for invalidate_mapping_pages(). That function can be called at > - * any time, and is not supposed to throw away dirty pages. But pages can > - * be marked dirty at any time too, so use remove_mapping which safely > - * discards clean, unused pages. > - * > - * Returns non-zero if the page was successfully invalidated. > - */ > -static int > -invalidate_complete_page(struct address_space *mapping, struct page *page) > -{ > - > - if (page->mapping != mapping) > - return 0; > - > - if (page_has_private(page) && !try_to_release_page(page, 0)) > - return 0; > - > - return remove_mapping(mapping, page); > -} > - > int truncate_inode_folio(struct address_space *mapping, struct folio *folio) > { > if (folio->mapping != mapping) > @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) > return 0; > if (page_mapped(page)) > return 0; > - return invalidate_complete_page(mapping, page); It seems the checking of page->mapping != mapping is removed here. IIUC, this would cause possibly unexpected side effect because swapcache page can be invalidate now. I think this function is not intended to deal with swapcache though it could do this. Or am I miss anything? Many thanks! > + if (page_has_private(page) && !try_to_release_page(page, 0)) > + return 0; > + > + return remove_mapping(mapping, page); > } > > /** > @@ -584,7 +566,7 @@ void invalidate_mapping_pagevec(struct address_space *mapping, > } > > /* > - * This is like invalidate_complete_page(), except it ignores the page's > + * This is like invalidate_inode_page(), except it ignores the page's > * refcount. We do this because invalidate_inode_pages2() needs stronger > * invalidation guarantees, and cannot afford to leave pages behind because > * shrink_page_list() has a temp ref on them, or because they're transiently >
On Tue, Feb 15, 2022 at 03:45:34PM +0800, Miaohe Lin wrote: > > @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) > > return 0; > > if (page_mapped(page)) > > return 0; > > - return invalidate_complete_page(mapping, page); > > It seems the checking of page->mapping != mapping is removed here. > IIUC, this would cause possibly unexpected side effect because > swapcache page can be invalidate now. I think this function is > not intended to deal with swapcache though it could do this. You're right that it might now pass instead of being skipped. But it's not currently called for swapcache pages. If we did want to prohibit swapcache pages explicitly, I'd rather we checked the flag instead of relying on page->mapping != page_mapping(page). The intent of that check was "has it been truncated", not "is it swapcache".
On 2022/2/16 4:09, Matthew Wilcox wrote: > On Tue, Feb 15, 2022 at 03:45:34PM +0800, Miaohe Lin wrote: >>> @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) >>> return 0; >>> if (page_mapped(page)) >>> return 0; >>> - return invalidate_complete_page(mapping, page); >> >> It seems the checking of page->mapping != mapping is removed here. >> IIUC, this would cause possibly unexpected side effect because >> swapcache page can be invalidate now. I think this function is >> not intended to deal with swapcache though it could do this. > > You're right that it might now pass instead of being skipped. > But it's not currently called for swapcache pages. If we did want AFAICS, __soft_offline_page might call invalidate_inode_page for swapcache page. It only checks !PageHuge(page). Maybe __soft_offline_page should change to check the flag or maybe it's fine to invalidate swapcache page there. I'm not sure... > to prohibit swapcache pages explicitly, I'd rather we checked the > flag instead of relying on page->mapping != page_mapping(page). Agree. > The intent of that check was "has it been truncated", not "is it > swapcache". Many thanks for clarifying this. > > > . >
On 2022/2/15 4:00, Matthew Wilcox (Oracle) wrote: > invalidate_inode_page() is the only caller of invalidate_complete_page() > and inlining it reveals that the first check is unnecessary (because we > hold the page locked, and we just retrieved the mapping from the page). > Actually, it does make a difference, in that tail pages no longer fail > at this check, so it's now possible to remove a tail page from a mapping. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- LGTM. Thanks. Reviewed-by: Miaohe Lin <linmiaohe@huawei.com> > mm/truncate.c | 28 +++++----------------------- > 1 file changed, 5 insertions(+), 23 deletions(-) > > diff --git a/mm/truncate.c b/mm/truncate.c > index 9dbf0b75da5d..e5e2edaa0b76 100644 > --- a/mm/truncate.c > +++ b/mm/truncate.c > @@ -193,27 +193,6 @@ static void truncate_cleanup_folio(struct folio *folio) > folio_clear_mappedtodisk(folio); > } > > -/* > - * This is for invalidate_mapping_pages(). That function can be called at > - * any time, and is not supposed to throw away dirty pages. But pages can > - * be marked dirty at any time too, so use remove_mapping which safely > - * discards clean, unused pages. > - * > - * Returns non-zero if the page was successfully invalidated. > - */ > -static int > -invalidate_complete_page(struct address_space *mapping, struct page *page) > -{ > - > - if (page->mapping != mapping) > - return 0; > - > - if (page_has_private(page) && !try_to_release_page(page, 0)) > - return 0; > - > - return remove_mapping(mapping, page); > -} > - > int truncate_inode_folio(struct address_space *mapping, struct folio *folio) > { > if (folio->mapping != mapping) > @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) > return 0; > if (page_mapped(page)) > return 0; > - return invalidate_complete_page(mapping, page); > + if (page_has_private(page) && !try_to_release_page(page, 0)) > + return 0; > + > + return remove_mapping(mapping, page); > } > > /** > @@ -584,7 +566,7 @@ void invalidate_mapping_pagevec(struct address_space *mapping, > } > > /* > - * This is like invalidate_complete_page(), except it ignores the page's > + * This is like invalidate_inode_page(), except it ignores the page's > * refcount. We do this because invalidate_inode_pages2() needs stronger > * invalidation guarantees, and cannot afford to leave pages behind because > * shrink_page_list() has a temp ref on them, or because they're transiently >
diff --git a/mm/truncate.c b/mm/truncate.c index 9dbf0b75da5d..e5e2edaa0b76 100644 --- a/mm/truncate.c +++ b/mm/truncate.c @@ -193,27 +193,6 @@ static void truncate_cleanup_folio(struct folio *folio) folio_clear_mappedtodisk(folio); } -/* - * This is for invalidate_mapping_pages(). That function can be called at - * any time, and is not supposed to throw away dirty pages. But pages can - * be marked dirty at any time too, so use remove_mapping which safely - * discards clean, unused pages. - * - * Returns non-zero if the page was successfully invalidated. - */ -static int -invalidate_complete_page(struct address_space *mapping, struct page *page) -{ - - if (page->mapping != mapping) - return 0; - - if (page_has_private(page) && !try_to_release_page(page, 0)) - return 0; - - return remove_mapping(mapping, page); -} - int truncate_inode_folio(struct address_space *mapping, struct folio *folio) { if (folio->mapping != mapping) @@ -309,7 +288,10 @@ int invalidate_inode_page(struct page *page) return 0; if (page_mapped(page)) return 0; - return invalidate_complete_page(mapping, page); + if (page_has_private(page) && !try_to_release_page(page, 0)) + return 0; + + return remove_mapping(mapping, page); } /** @@ -584,7 +566,7 @@ void invalidate_mapping_pagevec(struct address_space *mapping, } /* - * This is like invalidate_complete_page(), except it ignores the page's + * This is like invalidate_inode_page(), except it ignores the page's * refcount. We do this because invalidate_inode_pages2() needs stronger * invalidation guarantees, and cannot afford to leave pages behind because * shrink_page_list() has a temp ref on them, or because they're transiently
invalidate_inode_page() is the only caller of invalidate_complete_page() and inlining it reveals that the first check is unnecessary (because we hold the page locked, and we just retrieved the mapping from the page). Actually, it does make a difference, in that tail pages no longer fail at this check, so it's now possible to remove a tail page from a mapping. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/truncate.c | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-)