From: Zhang Yi <[email protected]>
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 on 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
zeroing out allocation unitsize and convert the tail blocks to unwritten
when truncate down realtime inodes, finally we could bring commit
943bc0882ceb back.
Patch 1-3 modify iomap_truncate_page() and dax_truncate_page() to pass
filesystem identified blocksize, and drop the assumption of
i_blocksize() as Dave suggested.
Patch 4-5 refactor and adjust the truncating down processing order to
first zero out the tail aligned blocks, then write back and update
i_size, finally drop cache beyond aligned EOF. Fix the data exposure
issue by zeroing out the entire EOF extent.
Patch 6-7 improves truncate down performace on realtime inodes with
big rtextsize(>1 fsblock) by converting the tail unaligned extent to
unwritten.
Patch 8 reverts commit 0841ea4a3b41 and brings commit 943bc0882ceb back,
don't increase i_size on IOMAP_ZERO and IOMAP_UNSHARE.
I've tested this series on fstests (1) with reflink=0, (2) with
reflink=1, (3) with 28K RT device and (4) with dax, no new failures
detected, and it passed generic/561 on RT device over 1000+ rounds,
please let me know if it needs other tests.
[1] https://lore.kernel.org/linux-xfs/87ttj8ircu.fsf@debian-BULLSEYE-live-builder-AMD64/
Thanks,
Yi.
Zhang Yi (8):
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 large realtime inode
xfs: speed up truncating down a big realtime inode
iomap: don't increase i_size in iomap_write_end()
fs/dax.c | 8 +-
fs/ext2/inode.c | 4 +-
fs/iomap/buffered-io.c | 61 +++++++-------
fs/xfs/xfs_inode.c | 9 ++-
fs/xfs/xfs_iomap.c | 5 +-
fs/xfs/xfs_iomap.h | 3 +-
fs/xfs/xfs_iops.c | 180 ++++++++++++++++++++++++++++-------------
include/linux/dax.h | 4 +-
include/linux/iomap.h | 4 +-
include/linux/math64.h | 22 +++++
10 files changed, 204 insertions(+), 96 deletions(-)
--
2.39.2
From: Zhang Yi <[email protected]>
When truncating down an inode, we call xfs_truncate_page() to zero out
the tail partial block that beyond new EOF, which prevents exposing
stale data. But xfs_truncate_page() always assumes the blocksize is
i_blocksize(inode), it's not always true if we have a large allocation
unit for a file and we should aligned to this unitsize, e.g. realtime
inode should aligned to the rtextsize.
Current xfs_setattr_size() can't support zeroing out a large alignment
size on trucate down since the process order is wrong. We first do zero
out through xfs_truncate_page(), and then update inode size through
truncate_setsize() immediately. If the zeroed range is larger than a
folio, the write back path would not write back zeroed pagecache beyond
the EOF folio, so it doesn't write zeroes to the entire tail extent and
could expose stale data after an appending write into the next aligned
extent.
We need to adjust the order to zero out tail aligned blocks, write back
zeroed or cached data, update i_size and drop all the pagecache beyond
the allocation unit containing EOF, preparing for the fix of realtime
inode and supporting the upcoming forced alignment feature.
Signed-off-by: Zhang Yi <[email protected]>
---
fs/xfs/xfs_iomap.c | 2 +-
fs/xfs/xfs_iomap.h | 3 +-
fs/xfs/xfs_iops.c | 162 +++++++++++++++++++++++++++++----------------
3 files changed, 109 insertions(+), 58 deletions(-)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8cdfcbb5baa7..0369b64cc3f4 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1468,10 +1468,10 @@ int
xfs_truncate_page(
struct xfs_inode *ip,
loff_t pos,
+ unsigned int blocksize,
bool *did_zero)
{
struct inode *inode = VFS_I(ip);
- unsigned int blocksize = i_blocksize(inode);
if (IS_DAX(inode))
return dax_truncate_page(inode, pos, blocksize, did_zero,
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index 4da13440bae9..feb1610cb645 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -25,7 +25,8 @@ int xfs_bmbt_to_iomap(struct xfs_inode *ip, struct iomap *iomap,
int xfs_zero_range(struct xfs_inode *ip, loff_t pos, loff_t len,
bool *did_zero);
-int xfs_truncate_page(struct xfs_inode *ip, loff_t pos, bool *did_zero);
+int xfs_truncate_page(struct xfs_inode *ip, loff_t pos,
+ unsigned int blocksize, bool *did_zero);
static inline xfs_filblks_t
xfs_aligned_fsb_count(
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ff222827e550..0919a42cceb6 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -792,6 +792,108 @@ xfs_setattr_nonsize(
return error;
}
+/*
+ * Zero and flush data on truncate.
+ *
+ * Zero out any data beyond EOF on size changed truncate, write back
+ * all cached data if we need to extend ondisk EOF, and drop all the
+ * pagecache that beyond the new EOF block.
+ */
+STATIC int
+xfs_setattr_truncate_data(
+ struct xfs_inode *ip,
+ xfs_off_t oldsize,
+ xfs_off_t newsize)
+{
+ struct inode *inode = VFS_I(ip);
+ bool did_zeroing = false;
+ bool extending_ondisk_eof;
+ unsigned int blocksize;
+ int error;
+
+ extending_ondisk_eof = newsize > ip->i_disk_size &&
+ oldsize != ip->i_disk_size;
+
+ /*
+ * Start with zeroing any data beyond EOF that we may expose on file
+ * extension, or zeroing out the rest of the block on a downward
+ * truncate.
+ *
+ * We've already locked out new page faults, so now we can safely call
+ * truncate_setsize() or truncate_pagecache() to remove pages from the
+ * page cache knowing they won't get refaulted until we drop the
+ * XFS_MMAPLOCK_EXCL after the extent manipulations are complete. The
+ * truncate_setsize() call also cleans partial EOF page PTEs on
+ * extending truncates and hence ensures sub-page block size filesystems
+ * are correctly handled, too.
+ */
+ if (newsize >= oldsize) {
+ /* File extentsion */
+ if (newsize != oldsize) {
+ trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
+ error = xfs_zero_range(ip, oldsize, newsize - oldsize,
+ &did_zeroing);
+ if (error)
+ return error;
+ }
+
+ truncate_setsize(inode, newsize);
+
+ /*
+ * We are going to log the inode size change in this transaction
+ * so any previous writes that are beyond the on disk EOF and
+ * the new EOF that have not been written out need to be written
+ * here. If we do not write the data out, we expose ourselves
+ * to the null files problem. Note that this includes any block
+ * zeroing we did above; otherwise those blocks may not be
+ * zeroed after a crash.
+ */
+ if (did_zeroing || extending_ondisk_eof) {
+ error = filemap_write_and_wait_range(inode->i_mapping,
+ ip->i_disk_size, newsize - 1);
+ if (error)
+ return error;
+ }
+ return 0;
+ }
+
+ /* Truncate down */
+ blocksize = i_blocksize(inode);
+
+ /*
+ * iomap won't detect a dirty page over an unwritten block (or a cow
+ * block over a hole) and subsequently skips zeroing the newly post-EOF
+ * portion of the page. Flush the new EOF to convert the block before
+ * the pagecache truncate.
+ */
+ error = filemap_write_and_wait_range(inode->i_mapping, newsize,
+ roundup_64(newsize, blocksize) - 1);
+ if (error)
+ return error;
+
+ error = xfs_truncate_page(ip, newsize, blocksize, &did_zeroing);
+ if (error)
+ return error;
+
+ if (did_zeroing || extending_ondisk_eof) {
+ error = filemap_write_and_wait_range(inode->i_mapping,
+ min_t(loff_t, ip->i_disk_size, newsize),
+ roundup_64(newsize, blocksize) - 1);
+ if (error)
+ return error;
+ }
+
+ /*
+ * Open code truncate_setsize(), update the incore i_size after flushing
+ * dirty tail pages to disk, don't zero out the partial EOF folio which
+ * may contains already zeroed tail blocks again and just drop all the
+ * pagecache beyond the allocation unit containing EOF.
+ */
+ i_size_write(inode, newsize);
+ truncate_pagecache(inode, roundup_64(newsize, blocksize));
+ return 0;
+}
+
/*
* Truncate file. Must have write permission and not be a directory.
*
@@ -811,7 +913,6 @@ xfs_setattr_size(
struct xfs_trans *tp;
int error;
uint lock_flags = 0;
- bool did_zeroing = false;
xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
ASSERT(S_ISREG(inode->i_mode));
@@ -853,40 +954,7 @@ xfs_setattr_size(
* the transaction because the inode cannot be unlocked once it is a
* part of the transaction.
*
- * Start with zeroing any data beyond EOF that we may expose on file
- * extension, or zeroing out the rest of the block on a downward
- * truncate.
- */
- if (newsize > oldsize) {
- trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
- error = xfs_zero_range(ip, oldsize, newsize - oldsize,
- &did_zeroing);
- } else {
- /*
- * iomap won't detect a dirty page over an unwritten block (or a
- * cow block over a hole) and subsequently skips zeroing the
- * newly post-EOF portion of the page. Flush the new EOF to
- * convert the block before the pagecache truncate.
- */
- error = filemap_write_and_wait_range(inode->i_mapping, newsize,
- newsize);
- if (error)
- return error;
- error = xfs_truncate_page(ip, newsize, &did_zeroing);
- }
-
- if (error)
- return error;
-
- /*
- * We've already locked out new page faults, so now we can safely remove
- * pages from the page cache knowing they won't get refaulted until we
- * drop the XFS_MMAP_EXCL lock after the extent manipulations are
- * complete. The truncate_setsize() call also cleans partial EOF page
- * PTEs on extending truncates and hence ensures sub-page block size
- * filesystems are correctly handled, too.
- *
- * We have to do all the page cache truncate work outside the
+ * We also have to do all the page cache truncate work outside the
* transaction context as the "lock" order is page lock->log space
* reservation as defined by extent allocation in the writeback path.
* Hence a truncate can fail with ENOMEM from xfs_trans_alloc(), but
@@ -894,28 +962,10 @@ xfs_setattr_size(
* user visible changes). There's not much we can do about this, except
* to hope that the caller sees ENOMEM and retries the truncate
* operation.
- *
- * And we update in-core i_size and truncate page cache beyond newsize
- * before writeback the [i_disk_size, newsize] range, so we're
- * guaranteed not to write stale data past the new EOF on truncate down.
*/
- truncate_setsize(inode, newsize);
-
- /*
- * We are going to log the inode size change in this transaction so
- * any previous writes that are beyond the on disk EOF and the new
- * EOF that have not been written out need to be written here. If we
- * do not write the data out, we expose ourselves to the null files
- * problem. Note that this includes any block zeroing we did above;
- * otherwise those blocks may not be zeroed after a crash.
- */
- if (did_zeroing ||
- (newsize > ip->i_disk_size && oldsize != ip->i_disk_size)) {
- error = filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
- ip->i_disk_size, newsize - 1);
- if (error)
- return error;
- }
+ error = xfs_setattr_truncate_data(ip, oldsize, newsize);
+ if (error)
+ return error;
error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
if (error)
--
2.39.2