Re: [PATCH v16 14/21] xfs: don't remove written extents past EOF on fsverity inodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] xfs: unconditional transaction storm on fsverity inode eviction
- [Medium] xfs: permanent unwritten extent leak due to premature tag clearing
--

commit 6b94de2dc949e4cfdd882209710ec5c6ad8019cf
Author: Andrey Albershteyn <aalbersh@kernel.org>

xfs: don't remove written extents past EOF on fsverity inodes

This commit updates the EOF blocks cleanup logic to handle fsverity inodes.
It skips the removal of written extents past EOF for fully constructed
fsverity files. It also cleans up delayed preallocations and unwritten
extents used during Merkle tree construction.

> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 268d159339d06..7fd951992557d 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
[ ... ]
> @@ -553,6 +554,13 @@ xfs_can_free_eofblocks(
>  	if (last_fsb <= end_fsb)
>  		return false;
>  
> +	/*
> +	 * Don't clean fsverity inodes which have merkle tree being built, the
> +	 * merkle tree is written beyond EOF
> +	 */
> +	if (xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION))
> +		return false;
> +

[Severity: High]
Looking at xfs_can_free_eofblocks() in fs/xfs/xfs_bmap_util.c, does the
subsequent lookup unconditionally find the written Merkle tree extents past
EOF for fully constructed fsverity files?

Because the Merkle tree is stored as written extents past EOF,
xfs_iext_lookup_extent() will find these blocks and return true.
This triggers xfs_free_eofblocks() on every cache eviction. Any
unprivileged user reading fsverity files could cause natural VFS
cache churn, potentially triggering an expensive transaction path
on every eviction.

[ ... ]
> @@ -593,15 +604,20 @@ xfs_free_eofblocks(
>  	 *
>  	 * Note that this means we also leave speculative preallocations in
>  	 * place for preallocated files.
> +	 *
> +	 * Clean up delalloc reservations for fsverity too as those won't be
> +	 * used
>  	 */
> -	if (ip->i_diflags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) {
> +	if (ip->i_diflags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND) ||
> +			has_verity) {
>  		if (ip->i_delayed_blks) {
>  			xfs_bmap_punch_delalloc_range(ip, XFS_DATA_FORK,
>  				round_up(XFS_ISIZE(ip), mp->m_sb.sb_blocksize),
>  				LLONG_MAX, NULL);
>  		}
>  		xfs_inode_clear_eofblocks_tag(ip);
> -		return 0;
> +		if (!has_verity)
> +			return 0;

[Severity: Medium]
Does clearing the EOF blocks tag here in xfs_free_eofblocks() permanently
leak unwritten extents if the subsequent transaction or truncation fails?

If xfs_trans_alloc() fails or xfs_itruncate_extents_flags() encounters an
error (for example, due to memory pressure during extent split in
xfs_bunmapi_range), the code jumps to err_cancel without restoring the
XFS_IEOFBLOCKS tag. Because the radix tree tag is missing, the background
blockgc worker will never revisit this inode to retry the cleanup.

>  	}
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> @@ -613,6 +629,9 @@ xfs_free_eofblocks(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	if (has_verity)
> +		bmapi_flags |= XFS_BMAPI_UNWRITTEN;
> +
>  	/*
>  	 * Do not update the on-disk file size.  If we update the on-disk file
>  	 * size and then the system crashes before the contents of the file are
> @@ -620,7 +639,7 @@ xfs_free_eofblocks(
>  	 * bug).
>  	 */
>  	error = xfs_itruncate_extents_flags(&tp, ip, XFS_DATA_FORK,
> -				XFS_ISIZE(ip), XFS_BMAPI_NODISCARD);
> +				XFS_ISIZE(ip), bmapi_flags);

[Severity: High]
While no blocks are unmapped here (since the Merkle tree is written,
not unwritten), does xfs_itruncate_extents_flags() unconditionally
dirty the inode core via xfs_trans_log_inode() and commit the
transaction?

Coupled with xfs_can_free_eofblocks() unconditionally returning true
for fsverity files, this appears to cause severe write amplification
and unnecessary journal traffic every time a fully constructed,
read-only fsverity file is evicted from the VFS cache.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918111539.1003439-1-aalbersh@kernel.org?part=14