Re: [PATCH 1/3] ext2fs: Add JBD2 on-disk layout headers

Hello,

Thanks for the iterations while testing :)

It took me time to manage to find several consecutive hours to be able
give a firm look before committing, which I have now done, congrats! :D

There is a couple things that I fixed on the fly: 

- We want to use pthread_cond_clockwait rather than
  pthread_cond_timedwait, to be able to use CLOCK_MONOTONIC instead of
  CLOCK_REALTIME, to avoid being hit by ntpdate and such.

- In diskfs_S_dir_rename, there was an addition of:

  pthread_mutex_unlock (&fnp->lock);

  which was clearly bogus: we were unlocking it again below.

There are a couple things that we'd want to fix now:

- when calling diskfs_file_update, don't we have to be inside a
  transaction? Otherwise if we pass wait=1 and use a journal, we won't
  be waiting AIUI? Notably, in diskfs_S_dir_rmdir we don't use a
  transaction. And ideally we'd have an assertion that makes sure we
  respect this.

- we should define some helper for this recurring pattern:

  if ((docommit) && (diskfs_synchronous || diskfs_journal_needs_sync (txn)))
    diskfs_journal_commit_transaction (txn);
  else
    diskfs_journal_stop_transaction (txn);

- journal_drain_deferred_blocks should document what it does, not just
  its call conditions :), and more generally the functions that are
  not already documented in a .h and not just a _locked variant of a
  documented function.

Milos Nikic, le mer. 01 avril 2026 10:55:19 -0700, a ecrit:
> As a quick reassurance regarding the buildd machines: the journal is strictly
> opt-in. If this code is merged as-is, zero journaling paths are actually
> exercised by default. (the weird assert blowup in v5 got fixed up as well)

Yes, but without actual testing before enabling on the buildds, I end up
being the beta-tester, without really having proper setup to properly
dig up information from the boxes which are supposed to keep building
packages.

And most often, unless I enable the new features on the buildds, the
subtle bugs that get discovered by heavy work do not get discovered, so
it ends up being a chicken&egg issue that I have seen keep repeat,
making buildd life difficult.

So thanks for the strong testing!

I'll upload an updated debian packages later.

Samuel