Re: [PATCH 2/7] xfs: refactor the predicate part of xfs_free_eofblocks

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

On Mon, Jan 11, 2021 at 03:23:28PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
> 
> Refactor the part of _free_eofblocks that decides if it's really going
> to truncate post-EOF blocks into a separate helper function.  The
> upcoming deferred inode inactivation patch requires us to be able to
> decide this prior to actual inactivation.  No functionality changes.

Is there any specific reason why the new xfs_has_eofblocks helper is in
xfs_inode.c?  That just makes following the logic a little harder.

> +
> +/*
> + * Decide if this inode have post-EOF blocks.  The caller is responsible
> + * for knowing / caring about the PREALLOC/APPEND flags.
> + */
> +int
> +xfs_has_eofblocks(
> +	struct xfs_inode	*ip,
> +	bool			*has)
> +{
> +	struct xfs_bmbt_irec	imap;
> +	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_fileoff_t		end_fsb;
> +	xfs_fileoff_t		last_fsb;
> +	xfs_filblks_t		map_len;
> +	int			nimaps;
> +	int			error;
> +
> +	*has = false;
> +	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;

Where does this strange magic come from?

> +	map_len = last_fsb - end_fsb;
> +
> +	nimaps = 1;
> +	xfs_ilock(ip, XFS_ILOCK_SHARED);
> +	error = xfs_bmapi_read(ip, end_fsb, map_len, &imap, &nimaps, 0);
> +	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +
> +	if (error || nimaps == 0)
> +		return error;
> +
> +	*has = imap.br_startblock != HOLESTARTBLOCK || ip->i_delayed_blks;
> +	return 0;

I think this logic could be simplified at lot by using xfs_iext_lookup
directly. Something like:

	*has = false;

	xfs_ilock(ip, XFS_ILOCK_SHARED);
	if (ip->i_delayed_blks) {
		*has = true;
		goto out_unlock;
	}
	if (!(ip->i_df.if_flags & XFS_IFEXTENTS)) {
		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
		if (error)
			goto out_unlock;
        }
	if (xfs_iext_lookup_extent(ip, &ip->i_df, end_fsb, &icur, &imap))
		*has = true;
out_unlock:
	xfs_iunlock(ip, XFS_ILOCK_SHARED);
	return error;