Re: [PATCH v2 15/15] xfs: bypass final dfops roll in trans commit path

[Bill O'Donnell <billodo@redhat.com>]

On Mon, Jul 23, 2018 at 09:04:14AM -0400, Brian Foster wrote:
> Once xfs_defer_finish() has completed all deferred operations, it
> checks the dirty state of the transaction and rolls it once more to
> return a clean transaction for the caller. This primarily to cover
> the case where repeated xfs_defer_finish() calls are made in a loop
> and we need to make sure that the caller starts the next iteration
> with a clean transaction. Otherwise we risk transaction reservation
> overrun.
> 
> This final transaction roll is not required in the transaction
> commit path, however, because the transaction is immediately
> committed and freed after dfops completion. Refactor the final roll
> into a separate helper such that we can avoid it in the transaction
> commit path.  Lift the dfops reset as well so dfops remains valid
> until after the last call to xfs_defer_trans_roll(). The reset is
> also unnecessary in the transaction commit path because the
> transaction is about to complete.
> 
> This eliminates unnecessary regrants of transactions where the
> associated transaction roll can be replaced by a transaction commit.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

The extra roll is a little confusing, but by inspection, this looks good.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_defer.c | 35 ++++++++++++++++++++++-------------
>  fs/xfs/libxfs/xfs_defer.h |  1 +
>  fs/xfs/xfs_trans.c        |  2 +-
>  3 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index cbee0a86c978..2d4c4b09977e 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -341,7 +341,7 @@ xfs_defer_reset(
>   * If an inode is provided, relog it to the new transaction.
>   */
>  int
> -xfs_defer_finish(
> +xfs_defer_finish_noroll(
>  	struct xfs_trans		**tp)
>  {
>  	struct xfs_defer_ops		*dop = (*tp)->t_dfops;
> @@ -430,21 +430,30 @@ xfs_defer_finish(
>  			cleanup_fn(*tp, state, error);
>  	}
>  
> -	/*
> -	 * Roll the transaction once more to avoid returning to the caller
> -	 * with a dirty transaction.
> -	 */
> -	if ((*tp)->t_flags & XFS_TRANS_DIRTY) {
> -		error = xfs_defer_trans_roll(tp);
> -		dop = (*tp)->t_dfops;
> -	}
>  out:
> -	if (error) {
> +	if (error)
>  		trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
> -	} else {
> +	 else
>  		trace_xfs_defer_finish_done((*tp)->t_mountp, dop, _RET_IP_);
> -		xfs_defer_reset(dop);
> -	}
> +
> +	return error;
> +}
> +
> +int
> +xfs_defer_finish(
> +	struct xfs_trans	**tp)
> +{
> +	int			error;
> +
> +	/*
> +	 * Finish and roll the transaction once more to avoid returning to the
> +	 * caller with a dirty transaction.
> +	 */
> +	error = xfs_defer_finish_noroll(tp);
> +	if (!error && ((*tp)->t_flags & XFS_TRANS_DIRTY))
> +		error = xfs_defer_trans_roll(tp);
> +	if (!error)
> +		xfs_defer_reset((*tp)->t_dfops);
>  
>  	return error;
>  }
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 56f927803940..85c41fe4dbae 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -48,6 +48,7 @@ enum xfs_defer_ops_type {
>  
>  void xfs_defer_add(struct xfs_defer_ops *dop, enum xfs_defer_ops_type type,
>  		struct list_head *h);
> +int xfs_defer_finish_noroll(struct xfs_trans **tp);
>  int xfs_defer_finish(struct xfs_trans **tp);
>  void __xfs_defer_cancel(struct xfs_defer_ops *dop);
>  void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index cd553aa9ecb0..7bf5c1202719 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -933,7 +933,7 @@ __xfs_trans_commit(
>  
>  	/* finish deferred items on final commit */
>  	if (!regrant && tp->t_dfops) {
> -		error = xfs_defer_finish(&tp);
> +		error = xfs_defer_finish_noroll(&tp);
>  		if (error) {
>  			xfs_defer_cancel(tp);
>  			goto out_unreserve;
> -- 
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html