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

[Christoph Hellwig <hch@infradead.org>]

On Thu, Mar 18, 2021 at 03:33:37PM -0700, Darrick J. Wong wrote:
> 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 |  148 +++++++++++++++++++++++++-----------------------
>  1 file changed, 78 insertions(+), 70 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index e7d68318e6a5..d4ceba5370c7 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -597,8 +597,17 @@ 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;

Should we have an assert here that this is called under the iolock?
Or can't the reclaim be expressed nicely?

> +/*
> + * 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.
> + */

Same thing here, usually asserts are better than comments..  That being
said can_free_eofblocks would benefit from at least a comment if the
assert doesn't work.

Otherwise this looks good.