diff mbox series

[37/45] xfs: track CIL ticket reservation in percpu structure

Message ID 20210305051143.182133-38-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>

To get it out from under the cil spinlock.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c  | 11 ++++++-----
 fs/xfs/xfs_log_priv.h |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

Comments

Darrick J. Wong March 11, 2021, 12:26 a.m. UTC | #1
On Fri, Mar 05, 2021 at 04:11:35PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To get it out from under the cil spinlock.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_cil.c  | 11 ++++++-----
>  fs/xfs/xfs_log_priv.h |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 5519d112c1fd..a2f93bd7644b 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -492,6 +492,7 @@ xlog_cil_insert_items(
>  	 * based on how close we are to the hard limit.
>  	 */
>  	cilpcp = get_cpu_ptr(cil->xc_pcp);
> +	cilpcp->space_reserved += ctx_res;
>  	cilpcp->space_used += len;
>  	if (space_used >= XLOG_CIL_SPACE_LIMIT(log) ||
>  	    cilpcp->space_used >
> @@ -502,10 +503,6 @@ xlog_cil_insert_items(
>  	}
>  	put_cpu_ptr(cilpcp);
>  
> -	spin_lock(&cil->xc_cil_lock);
> -	ctx->ticket->t_unit_res += ctx_res;
> -	ctx->ticket->t_curr_res += ctx_res;
> -
>  	/*
>  	 * If we've overrun the reservation, dump the tx details before we move
>  	 * the log items. Shutdown is imminent...
> @@ -527,6 +524,7 @@ xlog_cil_insert_items(
>  	 * We do this here so we only need to take the CIL lock once during
>  	 * the transaction commit.
>  	 */
> +	spin_lock(&cil->xc_cil_lock);
>  	list_for_each_entry(lip, &tp->t_items, li_trans) {
>  
>  		/* Skip items which aren't dirty in this transaction. */
> @@ -798,10 +796,13 @@ xlog_cil_push_work(
>  
>  	down_write(&cil->xc_ctx_lock);
>  
> -	/* Reset the CIL pcp counters */
> +	/* Aggregate and reset the CIL pcp counters */
>  	for_each_online_cpu(cpu) {
>  		cilpcp = per_cpu_ptr(cil->xc_pcp, cpu);
> +		ctx->ticket->t_curr_res += cilpcp->space_reserved;

Why isn't it necessary to update ctx->ticket->t_unit_res any more?

(Admittedly I'm struggling to figure out why it matters to keep it
updated even in the current code base...)

--D

>  		cilpcp->space_used = 0;
> +		cilpcp->space_reserved = 0;
> +
>  	}
>  
>  	spin_lock(&cil->xc_push_lock);
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 4eb373357f26..278b9eaea582 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -236,7 +236,7 @@ struct xfs_cil_ctx {
>   */
>  struct xlog_cil_pcp {
>  	uint32_t		space_used;
> -	uint32_t		curr_res;
> +	uint32_t		space_reserved;
>  	struct list_head	busy_extents;
>  	struct list_head	log_items;
>  };
> -- 
> 2.28.0
>
Dave Chinner March 12, 2021, 12:47 a.m. UTC | #2
On Wed, Mar 10, 2021 at 04:26:10PM -0800, Darrick J. Wong wrote:
> On Fri, Mar 05, 2021 at 04:11:35PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > To get it out from under the cil spinlock.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log_cil.c  | 11 ++++++-----
> >  fs/xfs/xfs_log_priv.h |  2 +-
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index 5519d112c1fd..a2f93bd7644b 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -492,6 +492,7 @@ xlog_cil_insert_items(
> >  	 * based on how close we are to the hard limit.
> >  	 */
> >  	cilpcp = get_cpu_ptr(cil->xc_pcp);
> > +	cilpcp->space_reserved += ctx_res;
> >  	cilpcp->space_used += len;
> >  	if (space_used >= XLOG_CIL_SPACE_LIMIT(log) ||
> >  	    cilpcp->space_used >
> > @@ -502,10 +503,6 @@ xlog_cil_insert_items(
> >  	}
> >  	put_cpu_ptr(cilpcp);
> >  
> > -	spin_lock(&cil->xc_cil_lock);
> > -	ctx->ticket->t_unit_res += ctx_res;
> > -	ctx->ticket->t_curr_res += ctx_res;
> > -
> >  	/*
> >  	 * If we've overrun the reservation, dump the tx details before we move
> >  	 * the log items. Shutdown is imminent...
> > @@ -527,6 +524,7 @@ xlog_cil_insert_items(
> >  	 * We do this here so we only need to take the CIL lock once during
> >  	 * the transaction commit.
> >  	 */
> > +	spin_lock(&cil->xc_cil_lock);
> >  	list_for_each_entry(lip, &tp->t_items, li_trans) {
> >  
> >  		/* Skip items which aren't dirty in this transaction. */
> > @@ -798,10 +796,13 @@ xlog_cil_push_work(
> >  
> >  	down_write(&cil->xc_ctx_lock);
> >  
> > -	/* Reset the CIL pcp counters */
> > +	/* Aggregate and reset the CIL pcp counters */
> >  	for_each_online_cpu(cpu) {
> >  		cilpcp = per_cpu_ptr(cil->xc_pcp, cpu);
> > +		ctx->ticket->t_curr_res += cilpcp->space_reserved;
> 
> Why isn't it necessary to update ctx->ticket->t_unit_res any more?

Because t_unit_res is never used by the CIL ticket becuse they
aren't permanent transaction reservations. The unit res is only
for granting new space to a ticket, yet the CIL only ever "steals"
granted space from an existing ticket. When
the ticket is dropped, we return unused reservations from the
CIL ticket, but never touch or look at the unit reservation.

I can add it back in here if you want, but it's largely dead code...

> (Admittedly I'm struggling to figure out why it matters to keep it
> updated even in the current code base...)

I think I originally did it a decade ago because I probably wasn't
100% sure on what impact not setting it would have. Getting the rest
of the delayed logging code right was far more important than
sweating on a tiny, largely insignificant detail like this.

Cheers,

Dave.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 5519d112c1fd..a2f93bd7644b 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -492,6 +492,7 @@  xlog_cil_insert_items(
 	 * based on how close we are to the hard limit.
 	 */
 	cilpcp = get_cpu_ptr(cil->xc_pcp);
+	cilpcp->space_reserved += ctx_res;
 	cilpcp->space_used += len;
 	if (space_used >= XLOG_CIL_SPACE_LIMIT(log) ||
 	    cilpcp->space_used >
@@ -502,10 +503,6 @@  xlog_cil_insert_items(
 	}
 	put_cpu_ptr(cilpcp);
 
-	spin_lock(&cil->xc_cil_lock);
-	ctx->ticket->t_unit_res += ctx_res;
-	ctx->ticket->t_curr_res += ctx_res;
-
 	/*
 	 * If we've overrun the reservation, dump the tx details before we move
 	 * the log items. Shutdown is imminent...
@@ -527,6 +524,7 @@  xlog_cil_insert_items(
 	 * We do this here so we only need to take the CIL lock once during
 	 * the transaction commit.
 	 */
+	spin_lock(&cil->xc_cil_lock);
 	list_for_each_entry(lip, &tp->t_items, li_trans) {
 
 		/* Skip items which aren't dirty in this transaction. */
@@ -798,10 +796,13 @@  xlog_cil_push_work(
 
 	down_write(&cil->xc_ctx_lock);
 
-	/* Reset the CIL pcp counters */
+	/* Aggregate and reset the CIL pcp counters */
 	for_each_online_cpu(cpu) {
 		cilpcp = per_cpu_ptr(cil->xc_pcp, cpu);
+		ctx->ticket->t_curr_res += cilpcp->space_reserved;
 		cilpcp->space_used = 0;
+		cilpcp->space_reserved = 0;
+
 	}
 
 	spin_lock(&cil->xc_push_lock);
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 4eb373357f26..278b9eaea582 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -236,7 +236,7 @@  struct xfs_cil_ctx {
  */
 struct xlog_cil_pcp {
 	uint32_t		space_used;
-	uint32_t		curr_res;
+	uint32_t		space_reserved;
 	struct list_head	busy_extents;
 	struct list_head	log_items;
 };