| Message ID | 20201014030357.21898-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Transparent Huge Page support for XFS | expand |
On Wed, Oct 14, 2020 at 04:03:44AM +0100, Matthew Wilcox (Oracle) wrote: > We may get tail pages returned from vfs_dedupe_get_page(). If we do, > we have to call page_mapping() instead of dereferencing page->mapping > directly. We may also deadlock trying to lock the page twice if they're > subpages of the same THP, so compare the head pages instead. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/read_write.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/read_write.c b/fs/read_write.c > index 19f5c4bf75aa..ead675fef582 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -1604,6 +1604,8 @@ static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset) > */ > static void vfs_lock_two_pages(struct page *page1, struct page *page2) > { > + page1 = thp_head(page1); > + page2 = thp_head(page2); Hmm, is this usage (calling thp_head() to extract the head page from an arbitrary page reference) a common enough idiom that it doesn't need a comment saying why we need the head page? I'm asking that genuinely-- thp_head() is new to me but maybe it's super obvious to everyone else? Or at least the mm developers? I suspect that might be the case....? Aside from that question, this looks fine to me. Also, I was sort of thinking about sending a patch to Linus at the end of the merge window moving all the remap/clone/dedupe common code to a separate file to declutter fs/read_write.c and mm/filemap.c. Does that sound ok? --D > /* Always lock in order of increasing index. */ > if (page1->index > page2->index) > swap(page1, page2); > @@ -1616,6 +1618,8 @@ static void vfs_lock_two_pages(struct page *page1, struct page *page2) > /* Unlock two pages, being careful not to unlock the same page twice. */ > static void vfs_unlock_two_pages(struct page *page1, struct page *page2) > { > + page1 = thp_head(page1); > + page2 = thp_head(page2); > unlock_page(page1); > if (page1 != page2) > unlock_page(page2); > @@ -1670,8 +1674,8 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff, > * someone is invalidating pages on us and we lose. > */ > if (!PageUptodate(src_page) || !PageUptodate(dest_page) || > - src_page->mapping != src->i_mapping || > - dest_page->mapping != dest->i_mapping) { > + page_mapping(src_page) != src->i_mapping || > + page_mapping(dest_page) != dest->i_mapping) { > same = false; > goto unlock; > } > -- > 2.28.0 >
On Wed, Oct 14, 2020 at 09:12:16AM -0700, Darrick J. Wong wrote: > On Wed, Oct 14, 2020 at 04:03:44AM +0100, Matthew Wilcox (Oracle) wrote: > > We may get tail pages returned from vfs_dedupe_get_page(). If we do, > > we have to call page_mapping() instead of dereferencing page->mapping > > directly. We may also deadlock trying to lock the page twice if they're > > subpages of the same THP, so compare the head pages instead. > > static void vfs_lock_two_pages(struct page *page1, struct page *page2) > > { > > + page1 = thp_head(page1); > > + page2 = thp_head(page2); > > Hmm, is this usage (calling thp_head() to extract the head page from an > arbitrary page reference) a common enough idiom that it doesn't need a > comment saying why we need the head page? It's pretty common. Lots of times it gets hidden inside macros, and sometimes it gets spelled as 'compound_head' instead of thp_head. The advantage of thp_head() is that it compiles away if CONFIG_TRANSPARENT_HUGEPAGE is disabled, while compound pages always exist. > I'm asking that genuinely-- thp_head() is new to me but maybe it's super > obvious to everyone else? Or at least the mm developers? I suspect > that might be the case....? thp_head is indeed new. It was merged in August this year, partly in response to Dave Chinner getting annoyed at the mixing of metaphors -- some things were thp_*, some were hpage_* and some were compound_*. Now everything is in the thp_* namespace if it refers to THPs. > Also, I was sort of thinking about sending a patch to Linus at the end > of the merge window moving all the remap/clone/dedupe common code to a > separate file to declutter fs/read_write.c and mm/filemap.c. Does that > sound ok? I don't think that would bother me at all.
diff --git a/fs/read_write.c b/fs/read_write.c index 19f5c4bf75aa..ead675fef582 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1604,6 +1604,8 @@ static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset) */ static void vfs_lock_two_pages(struct page *page1, struct page *page2) { + page1 = thp_head(page1); + page2 = thp_head(page2); /* Always lock in order of increasing index. */ if (page1->index > page2->index) swap(page1, page2); @@ -1616,6 +1618,8 @@ static void vfs_lock_two_pages(struct page *page1, struct page *page2) /* Unlock two pages, being careful not to unlock the same page twice. */ static void vfs_unlock_two_pages(struct page *page1, struct page *page2) { + page1 = thp_head(page1); + page2 = thp_head(page2); unlock_page(page1); if (page1 != page2) unlock_page(page2); @@ -1670,8 +1674,8 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff, * someone is invalidating pages on us and we lose. */ if (!PageUptodate(src_page) || !PageUptodate(dest_page) || - src_page->mapping != src->i_mapping || - dest_page->mapping != dest->i_mapping) { + page_mapping(src_page) != src->i_mapping || + page_mapping(dest_page) != dest->i_mapping) { same = false; goto unlock; }
We may get tail pages returned from vfs_dedupe_get_page(). If we do, we have to call page_mapping() instead of dereferencing page->mapping directly. We may also deadlock trying to lock the page twice if they're subpages of the same THP, so compare the head pages instead. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/read_write.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)