Re: [PATCH 1/3] xfs: refactor remote attr value buffer invalidation

[Christoph Hellwig <hch@infradead.org>]

The refactor in the subject is very misleading.  You are not refactoring
code, but fixing a bug.

> -			error = xfs_trans_read_buf(mp, args->trans,
> +			error = xfs_trans_read_buf(mp, NULL,
>  						   mp->m_ddev_targp,
>  						   dblkno, dblkcnt, 0, &bp,
>  						   &xfs_attr3_rmt_buf_ops);

xfs_trans_read_buf with an always NULL tp is a strange interface.  Any
reason not to use xfs_buf_read directly?

> +/* Mark stale any buffers for the remote value. */
> +void
> +xfs_attr_rmtval_stale(
> +	struct xfs_inode	*ip,
> +	struct xfs_bmbt_irec	*map)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_buf		*bp;
> +	xfs_daddr_t		dblkno;
> +	int			dblkcnt;
> +
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +	if (map->br_startblock == HOLESTARTBLOCK)
> +		return;
> +
> +	dblkno = XFS_FSB_TO_DADDR(mp, map->br_startblock),
> +	dblkcnt = XFS_FSB_TO_BB(mp, map->br_blockcount);

Now this helper seems like a real refactoring in that it splits out a
common helper.  It matches one o the call sites exactly, while the
other has a major change, so I think it shouldn't just be one extra
patch, but instead of two extra patche to clearly document the changes.

> -		/*
> -		 * If it's a hole, these are already unmapped
> -		 * so there's nothing to invalidate.
> -		 */
> -		if (map.br_startblock != HOLESTARTBLOCK) {

Isn't this something we should keep in the caller?  That way the actual
invalide helper can assert that the map contains neither a hole or
a delaystartblock.

> -			bp = xfs_trans_get_buf(*trans,
> -					dp->i_mount->m_ddev_targp,
> -					dblkno, dblkcnt, 0);
> -			if (!bp)
> -				return -ENOMEM;
> -			xfs_trans_binval(*trans, bp);

And this is a pretty big change in that we now trylock and never read
a buffer from disk if it isn't in core.  That change looks fine to me
from trying to understand what is going on, but it clearly needs to
be split out and documented.

> -			/*
> -			 * Roll to next transaction.
> -			 */
> -			error = xfs_trans_roll_inode(trans, dp);
> -			if (error)
> -				return error;
> -		}
> +		xfs_attr_rmtval_stale(dp, &map);
>  
>  		tblkno += map.br_blockcount;
>  		tblkcnt -= map.br_blockcount;
>  	}
>  
> -	return 0;
> +	return xfs_trans_roll_inode(trans, dp);

xfs_attr3_leaf_freextent not doesn't do anything with the trans but
rolling it.  I think you can drop both the roll and the trans argument.