Re: [PATCH 36/39] xfs: move CIL ordering to the logvec chain

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

On Wed, May 19, 2021 at 10:13:14PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Adding a list_sort() call to the CIL push work while the xc_ctx_lock
> is held exclusively has resulted in fairly long lock hold times and
> that stops all front end transaction commits from making progress.
> 
> We can move the sorting out of the xc_ctx_lock if we can transfer
> the ordering information to the log vectors as they are detached
> from the log items and then we can sort the log vectors.  With these
> changes, we can move the list_sort() call to just before we call
> xlog_write() when we aren't holding any locks at all.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.h     |  1 +
>  fs/xfs/xfs_log_cil.c | 23 ++++++++++++++---------
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index b4ad0e37a0c5..93aaee7c276e 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -10,6 +10,7 @@ struct xfs_cil_ctx;
>  
>  struct xfs_log_vec {
>  	struct list_head	lv_list;	/* CIL lv chain ptrs */
> +	uint32_t		lv_order_id;	/* chain ordering info */
>  	int			lv_niovecs;	/* number of iovecs in lv */
>  	struct xfs_log_iovec	*lv_iovecp;	/* iovec array */
>  	struct xfs_log_item	*lv_item;	/* owner */
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 035f0a60040a..cfd3128399f6 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -791,10 +791,10 @@ xlog_cil_order_cmp(
>  	const struct list_head	*a,
>  	const struct list_head	*b)
>  {
> -	struct xfs_log_item	*l1 = container_of(a, struct xfs_log_item, li_cil);
> -	struct xfs_log_item	*l2 = container_of(b, struct xfs_log_item, li_cil);
> +	struct xfs_log_vec	*l1 = container_of(a, struct xfs_log_vec, lv_list);
> +	struct xfs_log_vec	*l2 = container_of(b, struct xfs_log_vec, lv_list);
>  
> -	return l1->li_order_id > l2->li_order_id;
> +	return l1->lv_order_id > l2->lv_order_id;
>  }
>  
>  /*
> @@ -911,24 +911,22 @@ xlog_cil_push_work(
>  
>  	xlog_cil_pcp_aggregate(cil, ctx);
>  
> -	list_sort(NULL, &ctx->log_items, xlog_cil_order_cmp);
>  	while (!list_empty(&ctx->log_items)) {
>  		struct xfs_log_item	*item;
>  
>  		item = list_first_entry(&ctx->log_items,
>  					struct xfs_log_item, li_cil);
>  		lv = item->li_lv;
> -		list_del_init(&item->li_cil);
> -		item->li_order_id = 0;
> -		item->li_lv = NULL;
> -
> +		lv->lv_order_id = item->li_order_id;
>  		num_iovecs += lv->lv_niovecs;
>  		/* we don't write ordered log vectors */
>  		if (lv->lv_buf_len != XFS_LOG_VEC_ORDERED)
>  			num_bytes += lv->lv_bytes;
>  
>  		list_add_tail(&lv->lv_list, &ctx->lv_chain);
> -
> +		list_del_init(&item->li_cil);
> +		item->li_order_id = 0;
> +		item->li_lv = NULL;
>  	}
>  
>  	/*
> @@ -961,6 +959,13 @@ xlog_cil_push_work(
>  	spin_unlock(&cil->xc_push_lock);
>  	up_write(&cil->xc_ctx_lock);
>  
> +	/*
> +	 * Sort the log vector chain before we add the transaction headers.
> +	 * This ensures we always have the transaction headers at the start
> +	 * of the chain.
> +	 */
> +	list_sort(NULL, &ctx->lv_chain, xlog_cil_order_cmp);
> +
>  	/*
>  	 * Build a checkpoint transaction header and write it to the log to
>  	 * begin the transaction. We need to account for the space used by the
> -- 
> 2.31.1
>