Re: [PATCH v3 00/13] btrfs: support read-write for subpage metadata

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: riteshh <riteshh@linux.ibm.com>
Cc: Qu Wenruo <wqu@suse.com>, Ritesh Harjani <ritesh.list@gmail.com>,
	Neal Gompa <ngompa13@gmail.com>,
	Btrfs BTRFS <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH v3 00/13] btrfs: support read-write for subpage metadata
Date: Wed, 21 Apr 2021 15:15:17 +0800	[thread overview]
Message-ID: <b036930e-1596-3db4-fd14-2b329977d64e@gmx.com> (raw)
In-Reply-To: <20210421070331.r4enns6ticwpan35@riteshh-domain>

On 2021/4/21 下午3:03, riteshh wrote:
> On 21/04/19 09:24PM, Qu Wenruo wrote:
>> [...]
>>>>
>>>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>>>> index 45ec3f5ef839..49f78d643392 100644
>>>> --- a/fs/btrfs/file.c
>>>> +++ b/fs/btrfs/file.c
>>>> @@ -1341,7 +1341,17 @@ static int prepare_uptodate_page(struct inode
>>>> *inode,
>>>>                          unlock_page(page);
>>>>                          return -EIO;
>>>>                  }
>>>> -               if (page->mapping != inode->i_mapping) {
>>>> +
>>>> +               /*
>>>> +                * Since btrfs_readpage() will get the page unlocked, we
>>>> have
>>>> +                * a window where fadvice() can try to release the page.
>>>> +                * Here we check both inode mapping and PagePrivate() to
>>>> +                * make sure the page is not released.
>>>> +                *
>>>> +                * The priavte flag check is essential for subpage as we
>>>> need
>>>> +                * to store extra bitmap using page->private.
>>>> +                */
>>>> +               if (page->mapping != inode->i_mapping ||
>>>> PagePrivate(page)) {
>>>    ^ Obviously it should be !PagePrivate(page).
>>
>> Hi Ritesh,
>>
>> Mind to have another try on generic/095?
>>
>> This time the branch is updated with the following commit at top:
>>
>> commit d700b16dced6f2e2b47e1ca5588a92216ce84dfb (HEAD -> subpage,
>> github/subpage)
>> Author: Qu Wenruo <wqu@suse.com>
>> Date:   Mon Apr 19 13:41:31 2021 +0800
>>
>>      btrfs: fix a crash caused by race between prepare_pages() and
>>      btrfs_releasepage()
>>
>> The fix uses the PagePrivate() check to avoid the problem, and passes
>> several generic/auto loops without any sign of crash.
>>
>> But considering I always have difficult in reproducing the bug with previous
>> improper fix, your verification would be very helpful.
>>
>
> Hi Qu,
>
> Thanks for the patch. I did try above patch but even with this I could still
> reproduce the issue.
>
> 1. I think the original problem could be due to below logs.
> 	[   79.079641] run fstests generic/095 at 2021-04-21 06:46:23
> 	<...>
> 	[   83.634710] Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!
>
> Meaning, there might be a race here between DIO and buffered IO.
> So from DIO path we call invalidate_inode_pages2_range(). Somehow this maybe
> causing call of btrfs_releasepage().
>
> Now from code, invalidate_inode_pages2_range() can be called from both
> __iomap_dio_rw() and from iomap_dio_complete(). So it is not clear as to from
> where this might be triggering this bug. >
> I will try and debug more. But I thought I will update you with above findings.

Your finding and testing are really helpful.

BTW, Goldwyn helped me to test the same patchset on power too, but
unfortunately he didn't reproduce the bug either on generic/095.

So I'm afraid the bug is way more complex than I thought.

BTW, have you tried to enable KASAN and to see if KASAN can find the
problem?

Thanks,
Qu
>
> -ritesh
>