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

[Dave Chinner <david@fromorbit.com>]

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.
-- 
Dave Chinner
david@fromorbit.com