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 498376090a10efbf1c9edd3a644cebd55566ddd9..f0fdbd57aa8abcf1de2a5d5f7fddc1a3ceef339e 100644 (file)
--- 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 1bc4cb81c472f7401960a5adaf63801d962d7e6f..d3e0c4536db6b3c507fd36d2815c6e338540790f 100644 (file)
--- a/tests/test-repo.c
+++ b/tests/test-repo.c
@@ -27,8 +27,10 @@
 #include <glib.h>
 #include <libglnx.h>
 #include <locale.h>
+#include <sys/statvfs.h>
 
 #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,