| Message ID | 20200914130042.11442-12-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Overhaul multi-page lookups for THP | expand |
On Mon 14-09-20 14:00:41, Matthew Wilcox (Oracle) wrote: > Handle THP splitting in the parts of the truncation functions which > already handle partial pages. Factor all that code out into a new > function called truncate_inode_partial_page(). > > We lose the easy 'bail out' path if a truncate or hole punch is entirely > within a single page. We can add some more complex logic to restore > the optimisation if it proves to be worthwhile. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Overall this looks OK. Some smaller suggestions below... > @@ -931,33 +904,39 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, > index++; > } > > - if (partial_start) { > - struct page *page = NULL; > - shmem_getpage(inode, start - 1, &page, SGP_READ); > - if (page) { > - unsigned int top = PAGE_SIZE; > - if (start > end) { > - top = partial_end; > - partial_end = 0; > - } > - zero_user_segment(page, partial_start, top); > - set_page_dirty(page); > - unlock_page(page); > - put_page(page); > + index = -1; > + if (end != -1 && ((lend + 1) % PAGE_SIZE)) ^^ Hum, is this guaranteed to compile properly on 32-bit archs without optimization? It would be 64-bit division... Maybe we don't care, it just caught my eye... BTW you could just drop end != -1 part because end == -1 iff lend == -1 so (lend + 1) % PAGE_SIZE is stronger. > + index = lend >> PAGE_SHIFT; > + page = NULL; > + shmem_getpage(inode, lstart >> PAGE_SHIFT, &page, SGP_READ); > + if (page) { > + bool same_page; > + > + page = thp_head(page); > + same_page = lend + 1 < page_offset(page) + thp_size(page); ^^^^^^^^ Just lend here? > + if (same_page) > + index = -1; > + set_page_dirty(page); > + if (!truncate_inode_partial_page(page, lstart, lend)) { > + start = page->index + thp_nr_pages(page); > + if (same_page) > + end = page->index; > } > + unlock_page(page); > + put_page(page); > + page = NULL; > } > - if (partial_end) { > - struct page *page = NULL; > + > + if (index != -1) > shmem_getpage(inode, end, &page, SGP_READ); > - if (page) { > - zero_user_segment(page, 0, partial_end); > - set_page_dirty(page); > - unlock_page(page); > - put_page(page); > - } > + if (page) { > + page = thp_head(page); > + set_page_dirty(page); > + if (!truncate_inode_partial_page(page, lstart, lend)) > + end = page->index; > + unlock_page(page); > + put_page(page); > } > - if (start >= end) > - return; You use 'index' effectively as bool in all of the above (only ever check index != -1). And effectively you only use it to communicate whether tail partial page got already handled or not. Maybe there's some less cryptic way to achieve that? Even separate bool just for that would be probably better that this. > index = start; > while (index < end) { > diff --git a/mm/truncate.c b/mm/truncate.c > index d62aeffbffcc..06ed2f93069d 100644 > --- a/mm/truncate.c > +++ b/mm/truncate.c > @@ -224,6 +224,53 @@ int truncate_inode_page(struct address_space *mapping, struct page *page) > return 0; > } > > +/* > + * Handle partial (transparent) pages. The page may be entirely within the > + * range if a split has raced with us. If not, we zero the part of the > + * page that's within the (start, end] range, and then split the page if ^ '[' here - start is inclusive as well... > + * it's a THP. split_page_range() will discard pages which now lie beyond > + * i_size, and we rely on the caller to discard pages which lie within a > + * newly created hole. > + * > + * Returns false if THP splitting failed so the caller can can avoid ^^^^^^^ just 'can' > + * discarding the entire page which is stubbornly unsplit. > + */ > +bool truncate_inode_partial_page(struct page *page, loff_t start, loff_t end) > +{ > + loff_t pos = page_offset(page); > + unsigned int offset, length; > + > + if (pos < start) > + offset = start - pos; > + else > + offset = 0; > + length = thp_size(page); > + if (pos + length <= (u64)end) > + length = length - offset; > + else > + length = end + 1 - pos - offset; > + > + wait_on_page_writeback(page); > + if (length == thp_size(page)) { > + truncate_inode_page(page->mapping, page); > + return true; > + } > + > + /* > + * We may be zeroing pages we're about to discard, but it avoids > + * doing a complex calculation here, and then doing the zeroing > + * anyway if the page split fails. > + */ > + zero_user(page, offset, length); > + > + cleancache_invalidate_page(page->mapping, page); > + if (page_has_private(page)) > + do_invalidatepage(page, offset, length); > + if (!PageTransHuge(page)) > + return true; > + return split_huge_page(page) == 0; > +} > + > /* > * Used to get rid of pages on hardware memory corruption. > */ > @@ -288,20 +335,15 @@ void truncate_inode_pages_range(struct address_space *mapping, > { > pgoff_t start; /* inclusive */ > pgoff_t end; /* exclusive */ > - unsigned int partial_start; /* inclusive */ > - unsigned int partial_end; /* exclusive */ > struct pagevec pvec; > pgoff_t indices[PAGEVEC_SIZE]; > pgoff_t index; > int i; > + struct page * page; > > if (mapping->nrpages == 0 && mapping->nrexceptional == 0) > goto out; > > - /* Offsets within partial pages */ > - partial_start = lstart & (PAGE_SIZE - 1); > - partial_end = (lend + 1) & (PAGE_SIZE - 1); > - > /* > * 'start' and 'end' always covers the range of pages to be fully > * truncated. Partial pages are covered with 'partial_start' at the > @@ -334,48 +376,37 @@ void truncate_inode_pages_range(struct address_space *mapping, > cond_resched(); > } > > - if (partial_start) { > - struct page *page = find_lock_page(mapping, start - 1); > - if (page) { > - unsigned int top = PAGE_SIZE; > - if (start > end) { > - /* Truncation within a single page */ > - top = partial_end; > - partial_end = 0; > - } > - wait_on_page_writeback(page); > - zero_user_segment(page, partial_start, top); > - cleancache_invalidate_page(mapping, page); > - if (page_has_private(page)) > - do_invalidatepage(page, partial_start, > - top - partial_start); > - unlock_page(page); > - put_page(page); > + index = -1; > + if (end != -1 && ((lend + 1) % PAGE_SIZE)) ^^^^^^^^^ Again this is unnecessary... > + index = lend >> PAGE_SHIFT; > + page = find_lock_head(mapping, lstart >> PAGE_SHIFT); > + if (page) { > + bool same_page = lend + 1 < page_offset(page) + thp_size(page); ^^^^^^^ Again why +1 here? > + if (same_page) > + index = -1; > + if (!truncate_inode_partial_page(page, lstart, lend)) { > + start = page->index + thp_nr_pages(page); > + if (same_page) > + end = page->index; > } > + unlock_page(page); > + put_page(page); > + page = NULL; > } > - if (partial_end) { > - struct page *page = find_lock_page(mapping, end); > - if (page) { > - wait_on_page_writeback(page); > - zero_user_segment(page, 0, partial_end); > - cleancache_invalidate_page(mapping, page); > - if (page_has_private(page)) > - do_invalidatepage(page, 0, > - partial_end); > - unlock_page(page); > - put_page(page); > - } > + > + if (index != -1) > + page = find_lock_head(mapping, index); Similarly to shmem the use of index is a bit confusing here but it at least gets used in this case so OK. But I'd still find something like: if (!tail_page_already_truncated) page = find_lock_head(mapping, lend >> PAGE_SHIFT); easier to grasp. > + if (page) { > + if (!truncate_inode_partial_page(page, lstart, lend)) > + end = page->index; > + unlock_page(page); > + put_page(page); > } Honza
On Wed, Sep 30, 2020 at 01:59:19PM +0200, Jan Kara wrote: > > @@ -931,33 +904,39 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, > > index++; > > } > > > > - if (partial_start) { > > - struct page *page = NULL; > > - shmem_getpage(inode, start - 1, &page, SGP_READ); > > - if (page) { > > - unsigned int top = PAGE_SIZE; > > - if (start > end) { > > - top = partial_end; > > - partial_end = 0; > > - } > > - zero_user_segment(page, partial_start, top); > > - set_page_dirty(page); > > - unlock_page(page); > > - put_page(page); > > + index = -1; > > + if (end != -1 && ((lend + 1) % PAGE_SIZE)) > ^^ > Hum, is this guaranteed to compile properly on 32-bit archs without > optimization? It would be 64-bit division... Maybe we don't care, it just > caught my eye... Looks like GCC properly reduces it: 00000000 <f>: int f(long long x) { if ((x + 1) % 4096) 0: 8b 44 24 04 mov 0x4(%esp),%eax 4: 83 c0 01 add $0x1,%eax 7: 25 ff 0f 00 00 and $0xfff,%eax c: 0f 95 c0 setne %al f: 0f b6 c0 movzbl %al,%eax return 1; return 0; } 12: c3 ret > BTW you could just drop end != -1 part because end == -1 iff lend == -1 so > (lend + 1) % PAGE_SIZE is stronger. Yes. I think that actually makes it easier to read. > > + index = lend >> PAGE_SHIFT; > > + page = NULL; > > + shmem_getpage(inode, lstart >> PAGE_SHIFT, &page, SGP_READ); > > + if (page) { > > + bool same_page; > > + > > + page = thp_head(page); > > + same_page = lend + 1 < page_offset(page) + thp_size(page); > ^^^^^^^^ Just lend here? Oops. Yes. If lstart is 4096 and lend is 8191, this is definitely same_page. > > - if (partial_end) { > > - struct page *page = NULL; > > + > > + if (index != -1) > > shmem_getpage(inode, end, &page, SGP_READ); > > - if (page) { > > - zero_user_segment(page, 0, partial_end); > > - set_page_dirty(page); > > - unlock_page(page); > > - put_page(page); > > - } > > + if (page) { > > + page = thp_head(page); > > + set_page_dirty(page); > > + if (!truncate_inode_partial_page(page, lstart, lend)) > > + end = page->index; > > + unlock_page(page); > > + put_page(page); > > } > > - if (start >= end) > > - return; > > You use 'index' effectively as bool in all of the above (only ever check > index != -1). And effectively you only use it to communicate whether tail > partial page got already handled or not. Maybe there's some less cryptic > way to achieve that? Even separate bool just for that would be probably > better that this. As you note below, it makes more sense in truncate.c and for my own sanity I was trying to keep the two functions as similar as possible. I'm not sure why I looked up the page with 'end' instead of 'index'. This didn't end up being as big a simplification as I thought it was going to be. I think I'll reintroduce partial_end as a bool, like you suggest. > > index = start; > > while (index < end) { > > > diff --git a/mm/truncate.c b/mm/truncate.c > > index d62aeffbffcc..06ed2f93069d 100644 > > --- a/mm/truncate.c > > +++ b/mm/truncate.c > > @@ -224,6 +224,53 @@ int truncate_inode_page(struct address_space *mapping, struct page *page) > > return 0; > > } > > > > +/* > > + * Handle partial (transparent) pages. The page may be entirely within the > > + * range if a split has raced with us. If not, we zero the part of the > > + * page that's within the (start, end] range, and then split the page if > ^ '[' here - start is inclusive as well... Good point. > > + * it's a THP. split_page_range() will discard pages which now lie beyond > > + * i_size, and we rely on the caller to discard pages which lie within a > > + * newly created hole. > > + * > > + * Returns false if THP splitting failed so the caller can can avoid > ^^^^^^^ just 'can' You're going to put Randy out of a job ;-)
diff --git a/mm/internal.h b/mm/internal.h index 1c5f4425ed74..d17d9060902f 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -620,4 +620,5 @@ struct migration_target_control { gfp_t gfp_mask; }; +bool truncate_inode_partial_page(struct page *page, loff_t start, loff_t end); #endif /* __MM_INTERNAL_H */ diff --git a/mm/shmem.c b/mm/shmem.c index b2b94025841a..23bb79ec070f 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -855,32 +855,6 @@ void shmem_unlock_mapping(struct address_space *mapping) } } -/* - * Check whether a hole-punch or truncation needs to split a huge page, - * returning true if no split was required, or the split has been successful. - * - * Eviction (or truncation to 0 size) should never need to split a huge page; - * but in rare cases might do so, if shmem_undo_range() failed to trylock on - * head, and then succeeded to trylock on tail. - * - * A split can only succeed when there are no additional references on the - * huge page: so the split below relies upon find_get_entries() having stopped - * when it found a subpage of the huge page, without getting further references. - */ -static bool shmem_punch_compound(struct page *page, pgoff_t start, pgoff_t end) -{ - if (!PageTransCompound(page)) - return true; - - /* Just proceed to delete a huge page wholly within the range punched */ - if (PageHead(page) && - page->index >= start && page->index + HPAGE_PMD_NR <= end) - return true; - - /* Try to split huge page, so we can truly punch the hole or truncate */ - return split_huge_page(page) >= 0; -} - /* * Remove range of pages and swap entries from page cache, and free them. * If !unfalloc, truncate or punch hole; if unfalloc, undo failed fallocate. @@ -892,10 +866,9 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, struct shmem_inode_info *info = SHMEM_I(inode); pgoff_t start = (lstart + PAGE_SIZE - 1) >> PAGE_SHIFT; pgoff_t end = (lend + 1) >> PAGE_SHIFT; - unsigned int partial_start = lstart & (PAGE_SIZE - 1); - unsigned int partial_end = (lend + 1) & (PAGE_SIZE - 1); struct pagevec pvec; pgoff_t indices[PAGEVEC_SIZE]; + struct page *page; long nr_swaps_freed = 0; pgoff_t index; int i; @@ -908,7 +881,7 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, while (index < end && find_lock_entries(mapping, index, end - 1, &pvec, indices)) { for (i = 0; i < pagevec_count(&pvec); i++) { - struct page *page = pvec.pages[i]; + page = pvec.pages[i]; index = indices[i]; @@ -931,33 +904,39 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, index++; } - if (partial_start) { - struct page *page = NULL; - shmem_getpage(inode, start - 1, &page, SGP_READ); - if (page) { - unsigned int top = PAGE_SIZE; - if (start > end) { - top = partial_end; - partial_end = 0; - } - zero_user_segment(page, partial_start, top); - set_page_dirty(page); - unlock_page(page); - put_page(page); + index = -1; + if (end != -1 && ((lend + 1) % PAGE_SIZE)) + index = lend >> PAGE_SHIFT; + page = NULL; + shmem_getpage(inode, lstart >> PAGE_SHIFT, &page, SGP_READ); + if (page) { + bool same_page; + + page = thp_head(page); + same_page = lend + 1 < page_offset(page) + thp_size(page); + if (same_page) + index = -1; + set_page_dirty(page); + if (!truncate_inode_partial_page(page, lstart, lend)) { + start = page->index + thp_nr_pages(page); + if (same_page) + end = page->index; } + unlock_page(page); + put_page(page); + page = NULL; } - if (partial_end) { - struct page *page = NULL; + + if (index != -1) shmem_getpage(inode, end, &page, SGP_READ); - if (page) { - zero_user_segment(page, 0, partial_end); - set_page_dirty(page); - unlock_page(page); - put_page(page); - } + if (page) { + page = thp_head(page); + set_page_dirty(page); + if (!truncate_inode_partial_page(page, lstart, lend)) + end = page->index; + unlock_page(page); + put_page(page); } - if (start >= end) - return; index = start; while (index < end) { @@ -973,7 +952,7 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, continue; } for (i = 0; i < pagevec_count(&pvec); i++) { - struct page *page = pvec.pages[i]; + page = pvec.pages[i]; index = indices[i]; if (xa_is_value(page)) { @@ -998,18 +977,9 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend, break; } VM_BUG_ON_PAGE(PageWriteback(page), page); - if (shmem_punch_compound(page, start, end)) - truncate_inode_page(mapping, page); - else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) { - /* Wipe the page and don't get stuck */ - clear_highpage(page); - flush_dcache_page(page); - set_page_dirty(page); - if (index < - round_up(start, HPAGE_PMD_NR)) - start = index + 1; - } + truncate_inode_page(mapping, page); } + index = page->index + thp_nr_pages(page) - 1; unlock_page(page); } pagevec_remove_exceptionals(&pvec); diff --git a/mm/truncate.c b/mm/truncate.c index d62aeffbffcc..06ed2f93069d 100644 --- a/mm/truncate.c +++ b/mm/truncate.c @@ -224,6 +224,53 @@ int truncate_inode_page(struct address_space *mapping, struct page *page) return 0; } +/* + * Handle partial (transparent) pages. The page may be entirely within the + * range if a split has raced with us. If not, we zero the part of the + * page that's within the (start, end] range, and then split the page if + * it's a THP. split_page_range() will discard pages which now lie beyond + * i_size, and we rely on the caller to discard pages which lie within a + * newly created hole. + * + * Returns false if THP splitting failed so the caller can can avoid + * discarding the entire page which is stubbornly unsplit. + */ +bool truncate_inode_partial_page(struct page *page, loff_t start, loff_t end) +{ + loff_t pos = page_offset(page); + unsigned int offset, length; + + if (pos < start) + offset = start - pos; + else + offset = 0; + length = thp_size(page); + if (pos + length <= (u64)end) + length = length - offset; + else + length = end + 1 - pos - offset; + + wait_on_page_writeback(page); + if (length == thp_size(page)) { + truncate_inode_page(page->mapping, page); + return true; + } + + /* + * We may be zeroing pages we're about to discard, but it avoids + * doing a complex calculation here, and then doing the zeroing + * anyway if the page split fails. + */ + zero_user(page, offset, length); + + cleancache_invalidate_page(page->mapping, page); + if (page_has_private(page)) + do_invalidatepage(page, offset, length); + if (!PageTransHuge(page)) + return true; + return split_huge_page(page) == 0; +} + /* * Used to get rid of pages on hardware memory corruption. */ @@ -288,20 +335,15 @@ void truncate_inode_pages_range(struct address_space *mapping, { pgoff_t start; /* inclusive */ pgoff_t end; /* exclusive */ - unsigned int partial_start; /* inclusive */ - unsigned int partial_end; /* exclusive */ struct pagevec pvec; pgoff_t indices[PAGEVEC_SIZE]; pgoff_t index; int i; + struct page * page; if (mapping->nrpages == 0 && mapping->nrexceptional == 0) goto out; - /* Offsets within partial pages */ - partial_start = lstart & (PAGE_SIZE - 1); - partial_end = (lend + 1) & (PAGE_SIZE - 1); - /* * 'start' and 'end' always covers the range of pages to be fully * truncated. Partial pages are covered with 'partial_start' at the @@ -334,48 +376,37 @@ void truncate_inode_pages_range(struct address_space *mapping, cond_resched(); } - if (partial_start) { - struct page *page = find_lock_page(mapping, start - 1); - if (page) { - unsigned int top = PAGE_SIZE; - if (start > end) { - /* Truncation within a single page */ - top = partial_end; - partial_end = 0; - } - wait_on_page_writeback(page); - zero_user_segment(page, partial_start, top); - cleancache_invalidate_page(mapping, page); - if (page_has_private(page)) - do_invalidatepage(page, partial_start, - top - partial_start); - unlock_page(page); - put_page(page); + index = -1; + if (end != -1 && ((lend + 1) % PAGE_SIZE)) + index = lend >> PAGE_SHIFT; + page = find_lock_head(mapping, lstart >> PAGE_SHIFT); + if (page) { + bool same_page = lend + 1 < page_offset(page) + thp_size(page); + if (same_page) + index = -1; + if (!truncate_inode_partial_page(page, lstart, lend)) { + start = page->index + thp_nr_pages(page); + if (same_page) + end = page->index; } + unlock_page(page); + put_page(page); + page = NULL; } - if (partial_end) { - struct page *page = find_lock_page(mapping, end); - if (page) { - wait_on_page_writeback(page); - zero_user_segment(page, 0, partial_end); - cleancache_invalidate_page(mapping, page); - if (page_has_private(page)) - do_invalidatepage(page, 0, - partial_end); - unlock_page(page); - put_page(page); - } + + if (index != -1) + page = find_lock_head(mapping, index); + if (page) { + if (!truncate_inode_partial_page(page, lstart, lend)) + end = page->index; + unlock_page(page); + put_page(page); } - /* - * If the truncation happened within a single page no pages - * will be released, just zeroed, so we can bail out now. - */ - if (start >= end) - goto out; index = start; - for ( ; ; ) { + while (index < end) { cond_resched(); + if (!find_get_entries(mapping, index, end - 1, &pvec, indices)) { /* If all gone from start onwards, we're done */ @@ -387,7 +418,7 @@ void truncate_inode_pages_range(struct address_space *mapping, } for (i = 0; i < pagevec_count(&pvec); i++) { - struct page *page = pvec.pages[i]; + page = pvec.pages[i]; /* We rely upon deletion not changing page->index */ index = indices[i]; @@ -396,7 +427,7 @@ void truncate_inode_pages_range(struct address_space *mapping, continue; lock_page(page); - WARN_ON(page_to_index(page) != index); + index = page->index + thp_nr_pages(page) - 1; wait_on_page_writeback(page); truncate_inode_page(mapping, page); unlock_page(page);
Handle THP splitting in the parts of the truncation functions which already handle partial pages. Factor all that code out into a new function called truncate_inode_partial_page(). We lose the easy 'bail out' path if a truncate or hole punch is entirely within a single page. We can add some more complex logic to restore the optimisation if it proves to be worthwhile. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/internal.h | 1 + mm/shmem.c | 98 +++++++++++++++-------------------------- mm/truncate.c | 119 +++++++++++++++++++++++++++++++------------------- 3 files changed, 110 insertions(+), 108 deletions(-)