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
