[Linux-cachefs] [PATCH v13 3/4] xfs: refactor the usage around xfs_trans_context_{set, clear}

[Dave Chinner]

On Thu, Dec 17, 2020 at 09:11:56AM +0800, Yafang Shao wrote:
> The xfs_trans context should be active after it is allocated, and
> deactive when it is freed.
> The rolling transaction should be specially considered, because in the
> case when we clear the old transaction the thread's NOFS state shouldn't
> be changed, as a result we have to set NOFS in the old transaction's
> t_pflags in xfs_trans_context_swap().
> 
> So these helpers are refactored as,
> - xfs_trans_context_set()
>   Used in xfs_trans_alloc()
> - xfs_trans_context_clear()
>   Used in xfs_trans_free()
> 
> And a new helper is instroduced to handle the rolling transaction,
> - xfs_trans_context_swap()
>   Used in rolling transaction
> 
> This patch is based on Darrick's work to fix the issue in xfs/141 in the
> earlier version. [1]
> 
> 1. https://lore.kernel.org/linux-xfs/20201104001649.GN7123@magnolia

As I said in my last comments, this change of logic is not
necessary.  All we need to do is transfer the NOFS state to the new
transactions and *remove it from the old one*.

IOWs, all this patch should do is:

> @@ -119,7 +123,9 @@ xfs_trans_dup(
>  
>  	ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
>  	tp->t_rtx_res = tp->t_rtx_res_used;
> -	ntp->t_pflags = tp->t_pflags;
> +
> +	/* Associate the new transaction with this thread. */
> +	xfs_trans_context_swap(tp, ntp);
>  
>  	/* move deferred ops over to the new tp */
>  	xfs_defer_move(ntp, tp);

This, and

> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 44b11c64a15e..12380eaaf7ce 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -280,4 +280,17 @@ xfs_trans_context_clear(struct xfs_trans *tp)
>  	memalloc_nofs_restore(tp->t_pflags);
>  }
>  
> +static inline void
> +xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp)
> +{

introduce this wrapper.

> +	ntp->t_pflags = tp->t_pflags;
> +	/*
> +	 * For the rolling transaction, we have to set NOFS in the old
> +	 * transaction's t_pflags so that when we clear the context on
> +	 * the old transaction we don't actually change the thread's NOFS
> +	 * state.
> +	 */
> +	tp->t_pflags = current->flags | PF_MEMALLOC_NOFS;
> +}

But not with this implementation.

Think for a minute, please. All we want to do is avoid clearing
the nofs state when we call xfs_trans_context_clear(tp) if the state
has been handed to another transaction.

Your current implementation hands the state to ntp, but *then leaves
it on tp* as well. So then you hack a PF_MEMALLOC_NOFS flag into
tp->t_pflags so that it doesn't clear that flag (abusing the masking
done during clearing). That's just nasty because it relies on
internal memalloc_nofs_restore() details for correct functionality.

The obvious solution: we've moved the saved process state to a
different context, so it is no longer needed for the current
transaction we are about to commit. So How about just clearing the
saved state from the original transaction when swappingi like so:

static inline void
xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp)
{
	ntp->t_pflags = tp->t_pflags;
	tp->t_flags = 0;
}

And now, when we go to clear the transaction context, we can simply
do this:

static inline void
xfs_trans_context_clear(struct xfs_trans *tp)
{
	if (tp->t_pflags)
		memalloc_nofs_restore(tp->t_pflags);
}

and the problem is solved. The NOFS state will follow the active
transaction and not be reset until the entire transaction chain is
completed.

In the next patch you can go and introduce current->journal_info
into just the wrapper functions, maintaining the same overall
logic.

-Dave.
-- 
Dave Chinner
david at fromorbit.com