diff mbox series

[33/45] xfs: lift init CIL reservation out of xc_cil_lock

Message ID 20210305051143.182133-34-david@fromorbit.com (mailing list archive)
State New
Headers show
Series xfs: consolidated log and optimisation changes | expand

Commit Message

Dave Chinner March 5, 2021, 5:11 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

The xc_cil_lock is the most highly contended lock in XFS now. To
start the process of getting rid of it, lift the initial reservation
of the CIL log space out from under the xc_cil_lock.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

Comments

Darrick J. Wong March 10, 2021, 11:25 p.m. UTC | #1
On Fri, Mar 05, 2021 at 04:11:31PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xc_cil_lock is the most highly contended lock in XFS now. To
> start the process of getting rid of it, lift the initial reservation
> of the CIL log space out from under the xc_cil_lock.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_cil.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index e6e36488f0c7..50101336a7f4 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -430,23 +430,19 @@ xlog_cil_insert_items(
>  	 */
>  	xlog_cil_insert_format_items(log, tp, &len);
>  
> -	spin_lock(&cil->xc_cil_lock);

Hm, so looking ahead, the next few patches keep kicking this spin_lock
call further and further down in the file, and the commit messages give
me the impression that this might even go away entirely?

Let me see, the CIL locks are:

xc_ctx_lock, which prevents transactions from committing (into the cil)
any time the CIL itself is preparing a new commited item context so that
it can xlog_write (to disk) the log vectors associated with the current
context.

xc_cil_lock, which serializes transactions adding their items to the CIL
in the first place, hence the motivation to reduce this hot lock?

xc_push_lock, which I think is used to coordinate the CIL push worker
with all the upper level callers that want to force log items to disk?

And the locking order of these three locks is...

xc_ctx_lock --> xc_push_lock
    |
    \---------> xc_cil_lock

Assuming I grokked all that, then I guess moving the spin_lock call
works out because the test_and_clear_bit is atomic.  The rest of the
accounting stuff here is just getting moved further down in the file and
is still protected by xc_cil_lock.

If I understood all that,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> -
> -	/* 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);
> -
>  	/*
>  	 * We need to take the CIL checkpoint unit reservation on the first
>  	 * commit into the CIL. Test the XLOG_CIL_EMPTY bit first so we don't
> -	 * unnecessarily do an atomic op in the fast path here.
> +	 * unnecessarily do an atomic op in the fast path here. We don't need to
> +	 * hold the xc_cil_lock here to clear the XLOG_CIL_EMPTY bit as we are
> +	 * under the xc_ctx_lock here and that needs to be held exclusively to
> +	 * reset the XLOG_CIL_EMPTY bit.
>  	 */
>  	if (test_bit(XLOG_CIL_EMPTY, &cil->xc_flags) &&
> -	    test_and_clear_bit(XLOG_CIL_EMPTY, &cil->xc_flags)) {
> +	    test_and_clear_bit(XLOG_CIL_EMPTY, &cil->xc_flags))
>  		ctx_res = ctx->ticket->t_unit_res;
> -		ctx->ticket->t_curr_res = ctx_res;
> -		tp->t_ticket->t_curr_res -= ctx_res;
> -	}
> +
> +	spin_lock(&cil->xc_cil_lock);
>  
>  	/* do we need space for more log record headers? */
>  	iclog_space = log->l_iclog_size - log->l_iclog_hsize;
> @@ -456,11 +452,9 @@ xlog_cil_insert_items(
>  		/* need to take into account split region headers, too */
>  		split_res *= log->l_iclog_hsize + sizeof(struct xlog_op_header);
>  		ctx->ticket->t_unit_res += split_res;
> -		ctx->ticket->t_curr_res += split_res;
> -		tp->t_ticket->t_curr_res -= split_res;
> -		ASSERT(tp->t_ticket->t_curr_res >= len);
>  	}
> -	tp->t_ticket->t_curr_res -= len;
> +	tp->t_ticket->t_curr_res -= split_res + ctx_res + len;
> +	ctx->ticket->t_curr_res += split_res + ctx_res;
>  	ctx->space_used += len;
>  
>  	/*
> @@ -498,6 +492,9 @@ 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)
> -- 
> 2.28.0
>
Dave Chinner March 11, 2021, 5:42 a.m. UTC | #2
On Wed, Mar 10, 2021 at 03:25:41PM -0800, Darrick J. Wong wrote:
> On Fri, Mar 05, 2021 at 04:11:31PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The xc_cil_lock is the most highly contended lock in XFS now. To
> > start the process of getting rid of it, lift the initial reservation
> > of the CIL log space out from under the xc_cil_lock.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log_cil.c | 27 ++++++++++++---------------
> >  1 file changed, 12 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index e6e36488f0c7..50101336a7f4 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -430,23 +430,19 @@ xlog_cil_insert_items(
> >  	 */
> >  	xlog_cil_insert_format_items(log, tp, &len);
> >  
> > -	spin_lock(&cil->xc_cil_lock);
> 
> Hm, so looking ahead, the next few patches keep kicking this spin_lock
> call further and further down in the file, and the commit messages give
> me the impression that this might even go away entirely?
> 
> Let me see, the CIL locks are:
> 
> xc_ctx_lock, which prevents transactions from committing (into the cil)
> any time the CIL itself is preparing a new commited item context so that
> it can xlog_write (to disk) the log vectors associated with the current
> context.

Yes.

> xc_cil_lock, which serializes transactions adding their items to the CIL
> in the first place, hence the motivation to reduce this hot lock?

Right - it protects manipulations to the CIL log item tracking and
tracking state.  This spin lock is the first global serialisation
point in the transaction commit path, so it effectively sees unbound
concurrency (reservations allow hundreds of transactions can be
committing simultaneously).

> xc_push_lock, which I think is used to coordinate the CIL push worker
> with all the upper level callers that want to force log items to disk?

Yes, this one protects the current push state.

> And the locking order of these three locks is...
> 
> xc_ctx_lock --> xc_push_lock
>     |
>     \---------> xc_cil_lock
> 
> Assuming I grokked all that, then I guess moving the spin_lock call
> works out because the test_and_clear_bit is atomic.  The rest of the
> accounting stuff here is just getting moved further down in the file and
> is still protected by xc_cil_lock.

Yes.

> If I understood all that,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks!

-Dave.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index e6e36488f0c7..50101336a7f4 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -430,23 +430,19 @@  xlog_cil_insert_items(
 	 */
 	xlog_cil_insert_format_items(log, tp, &len);
 
-	spin_lock(&cil->xc_cil_lock);
-
-	/* 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);
-
 	/*
 	 * We need to take the CIL checkpoint unit reservation on the first
 	 * commit into the CIL. Test the XLOG_CIL_EMPTY bit first so we don't
-	 * unnecessarily do an atomic op in the fast path here.
+	 * unnecessarily do an atomic op in the fast path here. We don't need to
+	 * hold the xc_cil_lock here to clear the XLOG_CIL_EMPTY bit as we are
+	 * under the xc_ctx_lock here and that needs to be held exclusively to
+	 * reset the XLOG_CIL_EMPTY bit.
 	 */
 	if (test_bit(XLOG_CIL_EMPTY, &cil->xc_flags) &&
-	    test_and_clear_bit(XLOG_CIL_EMPTY, &cil->xc_flags)) {
+	    test_and_clear_bit(XLOG_CIL_EMPTY, &cil->xc_flags))
 		ctx_res = ctx->ticket->t_unit_res;
-		ctx->ticket->t_curr_res = ctx_res;
-		tp->t_ticket->t_curr_res -= ctx_res;
-	}
+
+	spin_lock(&cil->xc_cil_lock);
 
 	/* do we need space for more log record headers? */
 	iclog_space = log->l_iclog_size - log->l_iclog_hsize;
@@ -456,11 +452,9 @@  xlog_cil_insert_items(
 		/* need to take into account split region headers, too */
 		split_res *= log->l_iclog_hsize + sizeof(struct xlog_op_header);
 		ctx->ticket->t_unit_res += split_res;
-		ctx->ticket->t_curr_res += split_res;
-		tp->t_ticket->t_curr_res -= split_res;
-		ASSERT(tp->t_ticket->t_curr_res >= len);
 	}
-	tp->t_ticket->t_curr_res -= len;
+	tp->t_ticket->t_curr_res -= split_res + ctx_res + len;
+	ctx->ticket->t_curr_res += split_res + ctx_res;
 	ctx->space_used += len;
 
 	/*
@@ -498,6 +492,9 @@  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)