diff mbox series

xfs: fix simplify extent lookup in xfs_can_free_eofblocks

Message ID 20241002145921.GA21853@frogsfrogsfrogs (mailing list archive)
State Superseded, archived
Headers show
Series xfs: fix simplify extent lookup in xfs_can_free_eofblocks | expand

Commit Message

Darrick J. Wong Oct. 2, 2024, 2:59 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

In commit 11f4c3a53adde, we tried to simplify the extent lookup in
xfs_can_free_eofblocks so that it doesn't incur the overhead of all the
extra stuff that xfs_bmapi_read does around the iext lookup.

Unfortunately, this causes regressions on generic/603, xfs/108,
generic/219, xfs/173, generic/694, xfs/052, generic/230, and xfs/441
when always_cow is turned on.  In all cases, the regressions take the
form of alwayscow files consuming rather more space than the golden
output is expecting.  I observed that in all these cases, the cause of
the excess space usage was due to CoW fork delalloc reservations that go
beyond EOF.

For alwayscow files we allow posteof delalloc CoW reservations because
all writes go through the CoW fork.  Recall that all extents in the CoW
fork are accounted for via i_delayed_blks, which means that prior to
this patch, we'd invoke xfs_free_eofblocks on first close if anything
was in the CoW fork.  Now we don't do that.

Fix the problem by reverting the removal of the i_delayed_blks check.

Fixes: 11f4c3a53adde ("xfs: simplify extent lookup in xfs_can_free_eofblocks")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_bmap_util.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Oct. 2, 2024, 3:11 p.m. UTC | #1 
> +	if (ip->i_delayed_blks)
> +		found_blocks = true;
> +	else if (xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
>  		found_blocks = true;

This could be simplified a little bit by doing:

	if (ip->i_delayed_blks ||
	    xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
 		found_blocks = true;

but otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Oct. 2, 2024, 9:16 p.m. UTC | #2 
On Wed, Oct 02, 2024 at 08:11:17AM -0700, Christoph Hellwig wrote:
> > +	if (ip->i_delayed_blks)
> > +		found_blocks = true;
> > +	else if (xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
> >  		found_blocks = true;
> 
> This could be simplified a little bit by doing:
> 
> 	if (ip->i_delayed_blks ||
> 	    xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
>  		found_blocks = true;
> 
> but otherwise looks good:

Oops, heh, I thought I'd forgotten something. :(

V2 soon, though really there's a pile more fsdax corruption fixes so
heh.

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

Thanks!

--D
diff mbox series

Patch

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 053d567c91084..b0e0f83ff348a 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -542,10 +542,15 @@  xfs_can_free_eofblocks(
 		return false;
 
 	/*
-	 * Check if there is an post-EOF extent to free.
+	 * Check if there is an post-EOF extent to free.  If there are any
+	 * delalloc blocks attached to the inode (data fork delalloc
+	 * reservations or CoW extents of any kind), we need to free them so
+	 * that inactivation doesn't fail to erase them.
 	 */
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
-	if (xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
+	if (ip->i_delayed_blks)
+		found_blocks = true;
+	else if (xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
 		found_blocks = true;
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
 	return found_blocks;