lib/deploy: Use fallocate for early prune space check
#2866
Conversation
This file contains 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
Classic case of analysis getting confused by variables initialized by a function.
Noticed this diagnostic in my editor with clangd hooked up.
For easier diagnostics.
`size_to_remove` looks cryptic in contrast to `new_new_bootcsum_dirs_total_size`. Rename it in the style of the latter for easier reading.
The `f_bfree` member of the `statvfs` struct is documented as the "number of free blocks". However, different filesystems have different interpretations of this. E.g. on XFS, this is truly the number of blocks free for allocating data. On ext4 however, it includes blocks that are actually reserved by the filesystem and cannot be used for file data. (Note this is separate from the distinction between `f_bfree` and `f_bavail` which isn't relevant to us here since we're privileged.) If a kernel and initrd is sized just right so that it's still within the `f_bfree` limit but above what we can actually allocate, the early prune code won't kick in since it'll think that there is enough space. So we end up hitting `ENOSPC` when we actually copy the files in. Rework the early prune code to instead use `fallocate` which guarantees us that a file of a certain size can fit on the filesystem. `fallocate` requires filesystem support, but all the filesystems we care about for the bootfs support it (including even FAT). (There's technically a TOCTOU race here that existed also with the `statvfs` code where free space could change between when we check and when we copy. Ideally we'd be able to pass down that fd to the copying bits, but anyway in practice the bootfs is pretty much owned by libostree and one doesn't expect concurrent writes during a finalization operation.)
cgwalters
approved these changes
May 30, 2023
| /* This is a roundabout but more trustworthy way of doing a space check than | ||
| * relying on statvfs's f_bfree when you know the size of the objects. */ | ||
| static gboolean | ||
| dfd_fallocate_check (int dfd, __off_t len, gboolean *out_passed, GError **error) |
There was a problem hiding this comment.
I think what would be more efficient and cleaner here is to allocate a temporary fd per file we're going to copy, and then actually use the them to do the writes.
But that's also a lot more code changes.
There was a problem hiding this comment.
Indeed. I mentioned passing down the fd in the commit message, but yeah... churn.
|
Backport to Fedora RPMs: https://src.fedoraproject.org/rpms/ostree/pull-request/33# |
dustymabe
added a commit
to dustymabe/fedora-coreos-config
that referenced
this pull request
May 31, 2023
It has a fix for an autopruning corner case. ostreedev/ostree#2866
dustymabe
added a commit
to coreos/fedora-coreos-config
that referenced
this pull request
May 31, 2023
It has a fix for an autopruning corner case. ostreedev/ostree#2866
HuijingHei
pushed a commit
to HuijingHei/fedora-coreos-config
that referenced
this pull request
Oct 10, 2023
It has a fix for an autopruning corner case. ostreedev/ostree#2866
HuijingHei
pushed a commit
to HuijingHei/fedora-coreos-config
that referenced
this pull request
Oct 10, 2023
It has a fix for an autopruning corner case. ostreedev/ostree#2866
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.
The
f_bfreemember of thestatvfsstruct is documented as the"number of free blocks". However, different filesystems have different
interpretations of this. E.g. on XFS, this is truly the number of blocks
free for allocating data. On ext4 however, it includes blocks that
are actually reserved by the filesystem and cannot be used for file
data. (Note this is separate from the distinction between
f_bfreeandf_bavailwhich isn't relevant to us here since we're privileged.)If a kernel and initrd is sized just right so that it's still within the
f_bfreelimit but above what we can actually allocate, the early prunecode won't kick in since it'll think that there is enough space. So we
end up hitting
ENOSPCwhen we actually copy the files in.Rework the early prune code to instead use
fallocatewhich guaranteesus that a file of a certain size can fit on the filesystem.
fallocaterequires filesystem support, but all the filesystems we care about for
the bootfs support it (including even FAT).
(There's technically a TOCTOU race here that existed also with the
statvfscode where free space could change between when we checkand when we copy. Ideally we'd be able to pass down that fd to the
copying bits, but anyway in practice the bootfs is pretty much owned by
libostree and one doesn't expect concurrent writes during a finalization
operation.)