Re: [PATCH 2/3] bcachefs: prepare journal buf put to handle pin put

[Brian Foster <bfoster@redhat.com>]

On Sun, Sep 03, 2023 at 05:09:04PM -0400, Kent Overstreet wrote:
> On Thu, Aug 31, 2023 at 07:07:33AM -0400, Brian Foster wrote:
> > bcachefs freeze testing has uncovered some raciness between journal
> > entry open/close and pin list reference count management. The
> > details of the problem are described in a separate patch. In
> > preparation for the associated fix, refactor the journal buffer put
> > path a bit to allow it to eventually handle dropping the pin list
> > reference currently held by an open journal entry. Open code the
> > journal write dispatch since it is essentially a single line and
> > instead factor out the journal state update to be reused, consistent
> > with other journal state update helpers. No functional changes in
> > this patch.
> 
> So, I don't want to take this patch as is because even though
> __bch2_journal_buf_put() is small, you're putting it into an inline
> fastpath that we want to be as small and fast as possible - the main
> transaction commit path.
> 

Can you elaborate on what you mean here? ISTM all this patch does is
replace a function call to __bch2_journal_buf_put() that already
exists..

Brian

> journal_state_buf_put() is fine though, happy to take that part of the
> patch.
> 
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/bcachefs/journal.c |  9 ---------
> >  fs/bcachefs/journal.h | 22 +++++++++++++++++-----
> >  2 files changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
> > index 055920c26da6..76ebcdb3e3b4 100644
> > --- a/fs/bcachefs/journal.c
> > +++ b/fs/bcachefs/journal.c
> > @@ -132,15 +132,6 @@ journal_error_check_stuck(struct journal *j, int error, unsigned flags)
> >  	return stuck;
> >  }
> >  
> > -/* journal entry close/open: */
> > -
> > -void __bch2_journal_buf_put(struct journal *j)
> > -{
> > -	struct bch_fs *c = container_of(j, struct bch_fs, journal);
> > -
> > -	closure_call(&j->io, bch2_journal_write, c->io_complete_wq, NULL);
> > -}
> > -
> >  /*
> >   * Returns true if journal entry is now closed:
> >   *
> > diff --git a/fs/bcachefs/journal.h b/fs/bcachefs/journal.h
> > index 008a2e25a4fa..c7a31da077c9 100644
> > --- a/fs/bcachefs/journal.h
> > +++ b/fs/bcachefs/journal.h
> > @@ -252,9 +252,10 @@ static inline bool journal_entry_empty(struct jset *j)
> >  	return true;
> >  }
> >  
> > -void __bch2_journal_buf_put(struct journal *);
> > -
> > -static inline void bch2_journal_buf_put(struct journal *j, unsigned idx)
> > +/*
> > + * Drop reference on a buffer index and return true if the count has hit zero.
> > + */
> > +static inline union journal_res_state journal_state_buf_put(struct journal *j, unsigned idx)
> >  {
> >  	union journal_res_state s;
> >  
> > @@ -264,9 +265,20 @@ static inline void bch2_journal_buf_put(struct journal *j, unsigned idx)
> >  				    .buf2_count = idx == 2,
> >  				    .buf3_count = idx == 3,
> >  				    }).v, &j->reservations.counter);
> > +	return s;
> > +}
> >  
> > -	if (!journal_state_count(s, idx) && idx == s.unwritten_idx)
> > -		__bch2_journal_buf_put(j);
> > +void bch2_journal_write(struct closure *);
> > +static inline void bch2_journal_buf_put(struct journal *j, unsigned idx)
> > +{
> > +	struct bch_fs *c = container_of(j, struct bch_fs, journal);
> > +	union journal_res_state s;
> > +
> > +	s = journal_state_buf_put(j, idx);
> > +	if (!journal_state_count(s, idx)) {
> > +		if (idx == s.unwritten_idx)
> > +			closure_call(&j->io, bch2_journal_write, c->io_complete_wq, NULL);
> > +	}
> >  }
> >  
> >  /*
> > -- 
> > 2.41.0
> > 
>