Deduplicate repo+sysroot syncfs logic #3509
Merged
+80
−50
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Code Review
This pull request refactors full_system_sync() to use an existing repository file descriptor, which is a good simplification. It also adds a new validation in ensure_repo() to confirm that the sysroot and repository are on the same device, enhancing robustness. My review includes a minor suggestion to improve the portability of an error message's format string.
champtar
reviewed
Aug 21, 2025
champtar
reviewed
Aug 21, 2025
0d6963b to
a1fcddc
Compare
champtar
reviewed
Aug 24, 2025
This is a followup to ostreedev@6e5a27a which I believe is correct as is. However, we already have a file descriptor open for the ostree repo, which *must* be on the same filesystem as `/sysroot/ostree` (the deployment code forces hardlinking today). It's hence cleaner to reuse that extant fd instead of opening a new one - we know we did writes to that fd. But going farther here, there already is logic to use syncfs for the repo when downloading objects (in a common case we actually syncfs twice). Since these are really the same operation, unify them: - Add journaling to the repo one syncfs case - Change the sysroot case to just call it - Since we log consistently to the journal for all syncfs/fsfreeze operations now, drop the SyncStats bits which was a way to add info about that to a later journal message Additionally, let's add an extra check when we're opening the repo that it's on the same device just on general principle. Signed-off-by: Colin Walters <walters@verbum.org>
a1fcddc to
cb369f7
Compare
|
OK I rewrote this one again, it's now one commit and syncs up the syncfs logic |
champtar
approved these changes
Aug 25, 2025
|
|
||
| if (self->disable_fsync) | ||
| return TRUE; | ||
| /* FIXME: Added OSTREE_SUPPRESS_SYNCFS since valgrind in el7 doesn't know |
There was a problem hiding this comment.
EL 7 is long dead, should we drop OSTREE_SUPPRESS_SYNCFS ?
There was a problem hiding this comment.
Yeah I just kept it on the principle of not changing too many things at once.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a followup to 6e5a27a
which I believe is correct as is. However, we already have a file
descriptor open for the ostree repo, which must be on
the same filesystem as
/sysroot/ostree(the deploymentcode forces hardlinking today).
It's hence cleaner to reuse that extant fd instead of opening
a new one - we know we did writes to that fd.
But going farther here, there already is logic to use syncfs
for the repo when downloading objects (in a common case
we actually syncfs twice).
Since these are really the same operation, unify them:
operations now, drop the SyncStats bits which was a way
to add info about that to a later journal message
Additionally, let's add an extra check when we're
opening the repo that it's on the same device just on general
principle.
Signed-off-by: Colin Walters walters@verbum.org