Message-ID: <20240619002444.GH103034@frogsfrogsfrogs>
Date: Tue, 18 Jun 2024 17:24:44 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, hch@...radead.org, brauner@...nel.org,
	david@...morbit.com, chandanbabu@...nel.org, jack@...e.cz,
	yi.zhang@...wei.com, chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH -next v6 0/2] iomap/xfs: fix stale data exposure when
 truncating realtime inodes

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@...nel.org>

--D


> 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
> 
> 

Powered by blists - more mailing lists