On Thu, Jul 17, 2025 at 05:32:20PM +0300, Dan Carpenter wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> master
> head: e8352908bdcd2d0bcf0aca8c69fae85fd5ea5edb
> commit: 0e1d28c378988f28b773807c00e7f3be40f49bf3 [10097/10441] bcachefs:
> convert journal_reclaim.c to CLASS/guards
> config: i386-randconfig-141-20250716
> (https://download.01.org/0day-ci/archive/20250717/[email protected]/config)
> compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Closes: https://lore.kernel.org/r/[email protected]/
>
> smatch warnings:
> fs/bcachefs/journal_reclaim.c:877 journal_flush_done() warn: missing error
> code? 'ret'
>
> vim +/ret +877 fs/bcachefs/journal_reclaim.c
>
> 039fc4c5221f74 Kent Overstreet 2020-05-28 861 static int
> journal_flush_done(struct journal *j, u64 seq_to_flush,
> 039fc4c5221f74 Kent Overstreet 2020-05-28 862
> bool *did_work)
> 1c6fdbd8f2465d Kent Overstreet 2017-03-16 863 {
> 35f5197009cae2 Kent Overstreet 2025-01-25 864 int ret = 0;
> 1c6fdbd8f2465d Kent Overstreet 2017-03-16 865
> 1c6fdbd8f2465d Kent Overstreet 2017-03-16 866 ret =
> bch2_journal_error(j);
> 1c6fdbd8f2465d Kent Overstreet 2017-03-16 867 if (ret)
> 1c6fdbd8f2465d Kent Overstreet 2017-03-16 868 return ret;
> 1c6fdbd8f2465d Kent Overstreet 2017-03-16 869
> 0e1d28c378988f Kent Overstreet 2025-07-14 870
> guard(mutex)(&j->reclaim_lock);
> 000de45996c4b0 Kent Overstreet 2019-01-14 871
> 1e690efa72596a Kent Overstreet 2025-02-10 872 for (int type =
> JOURNAL_PIN_TYPE_NR - 1;
> 1e690efa72596a Kent Overstreet 2025-02-10 873 type >= 0;
> 1e690efa72596a Kent Overstreet 2025-02-10 874 --type)
> 1e690efa72596a Kent Overstreet 2025-02-10 875 if
> (journal_flush_pins_or_still_flushing(j, seq_to_flush, BIT(type))) {
> 35f5197009cae2 Kent Overstreet 2025-01-25 876
> *did_work = true;
> 0e1d28c378988f Kent Overstreet 2025-07-14 @877 return
> ret;
>
> I think ret was intended to be the return value from
> journal_flush_pins_or_still_flushing()?
It is a bit unusual, but not a bug.
If journal_flush_pins_or_still_flushing() returns true, then the flush
hasn't complete and we must return 0; we want the outer
closure_wait_event() in journal_flush_pins() to continue.
The early return is there because we don't want to call
journal_entry_close() until we've finished flushing all outstanding
journal pins - otherwise seq_to_flush can be U64_MAX, and we'll close a
bunch of journal entries and write tiny ones completely unnecessarily.
Having the early return be in the loop where we loop over types is
important, because flushing one journal pin can cause new journal pins
to be added (even of the same type, btree node writes may generate more
btree node writes, when updating the parent pointer has a full node and
has to trigger a split/compact).
This is part of our shutdown sequence, where order of flushing is
important in order to make sure that it terminates...
Applying this...
diff --git a/fs/bcachefs/journal_reclaim.c b/fs/bcachefs/journal_reclaim.c
index be50455c7f13..f23e5ee9ad75 100644
--- a/fs/bcachefs/journal_reclaim.c
+++ b/fs/bcachefs/journal_reclaim.c
@@ -874,7 +874,34 @@ static int journal_flush_done(struct journal *j, u64
seq_to_flush,
--type)
if (journal_flush_pins_or_still_flushing(j, seq_to_flush,
BIT(type))) {
*did_work = true;
- return ret;
+
+ /*
+ * Question from Dan Carpenter, on the early return:
+ *
+ * If journal_flush_pins_or_still_flushing() returns
+ * true, then the flush hasn't complete and we must
+ * return 0; we want the outer closure_wait_event() in
+ * journal_flush_pins() to continue.
+ *
+ * The early return is there because we don't want to
+ * call journal_entry_close() until we've finished
+ * flushing all outstanding journal pins - otherwise
+ * seq_to_flush can be U64_MAX, and we'll close a bunch
+ * of journal entries and write tiny ones completely
+ * unnecessarily.
+ *
+ * Having the early return be in the loop where we loop
+ * over types is important, because flushing one journal
+ * pin can cause new journal pins to be added (even of
+ * the same type, btree node writes may generate more
+ * btree node writes, when updating the parent pointer
+ * has a full node and has to trigger a split/compact).
+ *
+ * This is part of our shutdown sequence, where order of
+ * flushing is important in order to make sure that it
+ * terminates...
+ */
+ return 0;
}
if (seq_to_flush > journal_cur_seq(j))
