Re: [PATCH 02/11] xfs: refactor the predicate part of xfs_free_eofblocks

On Wed, Mar 17, 2021 at 09:33:29PM -0700, Darrick J. Wong wrote:

On Mon, Mar 15, 2021 at 06:46:15PM +0000, Christoph Hellwig wrote:

quoted

Going further through the series actually made me go back to this one,
so a few more comments:

quoted

 /*
+ * Decide if this inode have post-EOF blocks.  The caller is responsible
+ * for knowing / caring about the PREALLOC/APPEND flags.

Please spell out the XFS_DIFLAG_ here, as this really confused me.  In
fact even with that it still confuses me, as "caller is responsible"
here really means: only call this if you previously called
xfs_can_free_eofblocks and it return true.

Sorry about that; I'll spell them out in the future.

quoted

Which brings me to the structure of this:  I think without much pain
we can ensure xfs_can_free_eofblocks is always called with the iolock,
in which case we really should merge xfs_can_free_eofblocks and this
new helper to avoid the rather confusing fact that we have two similarly
named helper doing similiar but not the same thing.

I'll have a look into that tomorrow morning. :)

The only change that was necessary was moving the can_free_eofblocks
call in the blockgc code until after we've taken the IOLOCK.

quoted

quoted

 int
+xfs_has_eofblocks(
+	struct xfs_inode	*ip,
+	bool			*has)

I also think the calling convention can be simplified here.  If an
error occurs we obviously do not want to free the eofblocks.  So
instead of returning two calues we can just return a single bool.

Yeah, this area needs some simplification.  Will do.

I moved all the stuff in this function upwards into
xfs_can_free_eofblocks and it seems to work ok.

--D

--D