On Tue, Sep 05, 2023 at 08:41:44AM -0400, Brian Foster wrote: > 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..
Yes, you made the code in __bch2_journal_buf_put() inline. Since that's the slowpath, we don't want it inline... > > Brian > > > journal_state_buf_put() is fine though, happy to take that part of the > > patch. > > > > > > > > Signed-off-by: Brian Foster <[email protected]> > > > --- > > > 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 > > > > > >
