Re: [PATCH 38/45] xfs: convert CIL busy extents to per-cpu

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Mar 05, 2021 at 04:11:36PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To get them out from under the CIL lock.
> 
> This is an unordered list, so we can simply punt it to per-cpu lists
> during transaction commits and reaggregate it back into a single
> list during the CIL push work.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_cil.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index a2f93bd7644b..7428b98c8279 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -501,6 +501,9 @@ xlog_cil_insert_items(
>  		atomic_add(cilpcp->space_used, &ctx->space_used);
>  		cilpcp->space_used = 0;
>  	}
> +	/* attach the transaction to the CIL if it has any busy extents */
> +	if (!list_empty(&tp->t_busy))
> +		list_splice_init(&tp->t_busy, &cilpcp->busy_extents);
>  	put_cpu_ptr(cilpcp);
>  
>  	/*
> @@ -540,9 +543,6 @@ xlog_cil_insert_items(
>  			list_move_tail(&lip->li_cil, &cil->xc_cil);
>  	}
>  
> -	/* attach the transaction to the CIL if it has any busy extents */
> -	if (!list_empty(&tp->t_busy))
> -		list_splice_init(&tp->t_busy, &ctx->busy_extents);
>  	spin_unlock(&cil->xc_cil_lock);
>  
>  	if (tp->t_ticket->t_curr_res < 0)
> @@ -802,7 +802,10 @@ xlog_cil_push_work(
>  		ctx->ticket->t_curr_res += cilpcp->space_reserved;
>  		cilpcp->space_used = 0;
>  		cilpcp->space_reserved = 0;
> -
> +		if (!list_empty(&cilpcp->busy_extents)) {
> +			list_splice_init(&cilpcp->busy_extents,
> +					&ctx->busy_extents);
> +		}
>  	}
>  
>  	spin_lock(&cil->xc_push_lock);
> @@ -1459,17 +1462,24 @@ static void __percpu *
>  xlog_cil_pcp_alloc(
>  	struct xfs_cil		*cil)
>  {
> +	void __percpu		*pcptr;
>  	struct xlog_cil_pcp	*cilpcp;
> +	int			cpu;
>  
> -	cilpcp = alloc_percpu(struct xlog_cil_pcp);
> -	if (!cilpcp)
> +	pcptr = alloc_percpu(struct xlog_cil_pcp);
> +	if (!pcptr)
>  		return NULL;
>  
> +	for_each_possible_cpu(cpu) {
> +		cilpcp = per_cpu_ptr(pcptr, cpu);

So... in my mind, "cilpcp" and "pcptr" aren't really all that distinct
from each other.  I /think/ you're trying to use "cilpcp" everywhere
else to mean "pointer to a particular CPU's CIL data", and this change
makes that usage consistent in the alloc function.

However, this leaves xlog_cil_pcp_free using "cilpcp" to refer to the
entire chunk of per-CPU data structures.  Given that the first refers to
a specific structure and the second refers to them all in aggregate,
maybe _pcp_alloc and _pcp_free should use a name that at least sounds
plural?

e.g.

	void __percpu	*all_cilpcps = alloc_percpu(...);

	for_each_possible_cpu(cpu) {
		cilpcp = per_cpu_ptr(all_cilpcps, cpu);
		cilpcp->magicval = 7777;
	}

and

	cil->xc_all_pcps = xlog_cil_pcp_alloc(...);

Hm?

--D

> +		INIT_LIST_HEAD(&cilpcp->busy_extents);
> +	}
> +
>  	if (xlog_cil_pcp_hpadd(cil) < 0) {
> -		free_percpu(cilpcp);
> +		free_percpu(pcptr);
>  		return NULL;
>  	}
> -	return cilpcp;
> +	return pcptr;
>  }
>  
>  static void
> -- 
> 2.28.0
>