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

Christoph Hellwig
2021-01-13T14:59:05Z

On Mon, Jan 11, 2021 at 03:23:28PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> 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;