Message ID | 161610681213.1887542.5172499515393116902.stgit@magnolia (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: make xfs_can_free_eofblocks a predicate | expand |
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.
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
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.
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; } /*