On Tue, 5 Oct 2021, Rongwei Wang wrote:Hi, Hugh
Hi,
I have run our cases these two days to stress test new Patch #1. The new Patch
#1 mainly add filemap_invalidate_{un}lock before and after
truncate_pagecache(), basing on original Patch #1. And the crash has not
happened.
Now, I keep the original Patch #1, then adding the code below which suggested
by liu song (I'm not sure which one I should add in the next version,
Suggested-by or Signed-off-by? If you know, please remind me).
- if (filemap_nr_thps(inode->i_mapping))
+ if (filemap_nr_thps(inode->i_mapping)) {
+ filemap_invalidate_lock(inode->i_mapping);
truncate_pagecache(inode, 0);
+ filemap_invalidate_unlock(inode->i_mapping);
+ }
I won't NAK that patch; but I still believe it's unnecessary, and don't
see how it protects against all the races (collapse_file() does not use
that lock, whereas collapse_file() does use page lock). And if you're
hoping to fix 5.10, then you will have to backport those invalidate_lock
patches there too (they're really intended to protect hole-punching).
And the reason for keeping the original Patch #1 is mainly to fix the race
between collapse_file and truncate_pagecache. It seems necessary. Despite the
two-day test, I did not reproduce this race any more.
In addition, I also test the below method:
diff --git a/mm/truncate.c b/mm/truncate.c
index 3f47190f98a8..33604e4ce60a 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -210,8 +210,6 @@ invalidate_complete_page(struct address_space *mapping,
struct page *page)
int truncate_inode_page(struct address_space *mapping, struct page *page)
{
- VM_BUG_ON_PAGE(PageTail(page), page);
-
if (page->mapping != mapping)
return -EIO;
I am not very sure this VM_BUG_ON_PAGE(PageTail) is what Hugh means. And
the test results show that only removing this VM_BUG_ON_PAGE(PageTail) has no
effect. So, I still keep the original Patch #1 to fix one race.
Yes, that's exactly what I meant, and thank you for intending to try it.
But if that patch had "no effect", then I think you were not running the
kernel with that patch applied: because it deletes the BUG on line 213
of mm/truncate.c, which is what you reported in the first mail!
Or, is line 213 of mm/truncate.c in your 5.10.46-hugetext+ kernel
something else? I've been looking at 5.15-rc.
Today, I try again to create some cases to reproduce the race, such as ensuring that multiple processes are always executing ‘truncate_pagecache’ and only mapping 2M DSO. In this way, I try to ensure that the target of 'collapse_file' and 'truncate_pagecache' can only be the same VMA, to increase the probability of reproducing that race. But, I can't reproduce that race any more.
But I wasn't proposing to delete it merely to hide the BUG: as I hope
I explained, we could move it below the page->mapping check, but it
wouldn't really be of any value there since tails have NULL page->mapping
anyway (well, I didn't check first and second tails, maybe mapping gets
reused for some compound page field in those). I was proposing to delete
it because the page->mapping check then weeds out the racy case once
we're holding page lock, without the need for adding anything special.
Hugh