[PATCHSET v2 0/2] xfs: make xfs_can_free_eofblocks a predicate

[PATCHSET v2 0/2] xfs: make xfs_can_free_eofblocks a predicate

[Darrick J. Wong]

Hi all,

The two patches in this set reorganize the responsibilities between
xfs_can_free_eofblocks and xfs_free_eofblocks so that the first becomes
a true predicate, and the second becomes a simple update function.  The
goal is to be able to use the predicate to decide if a linked but
unopened inode has speculative post-EOF preallocations and hence must go
through the extra inactivation step.

This requires a slight change in behavior of the background block gc
workers, which will try to take the IOLOCK before calling the predicate.

v2: Various fixes recommended by Christoph.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=refactor-has-eofblocks-5.13
---
 fs/xfs/xfs_bmap_util.c |  155 ++++++++++++++++++++++++++----------------------
 fs/xfs/xfs_icache.c    |   15 ++---
 2 files changed, 92 insertions(+), 78 deletions(-)

[PATCH 1/2] xfs: move the xfs_can_free_eofblocks call under the IOLOCK

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In xfs_inode_free_eofblocks, move the xfs_can_free_eofblocks call
further down in the function to the point where we have taken the
IOLOCK.  This is preparation for the next patch, where we will need that
lock (or equivalent) so that we can check if there are any post-eof
blocks to clean out.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_icache.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 2fd4a39acb46..3c81daca0e9a 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1296,13 +1296,6 @@ xfs_inode_free_eofblocks(
 	if (!xfs_iflags_test(ip, XFS_IEOFBLOCKS))
 		return 0;
 
-	if (!xfs_can_free_eofblocks(ip, false)) {
-		/* inode could be preallocated or append-only */
-		trace_xfs_inode_free_eofblocks_invalid(ip);
-		xfs_inode_clear_eofblocks_tag(ip);
-		return 0;
-	}
-
 	/*
 	 * If the mapping is dirty the operation can block and wait for some
 	 * time. Unless we are waiting, skip it.
@@ -1324,7 +1317,13 @@ xfs_inode_free_eofblocks(
 	}
 	*lockflags |= XFS_IOLOCK_EXCL;
 
-	return xfs_free_eofblocks(ip);
+	if (xfs_can_free_eofblocks(ip, false))
+		return xfs_free_eofblocks(ip);
+
+	/* inode could be preallocated or append-only */
+	trace_xfs_inode_free_eofblocks_invalid(ip);
+	xfs_inode_clear_eofblocks_tag(ip);
+	return 0;
 }
 
 /*

[PATCH 2/2] xfs: move the check for post-EOF mappings into xfs_can_free_eofblocks

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Fix the weird split of responsibilities between xfs_can_free_eofblocks
and xfs_free_eofblocks by moving the chunk of code that looks for any
actual post-EOF space mappings from the second function into the first.

This clears the way for deferred inode inactivation to be able to decide
if an inode needs inactivation work before committing the released inode
to the inactivation code paths (vs. marking it for reclaim).

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_bmap_util.c |  155 ++++++++++++++++++++++++++----------------------
 1 file changed, 85 insertions(+), 70 deletions(-)


diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 2b9991e5ea47..e79e3d1ff38d 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -597,8 +597,24 @@ xfs_bmap_punch_delalloc_range(
  * regular files that are marked preallocated or append-only.
  */
 bool
-xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
+xfs_can_free_eofblocks(
+	struct xfs_inode	*ip,
+	bool			force)
 {
+	struct xfs_bmbt_irec	imap;
+	struct xfs_mount	*mp = ip->i_mount;
+	xfs_fileoff_t		end_fsb;
+	xfs_fileoff_t		last_fsb;
+	int			nimaps = 1;
+	int			error;
+
+	/*
+	 * Caller must either hold the exclusive io lock; or be inactivating
+	 * the inode, which guarantees there are no other users of the inode.
+	 */
+	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL) ||
+	       (VFS_I(ip)->i_state & I_FREEING));
+
 	/* prealloc/delalloc exists only on regular files */
 	if (!S_ISREG(VFS_I(ip)->i_mode))
 		return false;
@@ -624,91 +640,90 @@ xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
 		if (!force || ip->i_delayed_blks == 0)
 			return false;
 
-	return true;
-}
-
-/*
- * This is called to free any blocks beyond eof. The caller must hold
- * IOLOCK_EXCL unless we are in the inode reclaim path and have the only
- * reference to the inode.
- */
-int
-xfs_free_eofblocks(
-	struct xfs_inode	*ip)
-{
-	struct xfs_trans	*tp;
-	int			error;
-	xfs_fileoff_t		end_fsb;
-	xfs_fileoff_t		last_fsb;
-	xfs_filblks_t		map_len;
-	int			nimaps;
-	struct xfs_bmbt_irec	imap;
-	struct xfs_mount	*mp = ip->i_mount;
-
 	/*
-	 * Figure out if there are any blocks beyond the end
-	 * of the file.  If not, then there is nothing to do.
+	 * Do not try to free post-EOF blocks if EOF is beyond the end of the
+	 * range supported by the page cache, because the truncation will loop
+	 * forever.
 	 */
 	end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
 	last_fsb = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
 	if (last_fsb <= end_fsb)
-		return 0;
-	map_len = last_fsb - end_fsb;
+		return false;
 
-	nimaps = 1;
+	/*
+	 * Look up the mapping for the first block past EOF.  If we can't find
+	 * it, there's nothing to free.
+	 */
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
-	error = xfs_bmapi_read(ip, end_fsb, map_len, &imap, &nimaps, 0);
+	error = xfs_bmapi_read(ip, end_fsb, last_fsb - end_fsb, &imap, &nimaps,
+			0);
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	if (error || nimaps == 0)
+		return false;
 
 	/*
-	 * If there are blocks after the end of file, truncate the file to its
-	 * current size to free them up.
+	 * If there's a real mapping there or there are delayed allocation
+	 * reservations, then we have post-EOF blocks to try to free.
 	 */
-	if (!error && (nimaps != 0) &&
-	    (imap.br_startblock != HOLESTARTBLOCK ||
-	     ip->i_delayed_blks)) {
-		/*
-		 * Attach the dquots to the inode up front.
-		 */
-		error = xfs_qm_dqattach(ip);
-		if (error)
-			return error;
+	return imap.br_startblock != HOLESTARTBLOCK || ip->i_delayed_blks;
+}
 
-		/* wait on dio to ensure i_size has settled */
-		inode_dio_wait(VFS_I(ip));
+/*
+ * This is called to free any blocks beyond eof. The caller must hold
+ * IOLOCK_EXCL unless we are in the inode reclaim path and have the only
+ * reference to the inode.
+ */
+int
+xfs_free_eofblocks(
+	struct xfs_inode	*ip)
+{
+	struct xfs_trans	*tp;
+	struct xfs_mount	*mp = ip->i_mount;
+	int			error;
 
-		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0,
-				&tp);
-		if (error) {
-			ASSERT(XFS_FORCED_SHUTDOWN(mp));
-			return error;
-		}
+	/* Attach the dquots to the inode up front. */
+	error = xfs_qm_dqattach(ip);
+	if (error)
+		return error;
 
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_ijoin(tp, ip, 0);
+	/* Wait on dio to ensure i_size has settled. */
+	inode_dio_wait(VFS_I(ip));
 
-		/*
-		 * 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 flushed to disk then the files
-		 * may be full of holes (ie NULL files bug).
-		 */
-		error = xfs_itruncate_extents_flags(&tp, ip, XFS_DATA_FORK,
-					XFS_ISIZE(ip), XFS_BMAPI_NODISCARD);
-		if (error) {
-			/*
-			 * If we get an error at this point we simply don't
-			 * bother truncating the file.
-			 */
-			xfs_trans_cancel(tp);
-		} else {
-			error = xfs_trans_commit(tp);
-			if (!error)
-				xfs_inode_clear_eofblocks_tag(ip);
-		}
-
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
+	if (error) {
+		ASSERT(XFS_FORCED_SHUTDOWN(mp));
+		return error;
 	}
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
+	/*
+	 * 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
+	 * flushed to disk then the files may be full of holes (ie NULL files
+	 * bug).
+	 */
+	error = xfs_itruncate_extents_flags(&tp, ip, XFS_DATA_FORK,
+				XFS_ISIZE(ip), XFS_BMAPI_NODISCARD);
+	if (error)
+		goto err_cancel;
+
+	error = xfs_trans_commit(tp);
+	if (error)
+		goto out_unlock;
+
+	xfs_inode_clear_eofblocks_tag(ip);
+	goto out_unlock;
+
+err_cancel:
+	/*
+	 * If we get an error at this point we simply don't
+	 * bother truncating the file.
+	 */
+	xfs_trans_cancel(tp);
+out_unlock:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
 }
 

Re: [PATCH 1/2] xfs: move the xfs_can_free_eofblocks call under the IOLOCK

[Christoph Hellwig]

On Thu, Mar 25, 2021 at 05:21:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In xfs_inode_free_eofblocks, move the xfs_can_free_eofblocks call
> further down in the function to the point where we have taken the
> IOLOCK.  This is preparation for the next patch, where we will need that
> lock (or equivalent) so that we can check if there are any post-eof
> blocks to clean out.

I kinda prefer the old style that did the exception path inside the
branch.  But that is just sugacoating, the actual change looks good:

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

Re: [PATCH 2/2] xfs: move the check for post-EOF mappings into xfs_can_free_eofblocks

[Christoph Hellwig]

Looks good,

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

Re: [PATCH 1/2] xfs: move the xfs_can_free_eofblocks call under the IOLOCK

[Christoph Hellwig]

On Thu, Mar 18, 2021 at 11:01:10PM -0700, Darrick J. Wong wrote:
> I don't think so, because xfs_free_eofblocks will call
> xfs_inode_clear_eofblocks_tag if it succeeds in freeing anything.
> 
> Though perhaps you're correct that we need to clear the tag if
> !xfs_can_free_eofblocks, since we could have been called if
> XFS_ICI_BLOCKGC_TAG was set in the radix tree because we once had a
> posteof block but now we really only have cow blocks.

Yes, that's what I meant.

Re: [PATCH 1/2] xfs: move the xfs_can_free_eofblocks call under the IOLOCK

[Darrick J. Wong]

On Fri, Mar 19, 2021 at 05:53:40AM +0000, Christoph Hellwig wrote:
> On Thu, Mar 18, 2021 at 03:33:32PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > In xfs_inode_free_eofblocks, move the xfs_can_free_eofblocks call
> > further down in the function to the point where we have taken the
> > IOLOCK.  This is preparation for the next patch, where we will need that
> > lock (or equivalent) so that we can check if there are any post-eof
> > blocks to clean out.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_icache.c |   12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index e6a62f765422..7353c9fe05db 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -1294,13 +1294,6 @@ xfs_inode_free_eofblocks(
> >  	if (!xfs_iflags_test(ip, XFS_IEOFBLOCKS))
> >  		return 0;
> >  
> > -	if (!xfs_can_free_eofblocks(ip, false)) {
> > -		/* inode could be preallocated or append-only */
> > -		trace_xfs_inode_free_eofblocks_invalid(ip);
> > -		xfs_inode_clear_eofblocks_tag(ip);
> > -		return 0;
> > -	}
> > -
> >  	/*
> >  	 * If the mapping is dirty the operation can block and wait for some
> >  	 * time. Unless we are waiting, skip it.
> > @@ -1322,7 +1315,10 @@ xfs_inode_free_eofblocks(
> >  	}
> >  	*lockflags |= XFS_IOLOCK_EXCL;
> >  
> > -	return xfs_free_eofblocks(ip);
> > +	if (xfs_can_free_eofblocks(ip, false))
> > +		return xfs_free_eofblocks(ip);
> 
> Don't we still need to clear the radix tree tag here?

I don't think so, because xfs_free_eofblocks will call
xfs_inode_clear_eofblocks_tag if it succeeds in freeing anything.

Though perhaps you're correct that we need to clear the tag if
!xfs_can_free_eofblocks, since we could have been called if
XFS_ICI_BLOCKGC_TAG was set in the radix tree because we once had a
posteof block but now we really only have cow blocks.

Yeah, ok, I'll add that back...

> Also the fs_inode_free_eofblocks_inval tracepoint is unused now.

...along with the tracepoint.

--D

Re: [PATCH 1/2] xfs: move the xfs_can_free_eofblocks call under the IOLOCK

[Christoph Hellwig]

On Thu, Mar 18, 2021 at 03:33:32PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In xfs_inode_free_eofblocks, move the xfs_can_free_eofblocks call
> further down in the function to the point where we have taken the
> IOLOCK.  This is preparation for the next patch, where we will need that
> lock (or equivalent) so that we can check if there are any post-eof
> blocks to clean out.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_icache.c |   12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index e6a62f765422..7353c9fe05db 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1294,13 +1294,6 @@ xfs_inode_free_eofblocks(
>  	if (!xfs_iflags_test(ip, XFS_IEOFBLOCKS))
>  		return 0;
>  
> -	if (!xfs_can_free_eofblocks(ip, false)) {
> -		/* inode could be preallocated or append-only */
> -		trace_xfs_inode_free_eofblocks_invalid(ip);
> -		xfs_inode_clear_eofblocks_tag(ip);
> -		return 0;
> -	}
> -
>  	/*
>  	 * If the mapping is dirty the operation can block and wait for some
>  	 * time. Unless we are waiting, skip it.
> @@ -1322,7 +1315,10 @@ xfs_inode_free_eofblocks(
>  	}
>  	*lockflags |= XFS_IOLOCK_EXCL;
>  
> -	return xfs_free_eofblocks(ip);
> +	if (xfs_can_free_eofblocks(ip, false))
> +		return xfs_free_eofblocks(ip);

Don't we still need to clear the radix tree tag here?

Also the fs_inode_free_eofblocks_inval tracepoint is unused now.

[PATCH 1/2] xfs: move the xfs_can_free_eofblocks call under the IOLOCK

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In xfs_inode_free_eofblocks, move the xfs_can_free_eofblocks call
further down in the function to the point where we have taken the
IOLOCK.  This is preparation for the next patch, where we will need that
lock (or equivalent) so that we can check if there are any post-eof
blocks to clean out.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_icache.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index e6a62f765422..7353c9fe05db 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -1294,13 +1294,6 @@ xfs_inode_free_eofblocks(
 	if (!xfs_iflags_test(ip, XFS_IEOFBLOCKS))
 		return 0;
 
-	if (!xfs_can_free_eofblocks(ip, false)) {
-		/* inode could be preallocated or append-only */
-		trace_xfs_inode_free_eofblocks_invalid(ip);
-		xfs_inode_clear_eofblocks_tag(ip);
-		return 0;
-	}
-
 	/*
 	 * If the mapping is dirty the operation can block and wait for some
 	 * time. Unless we are waiting, skip it.
@@ -1322,7 +1315,10 @@ xfs_inode_free_eofblocks(
 	}
 	*lockflags |= XFS_IOLOCK_EXCL;
 
-	return xfs_free_eofblocks(ip);
+	if (xfs_can_free_eofblocks(ip, false))
+		return xfs_free_eofblocks(ip);
+
+	return 0;
 }
 
 /*