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

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

[Zhang Yi]

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.

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

[PATCH -next v6 1/2] xfs: reserve blocks for truncating large realtime inode

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

When unaligned truncate down a big realtime file, xfs_truncate_page()
only zeros out the tail EOF block, __xfs_bunmapi() should split the tail
written extent and convert the later one that beyond EOF block to
unwritten, but it couldn't work as expected now since the reserved block
is zero in xfs_setattr_size(), this could expose stale data just after
commit '943bc0882ceb ("iomap: don't increase i_size if it's not a write
operation")'.

If we truncate file that contains a large enough written extent:

     |<    rxext    >|<    rtext    >|
  ...WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
        ^ (new EOF)      ^ old EOF

Since we only zeros out the tail of the EOF block, and
xfs_itruncate_extents()->..->__xfs_bunmapi() unmap the whole ailgned
extents, it becomes this state:

     |<    rxext    >|
  ...WWWzWWWWWWWWWWWWW
        ^ new EOF

Then if we do an extending write like this, the blocks in the previous
tail extent becomes stale:

     |<    rxext    >|
  ...WWWzSSSSSSSSSSSSS..........WWWWWWWWWWWWWWWWW
        ^ old EOF               ^ append start  ^ new EOF

Fix this by reserving XFS_DIOSTRAT_SPACE_RES blocks for big realtime
inode.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iops.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ff222827e550..a00dcbc77e12 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -17,6 +17,8 @@
 #include "xfs_da_btree.h"
 #include "xfs_attr.h"
 #include "xfs_trans.h"
+#include "xfs_trans_space.h"
+#include "xfs_bmap_btree.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
 #include "xfs_symlink.h"
@@ -811,6 +813,7 @@ xfs_setattr_size(
 	struct xfs_trans	*tp;
 	int			error;
 	uint			lock_flags = 0;
+	uint			resblks = 0;
 	bool			did_zeroing = false;
 
 	xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
@@ -917,7 +920,17 @@ xfs_setattr_size(
 			return error;
 	}
 
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
+	/*
+	 * For realtime inode with more than one block rtextsize, we need the
+	 * block reservation for bmap btree block allocations/splits that can
+	 * happen since it could split the tail written extent and convert the
+	 * right beyond EOF one to unwritten.
+	 */
+	if (xfs_inode_has_bigrtalloc(ip))
+		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, resblks,
+				0, 0, &tp);
 	if (error)
 		return error;
 
-- 
2.39.2

[PATCH -next v6 2/2] iomap: don't increase i_size in iomap_write_end()

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

This reverts commit '0841ea4a3b41 ("iomap: keep on increasing i_size in
iomap_write_end()")'.

After xfs could zero out the tail blocks aligned to the allocation
unitsize and convert the tail blocks to unwritten for realtime inode on
truncate down, it couldn't expose any stale data when unaligned truncate
down realtime inodes, so we could keep on keeping i_size for
IOMAP_UNSHARE and IOMAP_ZERO in iomap_write_end().

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d46558990279..99bedc3f7c39 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -878,37 +878,22 @@ static bool iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
 		size_t copied, struct folio *folio)
 {
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
-	loff_t old_size = iter->inode->i_size;
-	size_t written;
 
 	if (srcmap->type == IOMAP_INLINE) {
 		iomap_write_end_inline(iter, folio, pos, copied);
-		written = copied;
-	} else if (srcmap->flags & IOMAP_F_BUFFER_HEAD) {
-		written = block_write_end(NULL, iter->inode->i_mapping, pos,
-					len, copied, &folio->page, NULL);
-		WARN_ON_ONCE(written != copied && written != 0);
-	} else {
-		written = __iomap_write_end(iter->inode, pos, len, copied,
-					    folio) ? copied : 0;
+		return true;
 	}
 
-	/*
-	 * Update the in-memory inode size after copying the data into the page
-	 * cache.  It's up to the file system to write the updated size to disk,
-	 * preferably after I/O completion so that no stale data is exposed.
-	 * Only once that's done can we unlock and release the folio.
-	 */
-	if (pos + written > old_size) {
-		i_size_write(iter->inode, pos + written);
-		iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
-	}
-	__iomap_put_folio(iter, pos, written, folio);
+	if (srcmap->flags & IOMAP_F_BUFFER_HEAD) {
+		size_t bh_written;
 
-	if (old_size < pos)
-		pagecache_isize_extended(iter->inode, old_size, pos);
+		bh_written = block_write_end(NULL, iter->inode->i_mapping, pos,
+					len, copied, &folio->page, NULL);
+		WARN_ON_ONCE(bh_written != copied && bh_written != 0);
+		return bh_written == copied;
+	}
 
-	return written == copied;
+	return __iomap_write_end(iter->inode, pos, len, copied, folio);
 }
 
 static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
@@ -923,6 +908,7 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 
 	do {
 		struct folio *folio;
+		loff_t old_size;
 		size_t offset;		/* Offset into folio */
 		size_t bytes;		/* Bytes to write to folio */
 		size_t copied;		/* Bytes copied from user */
@@ -974,6 +960,23 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		written = iomap_write_end(iter, pos, bytes, copied, folio) ?
 			  copied : 0;
 
+		/*
+		 * Update the in-memory inode size after copying the data into
+		 * the page cache.  It's up to the file system to write the
+		 * updated size to disk, preferably after I/O completion so that
+		 * no stale data is exposed.  Only once that's done can we
+		 * unlock and release the folio.
+		 */
+		old_size = iter->inode->i_size;
+		if (pos + written > old_size) {
+			i_size_write(iter->inode, pos + written);
+			iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
+		}
+		__iomap_put_folio(iter, pos, written, folio);
+
+		if (old_size < pos)
+			pagecache_isize_extended(iter->inode, old_size, pos);
+
 		cond_resched();
 		if (unlikely(written == 0)) {
 			/*
@@ -1344,6 +1347,7 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
 			bytes = folio_size(folio) - offset;
 
 		ret = iomap_write_end(iter, pos, bytes, bytes, folio);
+		__iomap_put_folio(iter, pos, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
 
@@ -1409,6 +1413,7 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		folio_mark_accessed(folio);
 
 		ret = iomap_write_end(iter, pos, bytes, bytes, folio);
+		__iomap_put_folio(iter, pos, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
 
-- 
2.39.2

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

[Darrick J. Wong]

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>

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

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

[Zhang Yi]

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

Re: [PATCH -next v6 2/2] iomap: don't increase i_size in iomap_write_end()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christian Brauner]

On Tue, 18 Jun 2024 22:21:10 +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.
> 
> [...]

I've put this into vfs.iomap for testing which should end up in fs-next asap.

---

Applied to the vfs.iomap branch of the vfs/vfs.git tree.
Patches in the vfs.iomap branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.iomap

[1/2] xfs: reserve blocks for truncating large realtime inode
      https://git.kernel.org/vfs/vfs/c/d048945150b7
[2/2] iomap: don't increase i_size in iomap_write_end()
      https://git.kernel.org/vfs/vfs/c/602f09f4029c

Re: [PATCH -next v6 1/2] xfs: reserve blocks for truncating large realtime inode

[Dave Chinner]

On Tue, Jun 18, 2024 at 10:21:11PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> When unaligned truncate down a big realtime file, xfs_truncate_page()
> only zeros out the tail EOF block, __xfs_bunmapi() should split the tail
> written extent and convert the later one that beyond EOF block to
> unwritten, but it couldn't work as expected now since the reserved block
> is zero in xfs_setattr_size(), this could expose stale data just after
> commit '943bc0882ceb ("iomap: don't increase i_size if it's not a write
> operation")'.
> 
> If we truncate file that contains a large enough written extent:
> 
>      |<    rxext    >|<    rtext    >|
>   ...WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
>         ^ (new EOF)      ^ old EOF
> 
> Since we only zeros out the tail of the EOF block, and
> xfs_itruncate_extents()->..->__xfs_bunmapi() unmap the whole ailgned
> extents, it becomes this state:
> 
>      |<    rxext    >|
>   ...WWWzWWWWWWWWWWWWW
>         ^ new EOF
> 
> Then if we do an extending write like this, the blocks in the previous
> tail extent becomes stale:
> 
>      |<    rxext    >|
>   ...WWWzSSSSSSSSSSSSS..........WWWWWWWWWWWWWWWWW
>         ^ old EOF               ^ append start  ^ new EOF
> 
> Fix this by reserving XFS_DIOSTRAT_SPACE_RES blocks for big realtime
> inode.

This same problem is going to happen with force aligned allocations,
right? i.e. it is a result of having a allocation block size larger
than one filesystem block?

If so, then....

> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_iops.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ff222827e550..a00dcbc77e12 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -17,6 +17,8 @@
>  #include "xfs_da_btree.h"
>  #include "xfs_attr.h"
>  #include "xfs_trans.h"
> +#include "xfs_trans_space.h"
> +#include "xfs_bmap_btree.h"
>  #include "xfs_trace.h"
>  #include "xfs_icache.h"
>  #include "xfs_symlink.h"
> @@ -811,6 +813,7 @@ xfs_setattr_size(
>  	struct xfs_trans	*tp;
>  	int			error;
>  	uint			lock_flags = 0;
> +	uint			resblks = 0;
>  	bool			did_zeroing = false;
>  
>  	xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
> @@ -917,7 +920,17 @@ xfs_setattr_size(
>  			return error;
>  	}
>  
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> +	/*
> +	 * For realtime inode with more than one block rtextsize, we need the
> +	 * block reservation for bmap btree block allocations/splits that can
> +	 * happen since it could split the tail written extent and convert the
> +	 * right beyond EOF one to unwritten.
> +	 */
> +	if (xfs_inode_has_bigrtalloc(ip))
> +		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);

.... should this be doing this generic check instead:

	if (xfs_inode_alloc_unitsize(ip) > 1)
		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH -next v6 1/2] xfs: reserve blocks for truncating large realtime inode

[Zhang Yi]

On 2024/7/1 9:16, Dave Chinner wrote:
> On Tue, Jun 18, 2024 at 10:21:11PM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> When unaligned truncate down a big realtime file, xfs_truncate_page()
>> only zeros out the tail EOF block, __xfs_bunmapi() should split the tail
>> written extent and convert the later one that beyond EOF block to
>> unwritten, but it couldn't work as expected now since the reserved block
>> is zero in xfs_setattr_size(), this could expose stale data just after
>> commit '943bc0882ceb ("iomap: don't increase i_size if it's not a write
>> operation")'.
>>
>> If we truncate file that contains a large enough written extent:
>>
>>      |<    rxext    >|<    rtext    >|
>>   ...WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
>>         ^ (new EOF)      ^ old EOF
>>
>> Since we only zeros out the tail of the EOF block, and
>> xfs_itruncate_extents()->..->__xfs_bunmapi() unmap the whole ailgned
>> extents, it becomes this state:
>>
>>      |<    rxext    >|
>>   ...WWWzWWWWWWWWWWWWW
>>         ^ new EOF
>>
>> Then if we do an extending write like this, the blocks in the previous
>> tail extent becomes stale:
>>
>>      |<    rxext    >|
>>   ...WWWzSSSSSSSSSSSSS..........WWWWWWWWWWWWWWWWW
>>         ^ old EOF               ^ append start  ^ new EOF
>>
>> Fix this by reserving XFS_DIOSTRAT_SPACE_RES blocks for big realtime
>> inode.
> 
> This same problem is going to happen with force aligned allocations,
> right? i.e. it is a result of having a allocation block size larger
> than one filesystem block?
> 
Yeah, right.

+cc John

> If so, then....
> 
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  fs/xfs/xfs_iops.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index ff222827e550..a00dcbc77e12 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -17,6 +17,8 @@
>>  #include "xfs_da_btree.h"
>>  #include "xfs_attr.h"
>>  #include "xfs_trans.h"
>> +#include "xfs_trans_space.h"
>> +#include "xfs_bmap_btree.h"
>>  #include "xfs_trace.h"
>>  #include "xfs_icache.h"
>>  #include "xfs_symlink.h"
>> @@ -811,6 +813,7 @@ xfs_setattr_size(
>>  	struct xfs_trans	*tp;
>>  	int			error;
>>  	uint			lock_flags = 0;
>> +	uint			resblks = 0;
>>  	bool			did_zeroing = false;
>>  
>>  	xfs_assert_ilocked(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL);
>> @@ -917,7 +920,17 @@ xfs_setattr_size(
>>  			return error;
>>  	}
>>  
>> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
>> +	/*
>> +	 * For realtime inode with more than one block rtextsize, we need the
>> +	 * block reservation for bmap btree block allocations/splits that can
>> +	 * happen since it could split the tail written extent and convert the
>> +	 * right beyond EOF one to unwritten.
>> +	 */
>> +	if (xfs_inode_has_bigrtalloc(ip))
>> +		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> 
> .... should this be doing this generic check instead:
> 
> 	if (xfs_inode_alloc_unitsize(ip) > 1)

        if (xfs_inode_alloc_unitsize(ip) > i_blocksize(inode)) ?

> 		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> 

Yeah, it makes sense to me, but Christoph suggested to think about force
aligned allocations later, so I only dealt with the big RT inode case here.
I can revise it if John and Christoph don't object.

Thanks,
Yi.

Re: [PATCH -next v6 1/2] xfs: reserve blocks for truncating large realtime inode

[Dave Chinner]

On Mon, Jul 01, 2024 at 10:26:18AM +0800, Zhang Yi wrote:
> On 2024/7/1 9:16, Dave Chinner wrote:
> > On Tue, Jun 18, 2024 at 10:21:11PM +0800, Zhang Yi wrote:
> >> @@ -917,7 +920,17 @@ xfs_setattr_size(
> >>  			return error;
> >>  	}
> >>  
> >> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> >> +	/*
> >> +	 * For realtime inode with more than one block rtextsize, we need the
> >> +	 * block reservation for bmap btree block allocations/splits that can
> >> +	 * happen since it could split the tail written extent and convert the
> >> +	 * right beyond EOF one to unwritten.
> >> +	 */
> >> +	if (xfs_inode_has_bigrtalloc(ip))
> >> +		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> > 
> > .... should this be doing this generic check instead:
> > 
> > 	if (xfs_inode_alloc_unitsize(ip) > 1)
> 
>         if (xfs_inode_alloc_unitsize(ip) > i_blocksize(inode)) ?
> 
> > 		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
> > 
> 
> Yeah, it makes sense to me, but Christoph suggested to think about force
> aligned allocations later, so I only dealt with the big RT inode case here.
> I can revise it if John and Christoph don't object.

Sorry, but I don't really care what either John or Christoph say on
this matter: xfs_inode_has_bigrtalloc() is recently introduced
technical debt that should not be propagated further.

xfs_inode_has_bigrtalloc() needs to be replaced completely with
xfs_inode_alloc_unitsize() and any conditional behaviour needed can
be based on the return value from xfs_inode_alloc_unitsize(). That
works for everything that has an allocation block size larger than
one filesystem block, not just one specific RT case.

Don't force John to have fix all these same RT bugs that are being
fixed with xfs_inode_has_bigrtalloc() just because forced alignment
stuff is not yet merged. Don't make John's life harder than it needs
to be to get that stuff merged, and don't waste my time arguing
about it: just fix the problem the right way the first time.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH -next v6 1/2] xfs: reserve blocks for truncating large realtime inode

[Christoph Hellwig]

On Mon, Jul 01, 2024 at 10:35:07PM +1000, Dave Chinner wrote:
> Sorry, but I don't really care what either John or Christoph say on
> this matter: xfs_inode_has_bigrtalloc() is recently introduced
> technical debt that should not be propagated further.

So send a patch to fix it.

> xfs_inode_has_bigrtalloc() needs to be replaced completely with
> xfs_inode_alloc_unitsize() and any conditional behaviour needed can
> be based on the return value from xfs_inode_alloc_unitsize(). That
> works for everything that has an allocation block size larger than
> one filesystem block, not just one specific RT case.

Only assuming we actually get these larger alloc sizes.  Which right
now we don't have, and to be honest I'm not sure they are a good
idea.  The whole larger alloc size thing has been a massive pain
to deal with, and we'll need good argument for furthering that pain.