Re: [PATCH -next v6 0/2] iomap/xfs: fix stale data exposure when truncating realtime inodes

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 2024/6/19 8:24, Darrick J. Wong wrote:
> On Tue, Jun 18, 2024 at 10:21:10PM +0800, Zhang Yi wrote:
>> Changes since v5:
>>  - Drop all the code about zeroing out the whole allocation unitsize
>>    on truncate down in xfs_setattr_size() as Christoph suggested, let's
>>    just fix this issue for RT file by converting tail blocks to
>>    unwritten now, and we could think about forced aligned extent and
>>    atomic write later until it needs, so only pick patch 6 and 8 in
>>    previous version, do some minor git log changes.
> 
> This mostly makes sense, let's see how it fares with overnight fstests.
> For now, this is a provisional
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 

Thanks for the review. BTW, what kind of fstests configs do you usually run?
I always run kvm-xfstests -g auto[1] before I submitting ext4 patches, I'd
also like to run full tests and hope to find regressions before submitting
xfs patches.

[1] https://github.com/tytso/xfstests-bld

Thanks,
Yi.

> 
> 
>> Changes since v4:
>>  - Drop the first patch in v4 "iomap: zeroing needs to be pagecache
>>    aware" since this series is not strongly depends on it, that patch
>>    still needs furtuer analyse and also should add to handle the case of
>>    a pending COW extent that extends over a data fork hole. This is a
>>    big job, so let's fix the exposure stale data issue and brings back
>>    the changes in iomap_write_end() first, don't block the ext4 buffered
>>    iomap conversion.
>>  - In patch 1, drop the 'ifndef rem_u64'.
>>  - In patch 4, factor out a helper xfs_setattr_truncate_data() to handle
>>    the zero out, update i_size, write back and drop pagecache on
>>    truncate.
>>  - In patch 5, switch to use xfs_inode_alloc_unitsize() in
>>    xfs_itruncate_extents_flags().
>>  - In patch 6, changes to reserve blocks for rtextsize > 1 realtime
>>    inodes on truncate down.
>>  - In patch 7, drop the unwritten convert threshold, always convert tail
>>    blocks to unwritten on truncate down realtime inodes.
>>  - Add patch 8 to bring back 'commit 943bc0882ceb ("iomap: don't
>>    increase i_size if it's not a write operation")'.
>>
>> Changes since v3:
>>  - Factor out a new helper to get the remainder in math64.h as Darrick
>>    suggested.
>>  - Adjust the truncating order to prevent too much redundant blocking
>>    writes as Dave suggested.
>>  - Improve to convert the tail extent to unwritten when truncating down
>>    an inode with large rtextsize as Darrick and Dave suggested.
>>
>> Since 'commit 943bc0882ceb ("iomap: don't increase i_size if it's not a
>> write operation")' merged, Chandan reported a stale data exposure issue
>> when running fstests generic/561 on xfs with realtime device [1]. This
>> issue has been fix in 6.10 by revert this commit through commit
>> '0841ea4a3b41 ("iomap: keep on increasing i_size in iomap_write_end()")',
>> but the real problem is xfs_setattr_size() doesn't zero out enough range
>> when truncate down a realtime inode. So this series fix this problem by
>> just converting the tail blocks to unwritten when truncate down realtime
>> inodes, then we could bring commit 943bc0882ceb back.
>>
>> I've tested this series on fstests (1) with reflink=0, (2) with
>> reflink=1, (3) with 28K RT device, no new failures detected, and it
>> passed generic/561 on RT device over 300+ rounds, please let me know if
>> we need any other test.
>>
>> [1] https://lore.kernel.org/linux-xfs/87ttj8ircu.fsf@debian-BULLSEYE-live-builder-AMD64/
>>
>> Thanks,
>> Yi.
>>
>> Zhang Yi (2):
>>   xfs: reserve blocks for truncating large realtime inode
>>   iomap: don't increase i_size in iomap_write_end()
>>
>>  fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++-------------------
>>  fs/xfs/xfs_iops.c      | 15 +++++++++++-
>>  2 files changed, 43 insertions(+), 25 deletions(-)
>>
>> -- 
>> 2.39.2
>>
>>