mbox series

[RFC,v4,0/8] iomap/xfs: fix stale data exposure when truncating realtime inodes

Message ID 20240529095206.2568162-1-yi.zhang@huaweicloud.com (mailing list archive)
Headers show
Series iomap/xfs: fix stale data exposure when truncating realtime inodes | expand

Message

Zhang Yi May 29, 2024, 9:51 a.m. UTC
From: Zhang Yi <yi.zhang@huawei.com>

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.

This series fix a stale data exposure issue reported by Chandan when
running fstests generic/561 on xfs with realtime device[1]. The real
problem is xfs_setattr_size() doesn't zero out enough range when
truncating a realtime inode, please see the patch 6 or [1] for
details.

Patch 1 is from Dave, it improves truncate down performace by changing
iomap_zero_iter() to aware dirty pages on unwritten extents, but for the
case of the zeroing range that contains a cow mapping over a hole still
needs to be handled. 

Patch 3-5 modify iomap_truncate_page() and dax_truncate_page() to pass
filesystem identified blocksize, and drop the assumption of
i_blocksize() as Dave suggested.

Patch 6-7 adjust the truncating down processing order to first zero out
the tail aligned blocks, then write back, update i_size and finally drop
cache beyond aligned EOF. Fix the data exposure issue by zeroing out the
entire EOF extent.

Patch 8-9 add a rtextsize threshold (64k), improves truncate down performace
on realtime inode with large rtextsize (beyonds this threshold) by
converting the tail unaligned extent to unwritten.

I've tested this series on fstests (1) with reflink=0, (2) with 28K RT
device and (3) with 96K RT device (beyonds rtextsize threshold), no new
failures detected. This series still needs to do furtuer tests with
reflink=1 after Patch 1 covers the cow mapping over a hole case.

[1] https://lore.kernel.org/linux-xfs/87ttj8ircu.fsf@debian-BULLSEYE-live-builder-AMD64/

Thanks,
Yi.

Dave Chinner (1):
  iomap: zeroing needs to be pagecache aware

Zhang Yi (7):
  math64: add rem_u64() to just return the remainder
  iomap: pass blocksize to iomap_truncate_page()
  fsdax: pass blocksize to dax_truncate_page()
  xfs: refactor the truncating order
  xfs: correct the truncate blocksize of realtime inode
  xfs: reserve blocks for truncating realtime inode
  xfs: improve truncate on a realtime inode with huge extsize

 fs/dax.c               |   8 +--
 fs/ext2/inode.c        |   4 +-
 fs/iomap/buffered-io.c |  50 ++++++++++++++--
 fs/xfs/xfs_inode.c     |   3 +
 fs/xfs/xfs_inode.h     |  12 ++++
 fs/xfs/xfs_iomap.c     |   5 +-
 fs/xfs/xfs_iomap.h     |   3 +-
 fs/xfs/xfs_iops.c      | 133 +++++++++++++++++++++++++----------------
 include/linux/dax.h    |   4 +-
 include/linux/iomap.h  |   4 +-
 include/linux/math64.h |  24 ++++++++
 11 files changed, 179 insertions(+), 71 deletions(-)

Comments

Christoph Hellwig May 31, 2024, 12:26 p.m. UTC | #1
Procedural question before I get into the actual review:  given we
are close to -rc3 and there is no user of the iomap change yet,
should we revert that for 6.10 and instead try again in 6.11 when
the XFS bits are sorted out?
Zhang Yi June 1, 2024, 7:38 a.m. UTC | #2
On 2024/5/31 20:26, Christoph Hellwig wrote:
> Procedural question before I get into the actual review:  given we
> are close to -rc3 and there is no user of the iomap change yet,
> should we revert that for 6.10 and instead try again in 6.11 when
> the XFS bits are sorted out?
> 

Okay, fine, it looks we still need some time to fix this issue.  I
will send out a patch to revert the commit '943bc0882ceb ("iomap:
don't increase i_size if it's not a write operation")' soon, other
commits in my previous series looks harmless, so I think we can
keep them.

Thanks,
Yi.
Christoph Hellwig June 1, 2024, 7:40 a.m. UTC | #3
On Sat, Jun 01, 2024 at 03:38:37PM +0800, Zhang Yi wrote:
> will send out a patch to revert the commit '943bc0882ceb ("iomap:
> don't increase i_size if it's not a write operation")' soon, other
> commits in my previous series looks harmless, so I think we can
> keep them.

Agreed and thanks!  For 6.11 we'll also need to figure out how to
keep the related changes together.  I suspect we should just re-merge
that iomap change through the XFS tree, but we can talk about that
later.