Re: [PATCH v2 1/2] mm, thp: check page mapping when truncating page cache

[Rongwei Wang <rongwei.wang@linux.alibaba.com>]

On 10/5/21 10:58 AM, Hugh Dickins wrote:
> On Tue, 5 Oct 2021, Rongwei Wang wrote:
> 
>> 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.
Hi, Hugh

I'm sorry the confusing '5.10.46-hugetext+'. I am also look and test at 
5.15-rc.
> 
> 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
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.

In fact, according to the previous experience, the current number of 
attempts should be able to reproduce that race.

If you have the idea about creating this case, please tell me, and I can 
try again. Or we can solve it when it appears again.

Thanks!
>