| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 22 Mar 2023 18:24:31 +0800
From: Shiyang Ruan <ruansy.fnst@...itsu.com>
To: "Darrick J. Wong" <djwong@...nel.org>
CC: <linux-xfs@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
<nvdimm@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
<david@...morbit.com>, <dan.j.williams@...el.com>,
<akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH] xfs: check shared state of when CoW, update reflink
flag when io ends
在 2023/3/21 23:13, Darrick J. Wong 写道:
> On Mon, Mar 20, 2023 at 06:02:05PM +0800, Shiyang Ruan wrote:
>>
>>
>> 在 2023/3/18 4:35, Darrick J. Wong 写道:
>>> On Fri, Mar 17, 2023 at 03:59:48AM +0000, Shiyang Ruan wrote:
>>>> As is mentioned[1] before, the generic/388 will randomly fail with dmesg
>>>> warning. This case uses fsstress with a lot of random operations. It is hard
>>>> to reproduce. Finally I found a 100% reproduce condition, which is setting
>>>> the seed to 1677104360. So I changed the generic/388 code: removed the loop
>>>> and used the code below instad:
>>>> ```
>>>> ($FSSTRESS_PROG $FSSTRESS_AVOID -d $SCRATCH_MNT -v -s 1677104360 -n 221 -p 1 >> $seqres.full) > /dev/null 2>&1
>>>> ($FSSTRESS_PROG $FSSTRESS_AVOID -d $SCRATCH_MNT -v -s 1677104360 -n 221 -p 1 >> $seqres.full) > /dev/null 2>&1
>>>> _check_dmesg_for dax_insert_entry
>>>> ```
>>>>
>>>> According to the operations log, and kernel debug log I added, I found that
>>>> the reflink flag of one inode won't be unset even if there's no more shared
>>>> extents any more.
>>>> Then write to this file again. Because of the reflink flag, xfs thinks it
>>>> needs cow, and extent(called it extA) will be CoWed to a new
>>>> extent(called it extB) incorrectly. And extA is not used any more,
>>>> but didn't be unmapped (didn't do dax_disassociate_entry()).
>>>
>>> IOWs, dax_iomap_copy_around (or something very near it) should be
>>> calling dax_disassociate_entry on the source range after copying extA's
>>> contents to extB to drop its page->shared count?
>>
>> If extA is a shared extent, its pages will be disassociated correctly by
>> invalidate_inode_pages2_range() in dax_iomap_iter().
>>
>> But the problem is that extA is not shared but now be CoWed,
>
> Aha! Ok, I hadn't realized that extA is not shared...
>
>> invalidate_inode_pages2_range() is also called but it can't disassociate the
>> old page (because the page is marked dirty, can't be invalidated)
>
> ...so what marked the old page dirty? Was it the case that the
> unshared extA got marked dirty, then later someone created a cow
> reservation (extB, I guess) that covered the already dirty extA?
>
> Should we be transferring the dirty state from A to B here before the
> invalidate_inode_pages2_range ?
>
>> Is the behavior to do CoW on a non-shared extent allowed?
>
> In general, yes, XFS allows COW on non-shared extents. The (cow) extent
> size hint provides for cowing the unshared blocks adjacent to a shared
> block to try to combat fragmentation.
Ok, I did't realize its benifit. Thanks a lot.
Now I've fixed it based on your suggestion and it works. The failed
cases all passed. Now I'm running the generic/388 for many and many
times to make sure it doesn't fail again. I'll send the patch if the
generic/388 is passed.
>
>>>
>>>> The next time we mapwrite to another file, xfs will allocate extA for it,
>>>> page fault handler do dax_associate_entry(). BUT bucause the extA didn't
>>>> be unmapped, it still stores old file's info in page->mapping,->index.
>>>> Then, It reports dmesg warning when it try to sotre the new file's info.
>>>>
>>>> So, I think:
>>>> 1. reflink flag should be updated after CoW operations.
>>>> 2. xfs_reflink_allocate_cow() should add "if extent is shared" to determine
>>>> xfs do CoW or not.
>>>>
>>>> I made the fix patch, it can resolve the fail of generic/388. But it causes
>>>> other cases fail: generic/127, generic/263, generic/616, xfs/315 xfs/421. I'm
>>>> not sure if the fix is right, or I have missed something somewhere. Please
>>>> give me some advice.
>>>>
>>>> Thank you very much!!
>>>>
>>>> [1]: https://lore.kernel.org/linux-xfs/1669908538-55-1-git-send-email-ruansy.fnst@fujitsu.com/
>>>>
>>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@...itsu.com>
>>>> ---
>>>> fs/xfs/xfs_reflink.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>> fs/xfs/xfs_reflink.h | 2 ++
>>>> 2 files changed, 46 insertions(+)
>>>>
>>>> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
>>>> index f5dc46ce9803..a6b07f5c1db2 100644
>>>> --- a/fs/xfs/xfs_reflink.c
>>>> +++ b/fs/xfs/xfs_reflink.c
>>>> @@ -154,6 +154,40 @@ xfs_reflink_find_shared(
>>>> return error;
>>>> }
>>>> +int xfs_reflink_extent_is_shared(
>>>> + struct xfs_inode *ip,
>>>> + struct xfs_bmbt_irec *irec,
>>>> + bool *shared)
>>>> +{
>>>> + struct xfs_mount *mp = ip->i_mount;
>>>> + struct xfs_perag *pag;
>>>> + xfs_agblock_t agbno;
>>>> + xfs_extlen_t aglen;
>>>> + xfs_agblock_t fbno;
>>>> + xfs_extlen_t flen;
>>>> + int error = 0;
>>>> +
>>>> + *shared = false;
>>>> +
>>>> + /* Holes, unwritten, and delalloc extents cannot be shared */
>>>> + if (!xfs_bmap_is_written_extent(irec))
>>>> + return 0;
>>>> +
>>>> + pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, irec->br_startblock));
>>>> + agbno = XFS_FSB_TO_AGBNO(mp, irec->br_startblock);
>>>> + aglen = irec->br_blockcount;
>>>> + error = xfs_reflink_find_shared(pag, NULL, agbno, aglen, &fbno, &flen,
>>>> + true);
>>>> + xfs_perag_put(pag);
>>>> + if (error)
>>>> + return error;
>>>> +
>>>> + if (fbno != NULLAGBLOCK)
>>>> + *shared = true;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> /*
>>>> * Trim the mapping to the next block where there's a change in the
>>>> * shared/unshared status. More specifically, this means that we
>>>> @@ -533,6 +567,12 @@ xfs_reflink_allocate_cow(
>>>> xfs_ifork_init_cow(ip);
>>>> }
>>>> + error = xfs_reflink_extent_is_shared(ip, imap, shared);
>>>> + if (error)
>>>> + return error;
>>>> + if (!*shared)
>>>> + return 0;
>>>> +
>>>> error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
>>>> if (error || !*shared)
>>>> return error;
>>>> @@ -834,6 +874,10 @@ xfs_reflink_end_cow_extent(
>>>> /* Remove the mapping from the CoW fork. */
>>>> xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
>>>> + error = xfs_reflink_clear_inode_flag(ip, &tp);
>>>
>>> This will disable COW on /all/ blocks in the entire file, including the
>>> shared ones. At a bare minimum you'd have to scan the entire data fork
>>> to ensure there are no shared extents. That's probably why doing this
>>> causes so many new regressions.
>>
>> This function will search for shared extent before actually clearing the
>> flag. If no shared extent found, the flag won't be cleared. The name of
>> this function is not very accurate.
>
> Oh, right. I forgot that _reflink_clear_inode_flag walks the entire
> data fork looking for shared extents, and only clears the flag if it
> doesn't find any.
>
> That said, if (say) this is a large sparse file with 300 million extent
> records and extent 299,999,999 is shared, this is going to make write
> completions realllllly slow, as each completion now has to perform its
> own walk...
>
>> BTW, in my thought, the reflink flag is to indicate if a file is now
>> containing any shared extents or not. So, it should be cleared immediately
>> if no extents shared any more. Is this right?
>
> ...which is why we don't clear the flag immediately. Or ever. Only
> repairs take the time to do that.
Got it. Thank you very much!
--
Ruan.
>
> --D
>
>>
>>
>> --
>> Thanks,
>> Ruan.
>>
>> PS: Let me paste the log of failed tests:
>> generic/127, generic/263, generic/616 are fsx tests. Their fail message are
>> meaningless. I am looking into their difference between good/bad results.
>>
>> xfs/315 0s ... - output mismatch (see
>> /root/xts/results//dax_reflink/xfs/315.out.bad)
>> --- tests/xfs/315.out 2022-08-03 10:56:02.696212673 +0800
>> +++ /root/xts/results//dax_reflink/xfs/315.out.bad 2023-03-20
>> 17:48:01.780369739 +0800
>> @@ -7,7 +7,6 @@
>> Inject error
>> CoW a few blocks
>> FS should be shut down, touch will fail
>> -touch: cannot touch 'SCRATCH_MNT/badfs': Input/output error
>> Remount to replay log
>> FS should be online, touch should succeed
>> Check files again
>> ...
>> (Run 'diff -u /root/xts/tests/xfs/315.out
>> /root/xts/results//dax_reflink/xfs/315.out.bad' to see the entire diff)
>> xfs/421 1s ... - output mismatch (see
>> /root/xts/results//dax_reflink/xfs/421.out.bad)
>> --- tests/xfs/421.out 2022-08-03 10:56:02.706212718 +0800
>> +++ /root/xts/results//dax_reflink/xfs/421.out.bad 2023-03-20
>> 17:48:02.222369739 +0800
>> @@ -14,8 +14,6 @@
>> Whence Result
>> DATA 0
>> HOLE 131072
>> -DATA 196608
>> -HOLE 262144
>> Compare files
>> c2803804acc9936eef8aab42c119bfac SCRATCH_MNT/test-421/file1
>> ...
>> (Run 'diff -u /root/xts/tests/xfs/421.out
>> /root/xts/results//dax_reflink/xfs/421.out.bad' to see the entire diff)
>>
>>>
>>> --D
>>>
>>>> + if (error)
>>>> + goto out_cancel;
>>>> +
>>>> error = xfs_trans_commit(tp);
>>>> xfs_iunlock(ip, XFS_ILOCK_EXCL);
>>>> if (error)
>>>> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
>>>> index 65c5dfe17ecf..d5835814bce6 100644
>>>> --- a/fs/xfs/xfs_reflink.h
>>>> +++ b/fs/xfs/xfs_reflink.h
>>>> @@ -16,6 +16,8 @@ static inline bool xfs_is_cow_inode(struct xfs_inode *ip)
>>>> return xfs_is_reflink_inode(ip) || xfs_is_always_cow_inode(ip);
>>>> }
>>>> +int xfs_reflink_extent_is_shared(struct xfs_inode *ip,
>>>> + struct xfs_bmbt_irec *irec, bool *shared);
>>>> extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
>>>> struct xfs_bmbt_irec *irec, bool *shared);
>>>> int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
>>>> --
>>>> 2.39.2
>>>>
Powered by blists - more mailing lists