Re: [PATCH v9 13/24] xfs: Remove unneeded xfs_trans_roll_inode calls

[Brian Foster <bfoster@redhat.com>]

On Thu, Apr 30, 2020 at 03:50:05PM -0700, Allison Collins wrote:
> Some calls to xfs_trans_roll_inode and xfs_defer_finish routines are not
> needed. If they are the last operations executed in these functions, and
> no further changes are made, then higher level routines will roll or
> commit the tranactions. The xfs_trans_roll in _removename is also not
> needed because invalidating blocks is an incore-only change.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 40 ----------------------------------------
>  1 file changed, 40 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index d83443c..af47566 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
...
> @@ -784,9 +770,6 @@ xfs_attr_leaf_removename(
>  		/* bp is gone due to xfs_da_shrink_inode */
>  		if (error)
>  			return error;
> -		error = xfs_defer_finish(&args->trans);
> -		if (error)
> -			return error;
>  	}
>  	return 0;

Looks like this could be simplified to return error at the end of the
function. Some of the others might have similar simplifications
available.

>  }
> @@ -1074,13 +1057,6 @@ xfs_attr_node_addname(
>  				goto out;
>  		}
>  
> -		/*
> -		 * Commit and start the next trans in the chain.
> -		 */
> -		error = xfs_trans_roll_inode(&args->trans, dp);
> -		if (error)
> -			goto out;
> -

There's an xfs_defer_finish() before this roll. Is that not also
extraneous? It looks like we return straight up to xfs_attr_set() (trans
commit) from here..

>  	} else if (args->rmtblkno > 0) {
>  		/*
>  		 * Added a "remote" value, just clear the incomplete flag.
...
> @@ -1194,10 +1158,6 @@ xfs_attr_node_removename(
>  		if (error)
>  			goto out;
>  
> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
> -		if (error)
> -			goto out;
> -

I'm still a bit on the fence about this one. At first glance it looks
like it's not necessary, but technically we're changing behavior by
combining setting the inactive flag and the first remote block unmap,
right? If so, that seems fairly reasonable, but it's hard to say for
sure with the state of the xattr transaction reservations...

Looking around some more, I suppose this could be analogous to the
!remote remove case where an entry is removed and a potential dabtree
join occurs under the same transaction. If that's the best argument we
have, however, I might suggest to split this one out into an independent
patch and let the commit log describe what's going on in more detail.
That way it's more obvious to reviewers and if it's wrong it's easier to
revert.

Brian

>  		error = xfs_attr_rmtval_invalidate(args);
>  		if (error)
>  			return error;
> -- 
> 2.7.4
>