From: Jonathan Lebon Date: Thu, 23 Jul 2026 02:09:15 +0000 (-0400) Subject: lib/commit: Fix min-free-space accounting for duplicate content objects X-Git-Tag: archive/raspbian/2026.4-1+rpi1^2~9^2~1^2~17^2~1 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=16e537cb8a9b69d8f254dfbf4abf4f0358251f56;p=ostree.git lib/commit: Fix min-free-space accounting for duplicate content objects First, in the _ostree_repo_bare_content_commit(), we never actually checked whether the file was already present in the repo before linking it in place. Do this so that we can no-op up front. Second, and the actual bug this patch is fixing: the min-free-space accounting in write_content_object() and _ostree_repo_bare_content_commit() optimistically reserves space from the running `txn.max_blocks` counter _before_ knowing whether we'll actually no-op or not based on the object already existing. Over many duplicate writes within a single transaction, the counter diverges from the actual free space and eventually hits zero, causing a spurious "min-free-space would be exceeded" error even with plenty of disk space available. This may be the source of the CI issue FCOS is hitting in https://github.com/coreos/fedora-coreos-config/pull/4265 due to the bootc SELinux relabeling done since https://github.com/bootc-dev/bootc/pull/2088. This patch saves the number of blocks reserved and credits them back in both code paths when a duplicate object is detected. This was heavily AI-guided, finding the bug, and then doing red-green testing towards the fix. Assisted-by: AI --- diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 49837609..f0fdbd57 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -550,6 +550,7 @@ _ostree_repo_bare_content_commit (OstreeRepo *self, OstreeRepoBareContent *barew OstreeRealRepoBareContent *real = (OstreeRealRepoBareContent *)barewrite; g_assert (real->initialized); + fsblkcnt_t object_blocks_reserved = 0; if ((self->min_free_space_percent > 0 || self->min_free_space_mb > 0) && self->in_transaction) { struct stat st_buf; @@ -559,15 +560,15 @@ _ostree_repo_bare_content_commit (OstreeRepo *self, OstreeRepoBareContent *barew g_mutex_lock (&self->txn_lock); g_assert_cmpint (self->txn.blocksize, >, 0); - const fsblkcnt_t object_blocks = (st_buf.st_size / self->txn.blocksize) + 1; - if (object_blocks > self->txn.max_blocks) + object_blocks_reserved = (st_buf.st_size / self->txn.blocksize) + 1; + if (object_blocks_reserved > self->txn.max_blocks) { self->cleanup_stagedir = TRUE; g_mutex_unlock (&self->txn_lock); return throw_min_free_space_error (self, st_buf.st_size, error); } /* This is the main bit that needs mutex protection */ - self->txn.max_blocks -= object_blocks; + self->txn.max_blocks -= object_blocks_reserved; g_mutex_unlock (&self->txn_lock); } @@ -578,9 +579,19 @@ _ostree_repo_bare_content_commit (OstreeRepo *self, OstreeRepoBareContent *barew checksum_buf, error)) return FALSE; + gboolean obj_existed; if (!commit_loose_regfile_object (self, checksum_buf, &real->tmpf, real->uid, real->gid, - real->mode, real->xattrs, NULL, cancellable, error)) + real->mode, real->xattrs, &obj_existed, cancellable, error)) return FALSE; + /* If the object already existed, credit back the space reservation we + * made above — no new disk space was consumed. + */ + if (obj_existed) + { + g_mutex_lock (&self->txn_lock); + self->txn.max_blocks += object_blocks_reserved; + g_mutex_unlock (&self->txn_lock); + } /* Let's have a guarantee that after commit the object is cleaned up */ _ostree_repo_bare_content_cleanup (barewrite); @@ -969,21 +980,28 @@ write_content_object (OstreeRepo *self, const char *expected_checksum, GInputStr (void)file_input_owned; // Conditionally owned - /* Free space check; only applies during transactions */ + /* Free space check; only applies during transactions. + * + * We optimistically reserve space here before we know the object's + * checksum. If we later discover the object already exists in the repo + * (i.e. it is a duplicate), we credit the reservation back — see the + * have_obj block below. + */ + fsblkcnt_t object_blocks_reserved = 0; if ((self->min_free_space_percent > 0 || self->min_free_space_mb > 0) && self->in_transaction) { g_mutex_lock (&self->txn_lock); g_assert_cmpint (self->txn.blocksize, >, 0); - const fsblkcnt_t object_blocks = (size / self->txn.blocksize) + 1; - if (object_blocks > self->txn.max_blocks) + object_blocks_reserved = (size / self->txn.blocksize) + 1; + if (object_blocks_reserved > self->txn.max_blocks) { - guint64 bytes_required = (guint64)object_blocks * self->txn.blocksize; + guint64 bytes_required = (guint64)object_blocks_reserved * self->txn.blocksize; self->cleanup_stagedir = TRUE; g_mutex_unlock (&self->txn_lock); return throw_min_free_space_error (self, bytes_required, error); } /* This is the main bit that needs mutex protection */ - self->txn.max_blocks -= object_blocks; + self->txn.max_blocks -= object_blocks_reserved; g_mutex_unlock (&self->txn_lock); } @@ -1110,11 +1128,14 @@ write_content_object (OstreeRepo *self, const char *expected_checksum, GInputStr if (!_ostree_repo_has_loose_object (self, actual_checksum, OSTREE_OBJECT_TYPE_FILE, &have_obj, cancellable, error)) return FALSE; - /* If we already have it, just update the stats. */ + /* If we already have it, just update the stats. Also credit back the + * free-space reservation we made above — no new disk space was consumed. + */ if (have_obj) { g_mutex_lock (&self->txn_lock); self->txn.stats.content_objects_total++; + self->txn.max_blocks += object_blocks_reserved; g_mutex_unlock (&self->txn_lock); if (!_create_payload_link (self, actual_checksum, actual_payload_checksum, file_info, diff --git a/tests/test-repo.c b/tests/test-repo.c index 1bc4cb81..d3e0c453 100644 --- a/tests/test-repo.c +++ b/tests/test-repo.c @@ -27,8 +27,10 @@ #include #include #include +#include #include "ostree-autocleanups.h" +#include "ostree-repo-private.h" #include "ostree-types.h" /* Test fixture. Creates a temporary directory. */ @@ -541,6 +543,70 @@ test_repo_lock_multi_thread (Fixture *fixture, gconstpointer test_data) g_thread_join (thread2); } +/* Test that writing duplicate content objects within a single transaction does + * not spuriously trigger the min-free-space check. + */ +static void +test_min_free_space_dup_content (Fixture *fixture, gconstpointer test_data) +{ + g_autoptr (GError) error = NULL; + + g_autoptr (OstreeRepo) repo = ostree_repo_create_at ( + fixture->tmpdir.fd, ".", OSTREE_REPO_MODE_BARE_USER_ONLY, NULL, NULL, &error); + g_assert_no_error (error); + + const guint n_files = 64; + const guint file_size = 4096; + const guint dup_rounds = 10; + + ostree_repo_prepare_transaction (repo, NULL, NULL, &error); + g_assert_no_error (error); + + /* Directly set the free-space budget via the private txn.max_blocks field. Budget for 2x the + * blocks that one round of unique writes will reserve. With the bug, 11 rounds would be charged + * (1 unique + 10 duplicate), easily exceeding 2x. With the fix, only the unique round is + * charged. + */ + fsblkcnt_t blocks_per_file = (file_size / repo->txn.blocksize) + 1; + repo->txn.max_blocks = n_files * blocks_per_file * 2; + /* Enable space check by setting a nonzero min_free_space_percent. */ + repo->min_free_space_percent = 1; + + g_autofree guint8 *buf = g_malloc0 (file_size); + + /* Round 1: write unique objects. Track checksums to verify each file + * produces a distinct content object. + */ + g_autoptr (GHashTable) seen = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); + for (guint i = 0; i < n_files; i++) + { + memcpy (buf, &i, sizeof (i)); + g_autofree char *checksum = ostree_repo_write_regfile_inline ( + repo, NULL, 0, 0, S_IFREG | 0644, NULL, buf, file_size, NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (checksum); + g_assert_false (g_hash_table_contains (seen, checksum)); + g_hash_table_add (seen, g_steal_pointer (&checksum)); + } + g_assert_cmpuint (g_hash_table_size (seen), ==, n_files); + + /* Duplicate rounds: rewrite the exact same objects many times. */ + for (guint round = 0; round < dup_rounds; round++) + { + for (guint i = 0; i < n_files; i++) + { + memcpy (buf, &i, sizeof (i)); + g_autofree char *checksum = ostree_repo_write_regfile_inline ( + repo, NULL, 0, 0, S_IFREG | 0644, NULL, buf, file_size, NULL, &error); + g_assert_no_error (error); + g_assert_nonnull (checksum); + } + } + + ostree_repo_commit_transaction (repo, NULL, NULL, &error); + g_assert_no_error (error); +} + int main (int argc, char **argv) { @@ -553,6 +619,8 @@ main (int argc, char **argv) g_test_add ("/repo/get_min_free_space", Fixture, NULL, setup, test_repo_get_min_free_space, teardown); g_test_add ("/repo/write_regfile_api", Fixture, NULL, setup, test_write_regfile_api, teardown); + g_test_add ("/repo/min_free_space_dup_content", Fixture, NULL, setup, + test_min_free_space_dup_content, teardown); g_test_add ("/repo/autolock", Fixture, NULL, setup, test_repo_autolock, teardown); g_test_add ("/repo/lock/single", Fixture, NULL, lock_setup, test_repo_lock_single, teardown); g_test_add ("/repo/lock/unlock-never-locked", Fixture, NULL, lock_setup,