From a8b52f6e21e052ed92b4ab3c9ab2104ca0522e8e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 14 Apr 2021 20:43:53 -0400 Subject: [PATCH 1/7] repo: Make locking APIs public Doing anything even somewhat sophisticated requires this; turns out our own `ostree prune` CLI wants this, e.g. https://github.com/ostreedev/ostree/issues/2337 Closes: https://github.com/ostreedev/ostree/issues/2286 (cherry picked from commit 0f36d8c2219decbb5794ff19b5259d15d3d8d791) The new symbols have been added to the released symbols file rather than the development symbols file as in the upstream PR since our package does a released build. Similarly, the upstream `LIBOSTREE_2021.3` symbol version has been used so that packages don't need to be rebuilt when we get to that version and drop our backported commits. https://phabricator.endlessm.com/T31868 --- apidoc/ostree-sections.txt | 6 +++ src/libostree/libostree-released.sym | 12 +++++ src/libostree/ostree-repo-commit.c | 8 +-- src/libostree/ostree-repo-private.h | 24 --------- src/libostree/ostree-repo-prune.c | 8 +-- src/libostree/ostree-repo-static-delta-core.c | 2 +- src/libostree/ostree-repo.c | 51 ++++++++++--------- src/libostree/ostree-repo.h | 50 ++++++++++++++++++ src/libostree/ostree-sysroot-cleanup.c | 2 +- tests/test-core.js | 8 +++ tests/test-repo.c | 27 +++++++++- tests/test-symbols.sh | 2 +- 12 files changed, 141 insertions(+), 59 deletions(-) diff --git a/apidoc/ostree-sections.txt b/apidoc/ostree-sections.txt index 64bc68d23..4904153fc 100644 --- a/apidoc/ostree-sections.txt +++ b/apidoc/ostree-sections.txt @@ -310,6 +310,12 @@ ostree_repo_get_min_free_space_bytes ostree_repo_get_config ostree_repo_get_dfd ostree_repo_get_default_repo_finders +OstreeRepoLockType +ostree_repo_lock_pop +ostree_repo_lock_push +OstreeRepoAutoLock +ostree_repo_auto_lock_push +ostree_repo_auto_lock_cleanup ostree_repo_hash ostree_repo_equal ostree_repo_copy_config diff --git a/src/libostree/libostree-released.sym b/src/libostree/libostree-released.sym index 4f80d7fc9..90d56babf 100644 --- a/src/libostree/libostree-released.sym +++ b/src/libostree/libostree-released.sym @@ -638,6 +638,18 @@ global: ostree_repo_gpg_sign_data; } LIBOSTREE_2020.7; +/* Endless backported symbols. Note that the upstream symbol version is + * used so that programs linking to the symbols don't need to be rebuilt + * when we get to the upstream release containing the symbols. + */ +LIBOSTREE_2021.3 { +global: + ostree_repo_auto_lock_push; + ostree_repo_auto_lock_cleanup; + ostree_repo_lock_push; + ostree_repo_lock_pop; +} LIBOSTREE_2020.8; + /* NOTE: Only add more content here in release commits! See the * comments at the top of this file. */ diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 690075e12..2b2f6ccba 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -1794,8 +1794,8 @@ ostree_repo_prepare_transaction (OstreeRepo *self, memset (&self->txn.stats, 0, sizeof (OstreeRepoTransactionStats)); - self->txn_locked = _ostree_repo_lock_push (self, OSTREE_REPO_LOCK_SHARED, - cancellable, error); + self->txn_locked = ostree_repo_lock_push (self, OSTREE_REPO_LOCK_SHARED, + cancellable, error); if (!self->txn_locked) return FALSE; @@ -2451,7 +2451,7 @@ ostree_repo_commit_transaction (OstreeRepo *self, if (self->txn_locked) { - if (!_ostree_repo_lock_pop (self, cancellable, error)) + if (!ostree_repo_lock_pop (self, cancellable, error)) return FALSE; self->txn_locked = FALSE; } @@ -2509,7 +2509,7 @@ ostree_repo_abort_transaction (OstreeRepo *self, if (self->txn_locked) { - if (!_ostree_repo_lock_pop (self, cancellable, error)) + if (!ostree_repo_lock_pop (self, cancellable, error)) return FALSE; self->txn_locked = FALSE; } diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 714fda8ba..5703db11b 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -496,30 +496,6 @@ _ostree_repo_maybe_regenerate_summary (OstreeRepo *self, GCancellable *cancellable, GError **error); -/* Locking APIs are currently private. - * See https://github.com/ostreedev/ostree/pull/1555 - */ -typedef enum { - OSTREE_REPO_LOCK_SHARED, - OSTREE_REPO_LOCK_EXCLUSIVE -} OstreeRepoLockType; - -gboolean _ostree_repo_lock_push (OstreeRepo *self, - OstreeRepoLockType lock_type, - GCancellable *cancellable, - GError **error); -gboolean _ostree_repo_lock_pop (OstreeRepo *self, - GCancellable *cancellable, - GError **error); - -typedef OstreeRepo OstreeRepoAutoLock; - -OstreeRepoAutoLock * _ostree_repo_auto_lock_push (OstreeRepo *self, - OstreeRepoLockType lock_type, - GCancellable *cancellable, - GError **error); -void _ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock); -G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRepoAutoLock, _ostree_repo_auto_lock_cleanup) gboolean _ostree_tmpf_fsverity_core (GLnxTmpfile *tmpf, diff --git a/src/libostree/ostree-repo-prune.c b/src/libostree/ostree-repo-prune.c index 0b9536281..c4ce64abc 100644 --- a/src/libostree/ostree-repo-prune.c +++ b/src/libostree/ostree-repo-prune.c @@ -204,7 +204,7 @@ ostree_repo_prune_static_deltas (OstreeRepo *self, const char *commit, GError **error) { g_autoptr(OstreeRepoAutoLock) lock = - _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); + ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); if (!lock) return FALSE; @@ -325,7 +325,7 @@ ostree_repo_traverse_reachable_refs (OstreeRepo *self, GError **error) { g_autoptr(OstreeRepoAutoLock) lock = - _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_SHARED, cancellable, error); + ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_SHARED, cancellable, error); if (!lock) return FALSE; @@ -400,7 +400,7 @@ ostree_repo_prune (OstreeRepo *self, GError **error) { g_autoptr(OstreeRepoAutoLock) lock = - _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); + ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); if (!lock) return FALSE; @@ -486,7 +486,7 @@ ostree_repo_prune_from_reachable (OstreeRepo *self, GError **error) { g_autoptr(OstreeRepoAutoLock) lock = - _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); + ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); if (!lock) return FALSE; diff --git a/src/libostree/ostree-repo-static-delta-core.c b/src/libostree/ostree-repo-static-delta-core.c index c25367248..a71c749ea 100644 --- a/src/libostree/ostree-repo-static-delta-core.c +++ b/src/libostree/ostree-repo-static-delta-core.c @@ -1268,7 +1268,7 @@ ostree_repo_static_delta_reindex (OstreeRepo *repo, /* Protect against parallel prune operation */ g_autoptr(OstreeRepoAutoLock) lock = - _ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_SHARED, cancellable, error); + ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_SHARED, cancellable, error); if (!lock) return FALSE; diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index ffb2e630a..553bfc3d0 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -444,7 +444,7 @@ pop_repo_lock (OstreeRepo *self, return TRUE; } -/* +/** * ostree_repo_lock_push: * @self: a #OstreeRepo * @lock_type: the type of lock to acquire @@ -470,12 +470,13 @@ pop_repo_lock (OstreeRepo *self, * %TRUE is returned. * * Returns: %TRUE on success, otherwise %FALSE with @error set + * Since: 2021.3 */ gboolean -_ostree_repo_lock_push (OstreeRepo *self, - OstreeRepoLockType lock_type, - GCancellable *cancellable, - GError **error) +ostree_repo_lock_push (OstreeRepo *self, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error) { g_return_val_if_fail (self != NULL, FALSE); g_return_val_if_fail (OSTREE_IS_REPO (self), FALSE); @@ -538,8 +539,8 @@ _ostree_repo_lock_push (OstreeRepo *self, } } -/* - * _ostree_repo_lock_pop: +/** + * ostree_repo_lock_pop: * @self: a #OstreeRepo * @cancellable: a #GCancellable * @error: a #GError @@ -560,11 +561,12 @@ _ostree_repo_lock_push (OstreeRepo *self, * %TRUE is returned. * * Returns: %TRUE on success, otherwise %FALSE with @error set + * Since: 2021.3 */ gboolean -_ostree_repo_lock_pop (OstreeRepo *self, - GCancellable *cancellable, - GError **error) +ostree_repo_lock_pop (OstreeRepo *self, + GCancellable *cancellable, + GError **error) { g_return_val_if_fail (self != NULL, FALSE); g_return_val_if_fail (OSTREE_IS_REPO (self), FALSE); @@ -628,7 +630,7 @@ _ostree_repo_lock_pop (OstreeRepo *self, } /* - * _ostree_repo_auto_lock_push: (skip) + * ostree_repo_auto_lock_push: (skip) * @self: a #OstreeRepo * @lock_type: the type of lock to acquire * @cancellable: a #GCancellable @@ -642,34 +644,37 @@ _ostree_repo_lock_pop (OstreeRepo *self, * * |[ * g_autoptr(OstreeRepoAutoLock) lock = NULL; - * lock = _ostree_repo_auto_lock_push (repo, lock_type, cancellable, error); + * lock = ostree_repo_auto_lock_push (repo, lock_type, cancellable, error); * if (!lock) * return FALSE; * ]| * * Returns: @self on success, otherwise %NULL with @error set + * Since: 2021.3 */ OstreeRepoAutoLock * -_ostree_repo_auto_lock_push (OstreeRepo *self, - OstreeRepoLockType lock_type, - GCancellable *cancellable, - GError **error) +ostree_repo_auto_lock_push (OstreeRepo *self, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error) { - if (!_ostree_repo_lock_push (self, lock_type, cancellable, error)) + if (!ostree_repo_lock_push (self, lock_type, cancellable, error)) return NULL; return (OstreeRepoAutoLock *)self; } -/* - * _ostree_repo_auto_lock_cleanup: (skip) +/** + * ostree_repo_auto_lock_cleanup: (skip) * @lock: a #OstreeRepoAutoLock * * A cleanup handler for use with ostree_repo_auto_lock_push(). If @lock is * not %NULL, ostree_repo_lock_pop() will be called on it. If * ostree_repo_lock_pop() fails, a critical warning will be emitted. + * + * Since: 2021.3 */ void -_ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock) +ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock) { OstreeRepo *repo = lock; if (repo) @@ -677,7 +682,7 @@ _ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock) g_autoptr(GError) error = NULL; int errsv = errno; - if (!_ostree_repo_lock_pop (repo, NULL, &error)) + if (!ostree_repo_lock_pop (repo, NULL, &error)) g_critical ("Cleanup repo lock failed: %s", error->message); errno = errsv; @@ -5831,8 +5836,8 @@ ostree_repo_regenerate_summary (OstreeRepo *self, g_autoptr(OstreeRepoAutoLock) lock = NULL; gboolean no_deltas_in_summary = FALSE; - lock = _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, - cancellable, error); + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); if (!lock) return FALSE; diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index e64c3230c..ab2a37bfd 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -1466,6 +1466,56 @@ gboolean ostree_repo_regenerate_summary (OstreeRepo *self, GCancellable *cancellable, GError **error); + +/** + * OstreeRepoLockType: + * @OSTREE_REPO_LOCK_SHARED: A "read only" lock; multiple readers are allowed. + * @OSTREE_REPO_LOCK_EXCLUSIVE: A writable lock at most one writer can be active, and zero readers. + * + * Flags controlling repository locking. + * + * Since: 2021.3 + */ +typedef enum { + OSTREE_REPO_LOCK_SHARED, + OSTREE_REPO_LOCK_EXCLUSIVE +} OstreeRepoLockType; + +_OSTREE_PUBLIC +gboolean ostree_repo_lock_push (OstreeRepo *self, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error); +_OSTREE_PUBLIC +gboolean ostree_repo_lock_pop (OstreeRepo *self, + GCancellable *cancellable, + GError **error); + +/* C convenience API only */ +#ifndef __GI_SCANNER__ + +/** + * OstreeRepoAutoLock: (skip) + * + * An opaque type for use with ostree_repo_auto_lock_push(). + * + * Since: 2021.3 + */ +typedef OstreeRepo OstreeRepoAutoLock; + +_OSTREE_PUBLIC +OstreeRepoAutoLock * ostree_repo_auto_lock_push (OstreeRepo *self, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error); + +_OSTREE_PUBLIC +void ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock); +G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRepoAutoLock, ostree_repo_auto_lock_cleanup) + +#endif + + /** * OSTREE_REPO_METADATA_REF: * diff --git a/src/libostree/ostree-sysroot-cleanup.c b/src/libostree/ostree-sysroot-cleanup.c index 271228340..91381cb0c 100644 --- a/src/libostree/ostree-sysroot-cleanup.c +++ b/src/libostree/ostree-sysroot-cleanup.c @@ -505,7 +505,7 @@ ostree_sysroot_cleanup_prune_repo (OstreeSysroot *sysroot, * the prune. */ g_autoptr(OstreeRepoAutoLock) lock = - _ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); + ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); if (!lock) return FALSE; diff --git a/tests/test-core.js b/tests/test-core.js index a9ef89192..6e54ab082 100755 --- a/tests/test-core.js +++ b/tests/test-core.js @@ -70,4 +70,12 @@ repo.commit_transaction(null, null); [,readCommit] = repo.resolve_rev('someref', true); assertEquals(readCommit, null); +// Basic locking API sanity test +repo.lock_push(OSTree.RepoLockType.SHARED, null); +repo.lock_push(OSTree.RepoLockType.SHARED, null); +repo.lock_pop(null); +repo.lock_pop(null); +repo.lock_push(OSTree.RepoLockType.EXCLUSIVE, null); +repo.lock_pop(null); + print("ok test-core"); diff --git a/tests/test-repo.c b/tests/test-repo.c index 9857228e1..ae221ac0a 100644 --- a/tests/test-repo.c +++ b/tests/test-repo.c @@ -197,6 +197,30 @@ test_repo_get_min_free_space (Fixture *fixture, } } +/* Just a sanity check of the C autolocking API */ +static void +test_repo_autolock (Fixture *fixture, + gconstpointer test_data) +{ + g_autoptr(GError) error = NULL; + g_autoptr(OstreeRepo) repo = ostree_repo_create_at (fixture->tmpdir.fd, ".", + OSTREE_REPO_MODE_ARCHIVE, + NULL, + NULL, &error); + g_assert_no_error (error); + + { + g_autoptr(OstreeRepoAutoLock) lock = ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + } + + g_autoptr(OstreeRepoAutoLock) lock1 = ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + + g_autoptr(OstreeRepoAutoLock) lock2 = ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); +} + int main (int argc, char **argv) @@ -212,7 +236,8 @@ main (int argc, test_repo_equal, teardown); g_test_add ("/repo/get_min_free_space", Fixture, NULL, setup, test_repo_get_min_free_space, teardown); - + g_test_add ("/repo/autolock", Fixture, NULL, setup, + test_repo_autolock, teardown); return g_test_run (); } diff --git a/tests/test-symbols.sh b/tests/test-symbols.sh index 3072c2126..bad48620c 100755 --- a/tests/test-symbols.sh +++ b/tests/test-symbols.sh @@ -66,7 +66,7 @@ echo 'ok documented symbols' # ONLY update this checksum in release commits! cat > released-sha256.txt < Date: Wed, 28 Apr 2021 21:13:15 -0600 Subject: [PATCH 2/7] repo: Require lock type in ostree_repo_lock_pop This simplifies the lock state management considerably since the previously pushed type doesn't need to be tracked. Instead, 2 counters are kept to track how many times each lock type has been pushed. When the number of exclusive locks drops to 0, the lock transitions back to shared. (cherry picked from commit c3ada6fa7a1db488fced6acde5faaaf435e09aba) --- src/libostree/ostree-repo-commit.c | 4 +- src/libostree/ostree-repo.c | 182 +++++++++++++++++++---------- src/libostree/ostree-repo.h | 9 +- tests/test-core.js | 6 +- 4 files changed, 131 insertions(+), 70 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 2b2f6ccba..1535ffaa9 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2451,7 +2451,7 @@ ostree_repo_commit_transaction (OstreeRepo *self, if (self->txn_locked) { - if (!ostree_repo_lock_pop (self, cancellable, error)) + if (!ostree_repo_lock_pop (self, OSTREE_REPO_LOCK_SHARED, cancellable, error)) return FALSE; self->txn_locked = FALSE; } @@ -2509,7 +2509,7 @@ ostree_repo_abort_transaction (OstreeRepo *self, if (self->txn_locked) { - if (!ostree_repo_lock_pop (self, cancellable, error)) + if (!ostree_repo_lock_pop (self, OSTREE_REPO_LOCK_SHARED, cancellable, error)) return FALSE; self->txn_locked = FALSE; } diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 553bfc3d0..5a66da726 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -214,7 +214,8 @@ static GPrivate repo_lock_table = G_PRIVATE_INIT (free_repo_lock_table); typedef struct { int fd; - GQueue stack; + guint shared; /* Number of shared locks */ + guint exclusive; /* Number of exclusive locks */ } OstreeRepoLock; typedef struct { @@ -223,6 +224,22 @@ typedef struct { const char *name; } OstreeRepoLockInfo; +static const char * +lock_state_name (int state) +{ + switch (state) + { + case LOCK_EX: + return "exclusive"; + case LOCK_SH: + return "shared"; + case LOCK_UN: + return "unlocked"; + default: + g_assert_not_reached (); + } +} + static void repo_lock_info (OstreeRepoLock *lock, OstreeRepoLockInfo *out_info) { @@ -230,17 +247,14 @@ repo_lock_info (OstreeRepoLock *lock, OstreeRepoLockInfo *out_info) g_assert (out_info != NULL); OstreeRepoLockInfo info; - info.len = g_queue_get_length (&lock->stack); + info.len = lock->shared + lock->exclusive; if (info.len == 0) - { info.state = LOCK_UN; - info.name = "unlocked"; - } + else if (lock->exclusive > 0) + info.state = LOCK_EX; else - { - info.state = GPOINTER_TO_INT (g_queue_peek_head (&lock->stack)); - info.name = (info.state == LOCK_EX) ? "exclusive" : "shared"; - } + info.state = LOCK_SH; + info.name = lock_state_name (info.state); *out_info = info; } @@ -256,7 +270,6 @@ free_repo_lock (gpointer data) repo_lock_info (lock, &info); g_debug ("Free lock: state=%s, depth=%u", info.name, info.len); - g_queue_clear (&lock->stack); if (lock->fd >= 0) { g_debug ("Closing repo lock file"); @@ -339,6 +352,7 @@ push_repo_lock (OstreeRepo *self, GError **error) { int flags = (lock_type == OSTREE_REPO_LOCK_EXCLUSIVE) ? LOCK_EX : LOCK_SH; + int next_state = flags; if (!blocking) flags |= LOCK_NB; @@ -355,7 +369,6 @@ push_repo_lock (OstreeRepo *self, if (lock == NULL) { lock = g_new0 (OstreeRepoLock, 1); - g_queue_init (&lock->stack); g_debug ("Opening repo lock file"); lock->fd = TEMP_FAILURE_RETRY (openat (self->repo_dir_fd, ".lock", O_CREAT | O_RDWR | O_CLOEXEC, @@ -374,31 +387,42 @@ push_repo_lock (OstreeRepo *self, repo_lock_info (lock, &info); g_debug ("Push lock: state=%s, depth=%u", info.name, info.len); - if (info.state == LOCK_EX) + guint *counter; + if (next_state == LOCK_EX) + counter = &(lock->exclusive); + else + counter = &(lock->shared); + + /* Check for overflow */ + g_assert_cmpuint (*counter, <, G_MAXUINT); + + if (info.state == LOCK_EX || info.state == next_state) { - g_debug ("Repo already locked exclusively, extending stack"); - g_queue_push_head (&lock->stack, GINT_TO_POINTER (LOCK_EX)); + g_debug ("Repo already locked %s, maintaining state", info.name); } else { - int next_state = (flags & LOCK_EX) ? LOCK_EX : LOCK_SH; - const char *next_state_name = (flags & LOCK_EX) ? "exclusive" : "shared"; + /* We should never upgrade from exclusive to shared */ + g_assert (!(info.state == LOCK_EX && next_state == LOCK_SH)); + const char *next_state_name = lock_state_name (next_state); g_debug ("Locking repo %s", next_state_name); if (!do_repo_lock (lock->fd, flags)) return glnx_throw_errno_prefix (error, "Locking repo %s failed", next_state_name); - - g_queue_push_head (&lock->stack, GINT_TO_POINTER (next_state)); } + /* Update state */ + (*counter)++; + return TRUE; } static gboolean -pop_repo_lock (OstreeRepo *self, - gboolean blocking, - GError **error) +pop_repo_lock (OstreeRepo *self, + OstreeRepoLockType lock_type, + gboolean blocking, + GError **error) { int flags = blocking ? 0 : LOCK_NB; @@ -412,34 +436,57 @@ pop_repo_lock (OstreeRepo *self, OstreeRepoLockInfo info; repo_lock_info (lock, &info); g_return_val_if_fail (info.len > 0, FALSE); - g_debug ("Pop lock: state=%s, depth=%u", info.name, info.len); - if (info.len > 1) + + int state_to_drop; + guint *counter; + if (lock_type == OSTREE_REPO_LOCK_EXCLUSIVE) + { + state_to_drop = LOCK_EX; + counter = &(lock->exclusive); + } + else { - int next_state = GPOINTER_TO_INT (g_queue_peek_nth (&lock->stack, 1)); + state_to_drop = LOCK_SH; + counter = &(lock->shared); + } - /* Drop back to the previous lock state if it differs */ - if (next_state != info.state) - { - /* We should never drop from shared to exclusive */ - g_return_val_if_fail (next_state == LOCK_SH, FALSE); - g_debug ("Returning lock state to shared"); - if (!do_repo_lock (lock->fd, next_state | flags)) - return glnx_throw_errno_prefix (error, - "Setting repo lock to shared failed"); - } - else - g_debug ("Maintaining lock state as %s", info.name); + /* Make sure caller specified a valid type to release */ + g_assert_cmpuint (*counter, >, 0); + + int next_state; + if (info.len == 1) + { + /* Lock counters will be empty, unlock */ + next_state = LOCK_UN; } + else if (state_to_drop == LOCK_EX) + next_state = (lock->exclusive > 1) ? LOCK_EX : LOCK_SH; else + next_state = (lock->exclusive > 0) ? LOCK_EX : LOCK_SH; + + if (next_state == LOCK_UN) { - /* Lock stack will be empty, unlock */ g_debug ("Unlocking repo"); if (!do_repo_unlock (lock->fd, flags)) return glnx_throw_errno_prefix (error, "Unlocking repo failed"); } + else if (info.state == next_state) + { + g_debug ("Maintaining lock state as %s", info.name); + } + else + { + /* We should never drop from shared to exclusive */ + g_return_val_if_fail (next_state == LOCK_SH, FALSE); + g_debug ("Returning lock state to shared"); + if (!do_repo_lock (lock->fd, next_state | flags)) + return glnx_throw_errno_prefix (error, + "Setting repo lock to shared failed"); + } - g_queue_pop_head (&lock->stack); + /* Update state */ + (*counter)--; return TRUE; } @@ -451,13 +498,13 @@ pop_repo_lock (OstreeRepo *self, * @cancellable: a #GCancellable * @error: a #GError * - * Takes a lock on the repository and adds it to the lock stack. If @lock_type + * Takes a lock on the repository and adds it to the lock state. If @lock_type * is %OSTREE_REPO_LOCK_SHARED, a shared lock is taken. If @lock_type is * %OSTREE_REPO_LOCK_EXCLUSIVE, an exclusive lock is taken. The actual lock * state is only changed when locking a previously unlocked repository or - * upgrading the lock from shared to exclusive. If the requested lock state is + * upgrading the lock from shared to exclusive. If the requested lock type is * unchanged or would represent a downgrade (exclusive to shared), the lock - * state is not changed and the stack is simply updated. + * state is not changed. * * ostree_repo_lock_push() waits for the lock depending on the repository's * lock-timeout-secs configuration. When lock-timeout-secs is -1, a blocking lock is @@ -542,13 +589,16 @@ ostree_repo_lock_push (OstreeRepo *self, /** * ostree_repo_lock_pop: * @self: a #OstreeRepo + * @lock_type: the type of lock to release * @cancellable: a #GCancellable * @error: a #GError * - * Remove the current repository lock state from the lock stack. If the lock - * stack becomes empty, the repository is unlocked. Otherwise, the lock state - * only changes when transitioning from an exclusive lock back to a shared - * lock. + * Release a lock of type @lock_type from the lock state. If the lock state + * becomes empty, the repository is unlocked. Otherwise, the lock state only + * changes when transitioning from an exclusive lock back to a shared lock. The + * requested @lock_type must be the same type that was requested in the call to + * ostree_repo_lock_push(). It is a programmer error if these do not match and + * the program may abort if the lock would reach an invalid state. * * ostree_repo_lock_pop() waits for the lock depending on the repository's * lock-timeout-secs configuration. When lock-timeout-secs is -1, a blocking lock is @@ -564,9 +614,10 @@ ostree_repo_lock_push (OstreeRepo *self, * Since: 2021.3 */ gboolean -ostree_repo_lock_pop (OstreeRepo *self, - GCancellable *cancellable, - GError **error) +ostree_repo_lock_pop (OstreeRepo *self, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error) { g_return_val_if_fail (self != NULL, FALSE); g_return_val_if_fail (OSTREE_IS_REPO (self), FALSE); @@ -583,7 +634,7 @@ ostree_repo_lock_pop (OstreeRepo *self, else if (self->lock_timeout_seconds == REPO_LOCK_BLOCKING) { g_debug ("Popping lock blocking"); - return pop_repo_lock (self, TRUE, error); + return pop_repo_lock (self, lock_type, TRUE, error); } else { @@ -598,7 +649,7 @@ ostree_repo_lock_pop (OstreeRepo *self, return FALSE; g_autoptr(GError) local_error = NULL; - if (pop_repo_lock (self, FALSE, &local_error)) + if (pop_repo_lock (self, lock_type, FALSE, &local_error)) return TRUE; if (!g_error_matches (local_error, G_IO_ERROR, @@ -629,18 +680,22 @@ ostree_repo_lock_pop (OstreeRepo *self, } } -/* +struct OstreeRepoAutoLock { + OstreeRepo *repo; + OstreeRepoLockType lock_type; +}; + +/** * ostree_repo_auto_lock_push: (skip) * @self: a #OstreeRepo * @lock_type: the type of lock to acquire * @cancellable: a #GCancellable * @error: a #GError * - * Like ostree_repo_lock_push(), but for usage with #OstreeRepoAutoLock. - * The intended usage is to declare the #OstreeRepoAutoLock with - * g_autoptr() so that ostree_repo_auto_lock_cleanup() is called when it - * goes out of scope. This will automatically pop the lock status off - * the stack if it was acquired successfully. + * Like ostree_repo_lock_push(), but for usage with #OstreeRepoAutoLock. The + * intended usage is to declare the #OstreeRepoAutoLock with g_autoptr() so + * that ostree_repo_auto_lock_cleanup() is called when it goes out of scope. + * This will automatically release the lock if it was acquired successfully. * * |[ * g_autoptr(OstreeRepoAutoLock) lock = NULL; @@ -660,7 +715,11 @@ ostree_repo_auto_lock_push (OstreeRepo *self, { if (!ostree_repo_lock_push (self, lock_type, cancellable, error)) return NULL; - return (OstreeRepoAutoLock *)self; + + OstreeRepoAutoLock *auto_lock = g_slice_new (OstreeRepoAutoLock); + auto_lock->repo = self; + auto_lock->lock_type = lock_type; + return auto_lock; } /** @@ -674,18 +733,19 @@ ostree_repo_auto_lock_push (OstreeRepo *self, * Since: 2021.3 */ void -ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock) +ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *auto_lock) { - OstreeRepo *repo = lock; - if (repo) + if (auto_lock != NULL) { g_autoptr(GError) error = NULL; int errsv = errno; - if (!ostree_repo_lock_pop (repo, NULL, &error)) + if (!ostree_repo_lock_pop (auto_lock->repo, auto_lock->lock_type, NULL, &error)) g_critical ("Cleanup repo lock failed: %s", error->message); errno = errsv; + + g_slice_free (OstreeRepoAutoLock, auto_lock); } } diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index ab2a37bfd..33375edc5 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -1487,9 +1487,10 @@ gboolean ostree_repo_lock_push (OstreeRepo *self, GCancellable *cancellable, GError **error); _OSTREE_PUBLIC -gboolean ostree_repo_lock_pop (OstreeRepo *self, - GCancellable *cancellable, - GError **error); +gboolean ostree_repo_lock_pop (OstreeRepo *self, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error); /* C convenience API only */ #ifndef __GI_SCANNER__ @@ -1501,7 +1502,7 @@ gboolean ostree_repo_lock_pop (OstreeRepo *self, * * Since: 2021.3 */ -typedef OstreeRepo OstreeRepoAutoLock; +typedef struct OstreeRepoAutoLock OstreeRepoAutoLock; _OSTREE_PUBLIC OstreeRepoAutoLock * ostree_repo_auto_lock_push (OstreeRepo *self, diff --git a/tests/test-core.js b/tests/test-core.js index 6e54ab082..5a71fe4a5 100755 --- a/tests/test-core.js +++ b/tests/test-core.js @@ -73,9 +73,9 @@ assertEquals(readCommit, null); // Basic locking API sanity test repo.lock_push(OSTree.RepoLockType.SHARED, null); repo.lock_push(OSTree.RepoLockType.SHARED, null); -repo.lock_pop(null); -repo.lock_pop(null); +repo.lock_pop(OSTree.RepoLockType.SHARED, null); +repo.lock_pop(OSTree.RepoLockType.SHARED, null); repo.lock_push(OSTree.RepoLockType.EXCLUSIVE, null); -repo.lock_pop(null); +repo.lock_pop(OSTree.RepoLockType.EXCLUSIVE, null); print("ok test-core"); From 62b592e9797382af4b7b8b68d3208f40bf96ccff Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Wed, 28 Apr 2021 13:25:38 -0600 Subject: [PATCH 3/7] build-sys: Bump required GLib to 2.44 This will allow usage of `GMutexLocker`. This should be available on many older distros: * RHEL 7 - 2.56.1 * RHEL 8 - 2.56.4 * Debian 9 stretch (oldstable) - 2.50.3 * Debian 10 buster (stable) - 2.58.3 * Ubuntu 16.04 xenial - 2.48.2 * Ubuntu 18.04 bionic - 2.56.4 (cherry picked from commit eb09207e1abd7499bd92866cce1de6148d659a4a) --- Makefile.am | 2 +- configure.ac | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile.am b/Makefile.am index 87a705cca..640ef983e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -31,7 +31,7 @@ AM_CPPFLAGS += -DDATADIR='"$(datadir)"' -DLIBEXECDIR='"$(libexecdir)"' \ -DOSTREE_COMPILATION \ -DG_LOG_DOMAIN=\"OSTree\" \ -DOSTREE_GITREV='"$(OSTREE_GITREV)"' \ - -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_40 '-DGLIB_VERSION_MAX_ALLOWED=G_ENCODE_VERSION(2,50)' \ + -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_44 '-DGLIB_VERSION_MAX_ALLOWED=G_ENCODE_VERSION(2,50)' \ -DSOUP_VERSION_MIN_REQUIRED=SOUP_VERSION_2_40 '-DSOUP_VERSION_MAX_ALLOWED=G_ENCODE_VERSION(2,48)' # For strict aliasing, see https://bugzilla.gnome.org/show_bug.cgi?id=791622 AM_CFLAGS += -std=gnu99 -fno-strict-aliasing $(WARN_CFLAGS) diff --git a/configure.ac b/configure.ac index 088acc3d9..d75ea5e18 100644 --- a/configure.ac +++ b/configure.ac @@ -114,7 +114,7 @@ AM_PATH_GLIB_2_0(,,AC_MSG_ERROR([GLib not found])) dnl When bumping the gio-unix-2.0 dependency (or glib-2.0 in general), dnl remember to bump GLIB_VERSION_MIN_REQUIRED and dnl GLIB_VERSION_MAX_ALLOWED in Makefile.am -GIO_DEPENDENCY="gio-unix-2.0 >= 2.40.0" +GIO_DEPENDENCY="gio-unix-2.0 >= 2.44.0" PKG_CHECK_MODULES(OT_DEP_GIO_UNIX, $GIO_DEPENDENCY) dnl 5.1.0 is an arbitrary version here From 4204a7bb963f1e8d4b405cbe69c32d3929d9cddb Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 16 Apr 2021 09:55:40 -0600 Subject: [PATCH 4/7] repo: Make locking per-OstreeRepo Previously each thread maintained its own lock file descriptor regardless of whether the thread was using the same `OstreeRepo` as another thread. This was very safe but it made certain multithreaded procedures difficult. For example, if a main thread took an exclusive lock and then spawned worker threads, it would deadlock if one of the worker threads tried to acquire the lock. This moves the file descriptor from thread local storage to the `OstreeRepo` structure so that threads using the same `OstreeRepo` can share the lock. A mutex guards against threads altering the lock state concurrently. Fixes: #2344 (cherry picked from commit ccef9784d76c032b00ce5524fbfba39fa732b2fd) --- src/libostree/ostree-repo-private.h | 9 ++ src/libostree/ostree-repo.c | 165 ++++++++++------------------ 2 files changed, 68 insertions(+), 106 deletions(-) diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 5703db11b..a9bbb15df 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -104,6 +104,13 @@ typedef struct { fsblkcnt_t max_blocks; } OstreeRepoTxn; +typedef struct { + GMutex mutex; /* All other members should only be accessed with this held */ + int fd; /* The open file or flock file descriptor */ + guint shared; /* Number of shared locks curently held */ + guint exclusive; /* Number of exclusive locks currently held */ +} OstreeRepoLock; + typedef enum { _OSTREE_FEATURE_NO, _OSTREE_FEATURE_MAYBE, @@ -159,6 +166,8 @@ struct OstreeRepo { GWeakRef sysroot; /* Weak to avoid a circular ref; see also `is_system` */ char *remotes_config_dir; + OstreeRepoLock lock; + GMutex txn_lock; OstreeRepoTxn txn; gboolean txn_locked; diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 5a66da726..1e20d7ee1 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -172,52 +172,43 @@ G_DEFINE_TYPE (OstreeRepo, ostree_repo, G_TYPE_OBJECT) /* Repository locking * * To guard against objects being deleted (e.g., prune) while they're in - * use by another operation is accessing them (e.g., commit), the + * use by another operation that is accessing them (e.g., commit), the * repository must be locked by concurrent writers. * - * The locking is implemented by maintaining a thread local table of - * lock stacks per repository. This allows thread safe locking since - * each thread maintains its own lock stack. See the OstreeRepoLock type - * below. + * The repository locking has several important features: * - * The actual locking is done using either open file descriptor locks or - * flock locks. This allows the locking to work with concurrent - * processes. The lock file is held on the ".lock" file within the - * repository. + * * There are 2 states - shared and exclusive. Multiple users can hold + * a shared lock concurrently while only one user can hold an + * exclusive lock. + * + * * The lock can be taken recursively so long as each acquisition is paired + * with a matching release. The recursion is also latched to the strongest + * state. Once an exclusive lock has been taken, it will remain exclusive + * until all exclusive locks have been released. + * + * * It is both multiprocess- and multithread-safe. Threads that share + * an OstreeRepo use the lock cooperatively while processes and + * threads using separate OstreeRepo structures will block when + * acquiring incompatible lock states. + * + * The actual locking is implemented using either open file descriptor + * locks or flock locks. This allows the locking to work with concurrent + * processes or concurrent threads using a separate OstreeRepo. The lock + * file is held on the ".lock" file within the repository. * * The intended usage is to take a shared lock when writing objects or * reading objects in critical sections. Exclusive locks are taken when * deleting objects. * - * To allow fine grained locking within libostree, the lock is - * maintained as a stack. The core APIs then push or pop from the stack. - * When pushing or popping a lock state identical to the existing or - * next state, the stack is simply updated. Only when upgrading or - * downgrading the lock (changing to/from unlocked, pushing exclusive on - * shared or popping exclusive to shared) are actual locking operations - * performed. + * To allow fine grained locking, the lock state is maintained in shared and + * exclusive counters. Callers then push or pop lock types to increment or + * decrement the counters. When pushing or popping a lock type identical to + * the existing or next state, the lock state is simply updated. Only when + * upgrading or downgrading the lock (changing to/from unlocked, pushing + * exclusive on shared or popping exclusive to shared) are actual locking + * operations performed. */ -static void -free_repo_lock_table (gpointer data) -{ - GHashTable *lock_table = data; - - if (lock_table != NULL) - { - g_debug ("Free lock table"); - g_hash_table_destroy (lock_table); - } -} - -static GPrivate repo_lock_table = G_PRIVATE_INIT (free_repo_lock_table); - -typedef struct { - int fd; - guint shared; /* Number of shared locks */ - guint exclusive; /* Number of exclusive locks */ -} OstreeRepoLock; - typedef struct { guint len; int state; @@ -241,16 +232,18 @@ lock_state_name (int state) } static void -repo_lock_info (OstreeRepoLock *lock, OstreeRepoLockInfo *out_info) +repo_lock_info (OstreeRepo *self, GMutexLocker *locker, + OstreeRepoLockInfo *out_info) { - g_assert (lock != NULL); + g_assert (self != NULL); + g_assert (locker != NULL); g_assert (out_info != NULL); OstreeRepoLockInfo info; - info.len = lock->shared + lock->exclusive; + info.len = self->lock.shared + self->lock.exclusive; if (info.len == 0) info.state = LOCK_UN; - else if (lock->exclusive > 0) + else if (self->lock.exclusive > 0) info.state = LOCK_EX; else info.state = LOCK_SH; @@ -259,26 +252,6 @@ repo_lock_info (OstreeRepoLock *lock, OstreeRepoLockInfo *out_info) *out_info = info; } -static void -free_repo_lock (gpointer data) -{ - OstreeRepoLock *lock = data; - - if (lock != NULL) - { - OstreeRepoLockInfo info; - repo_lock_info (lock, &info); - - g_debug ("Free lock: state=%s, depth=%u", info.name, info.len); - if (lock->fd >= 0) - { - g_debug ("Closing repo lock file"); - (void) close (lock->fd); - } - g_free (lock); - } -} - /* Wrapper to handle flock vs OFD locking based on GLnxLockFile */ static gboolean do_repo_lock (int fd, @@ -356,42 +329,29 @@ push_repo_lock (OstreeRepo *self, if (!blocking) flags |= LOCK_NB; - GHashTable *lock_table = g_private_get (&repo_lock_table); - if (lock_table == NULL) - { - g_debug ("Creating repo lock table"); - lock_table = g_hash_table_new_full (NULL, NULL, NULL, - (GDestroyNotify)free_repo_lock); - g_private_set (&repo_lock_table, lock_table); - } + g_autoptr(GMutexLocker) locker = g_mutex_locker_new (&self->lock.mutex); - OstreeRepoLock *lock = g_hash_table_lookup (lock_table, self); - if (lock == NULL) + if (self->lock.fd == -1) { - lock = g_new0 (OstreeRepoLock, 1); g_debug ("Opening repo lock file"); - lock->fd = TEMP_FAILURE_RETRY (openat (self->repo_dir_fd, ".lock", - O_CREAT | O_RDWR | O_CLOEXEC, - DEFAULT_REGFILE_MODE)); - if (lock->fd < 0) - { - free_repo_lock (lock); - return glnx_throw_errno_prefix (error, - "Opening lock file %s/.lock failed", - gs_file_get_path_cached (self->repodir)); - } - g_hash_table_insert (lock_table, self, lock); + self->lock.fd = TEMP_FAILURE_RETRY (openat (self->repo_dir_fd, ".lock", + O_CREAT | O_RDWR | O_CLOEXEC, + DEFAULT_REGFILE_MODE)); + if (self->lock.fd < 0) + return glnx_throw_errno_prefix (error, + "Opening lock file %s/.lock failed", + gs_file_get_path_cached (self->repodir)); } OstreeRepoLockInfo info; - repo_lock_info (lock, &info); + repo_lock_info (self, locker, &info); g_debug ("Push lock: state=%s, depth=%u", info.name, info.len); guint *counter; if (next_state == LOCK_EX) - counter = &(lock->exclusive); + counter = &(self->lock.exclusive); else - counter = &(lock->shared); + counter = &(self->lock.shared); /* Check for overflow */ g_assert_cmpuint (*counter, <, G_MAXUINT); @@ -407,7 +367,7 @@ push_repo_lock (OstreeRepo *self, const char *next_state_name = lock_state_name (next_state); g_debug ("Locking repo %s", next_state_name); - if (!do_repo_lock (lock->fd, flags)) + if (!do_repo_lock (self->lock.fd, flags)) return glnx_throw_errno_prefix (error, "Locking repo %s failed", next_state_name); } @@ -426,15 +386,11 @@ pop_repo_lock (OstreeRepo *self, { int flags = blocking ? 0 : LOCK_NB; - GHashTable *lock_table = g_private_get (&repo_lock_table); - g_return_val_if_fail (lock_table != NULL, FALSE); - - OstreeRepoLock *lock = g_hash_table_lookup (lock_table, self); - g_return_val_if_fail (lock != NULL, FALSE); - g_return_val_if_fail (lock->fd != -1, FALSE); + g_autoptr(GMutexLocker) locker = g_mutex_locker_new (&self->lock.mutex); + g_return_val_if_fail (self->lock.fd != -1, FALSE); OstreeRepoLockInfo info; - repo_lock_info (lock, &info); + repo_lock_info (self, locker, &info); g_return_val_if_fail (info.len > 0, FALSE); g_debug ("Pop lock: state=%s, depth=%u", info.name, info.len); @@ -443,12 +399,12 @@ pop_repo_lock (OstreeRepo *self, if (lock_type == OSTREE_REPO_LOCK_EXCLUSIVE) { state_to_drop = LOCK_EX; - counter = &(lock->exclusive); + counter = &(self->lock.exclusive); } else { state_to_drop = LOCK_SH; - counter = &(lock->shared); + counter = &(self->lock.shared); } /* Make sure caller specified a valid type to release */ @@ -461,14 +417,14 @@ pop_repo_lock (OstreeRepo *self, next_state = LOCK_UN; } else if (state_to_drop == LOCK_EX) - next_state = (lock->exclusive > 1) ? LOCK_EX : LOCK_SH; + next_state = (self->lock.exclusive > 1) ? LOCK_EX : LOCK_SH; else - next_state = (lock->exclusive > 0) ? LOCK_EX : LOCK_SH; + next_state = (self->lock.exclusive > 0) ? LOCK_EX : LOCK_SH; if (next_state == LOCK_UN) { g_debug ("Unlocking repo"); - if (!do_repo_unlock (lock->fd, flags)) + if (!do_repo_unlock (self->lock.fd, flags)) return glnx_throw_errno_prefix (error, "Unlocking repo failed"); } else if (info.state == next_state) @@ -480,7 +436,7 @@ pop_repo_lock (OstreeRepo *self, /* We should never drop from shared to exclusive */ g_return_val_if_fail (next_state == LOCK_SH, FALSE); g_debug ("Returning lock state to shared"); - if (!do_repo_lock (lock->fd, next_state | flags)) + if (!do_repo_lock (self->lock.fd, next_state | flags)) return glnx_throw_errno_prefix (error, "Setting repo lock to shared failed"); } @@ -1117,13 +1073,8 @@ ostree_repo_finalize (GObject *object) g_clear_pointer (&self->remotes, g_hash_table_destroy); g_mutex_clear (&self->remotes_lock); - GHashTable *lock_table = g_private_get (&repo_lock_table); - if (lock_table) - { - g_hash_table_remove (lock_table, self); - if (g_hash_table_size (lock_table) == 0) - g_private_replace (&repo_lock_table, NULL); - } + glnx_close_fd (&self->lock.fd); + g_mutex_clear (&self->lock.mutex); G_OBJECT_CLASS (ostree_repo_parent_class)->finalize (object); } @@ -1285,6 +1236,7 @@ ostree_repo_init (OstreeRepo *self) self->test_error_flags = g_parse_debug_string (g_getenv ("OSTREE_REPO_TEST_ERROR"), test_error_keys, G_N_ELEMENTS (test_error_keys)); + g_mutex_init (&self->lock.mutex); g_mutex_init (&self->cache_lock); g_mutex_init (&self->txn_lock); @@ -1298,6 +1250,7 @@ ostree_repo_init (OstreeRepo *self) self->tmp_dir_fd = -1; self->objects_dir_fd = -1; self->uncompressed_objects_dir_fd = -1; + self->lock.fd = -1; self->sysroot_kind = OSTREE_REPO_SYSROOT_KIND_UNKNOWN; } From 46e74d3c7a5ac8e64f635559a4c0f530b2c842f7 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 6 May 2021 16:49:51 -0600 Subject: [PATCH 5/7] repo: Make locking precondition failures fatal Use `g_error` and `g_assert*` rather than `g_return*` when checking the locking preconditions so that failures result in the program terminating. Since this code is protecting filesystem data, we'd rather crash than delete or corrupt data unexpectedly. `g_error` is used when the error is due to the caller requesting an invalid transition like attempting to pop a lock type that hasn't been taken. It also provides a semi-useful message about what happened. (cherry picked from commit 89f4ce2c1d3cacccee8129ac54bd60775dbbe5d2) --- src/libostree/ostree-repo.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 1e20d7ee1..1b9845e8e 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -354,7 +354,8 @@ push_repo_lock (OstreeRepo *self, counter = &(self->lock.shared); /* Check for overflow */ - g_assert_cmpuint (*counter, <, G_MAXUINT); + if (*counter == G_MAXUINT) + g_error ("Repo lock %s counter would overflow", lock_state_name (next_state)); if (info.state == LOCK_EX || info.state == next_state) { @@ -387,13 +388,16 @@ pop_repo_lock (OstreeRepo *self, int flags = blocking ? 0 : LOCK_NB; g_autoptr(GMutexLocker) locker = g_mutex_locker_new (&self->lock.mutex); - g_return_val_if_fail (self->lock.fd != -1, FALSE); + if (self->lock.fd == -1) + g_error ("Cannot pop repo never locked repo lock"); OstreeRepoLockInfo info; repo_lock_info (self, locker, &info); - g_return_val_if_fail (info.len > 0, FALSE); g_debug ("Pop lock: state=%s, depth=%u", info.name, info.len); + if (info.len == 0 || info.state == LOCK_UN) + g_error ("Cannot pop already unlocked repo lock"); + int state_to_drop; guint *counter; if (lock_type == OSTREE_REPO_LOCK_EXCLUSIVE) @@ -408,7 +412,9 @@ pop_repo_lock (OstreeRepo *self, } /* Make sure caller specified a valid type to release */ - g_assert_cmpuint (*counter, >, 0); + if (*counter == 0) + g_error ("Repo %s lock pop requested, but none have been taken", + lock_state_name (state_to_drop)); int next_state; if (info.len == 1) @@ -434,7 +440,7 @@ pop_repo_lock (OstreeRepo *self, else { /* We should never drop from shared to exclusive */ - g_return_val_if_fail (next_state == LOCK_SH, FALSE); + g_assert (next_state == LOCK_SH); g_debug ("Returning lock state to shared"); if (!do_repo_lock (self->lock.fd, next_state | flags)) return glnx_throw_errno_prefix (error, From 7271863fa038a3690ee49d15c0cf1111cff895ce Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 19 Apr 2021 10:03:57 -0600 Subject: [PATCH 6/7] test-concurrency: Lower lock timeout If there's a locking issue in this test, then it's likely not going to resolve after a few seconds of serializing access. Lower the default 30 second lock timeout to 5 seconds to prevent the test from hanging unnecessarily. (cherry picked from commit 055b263dee7a26ae085527a3a39d96e68e78a512) --- tests/test-concurrency.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test-concurrency.py b/tests/test-concurrency.py index e4ce21e9e..3679ddb63 100755 --- a/tests/test-concurrency.py +++ b/tests/test-concurrency.py @@ -40,11 +40,15 @@ def mktree(dname, serial=0): f.write('{} {} {}\n'.format(dname, serial, v)) subprocess.check_call(['ostree', '--repo=repo', 'init', '--mode=bare']) -# like the bit in libtest, but let's do it unconditionally since it's simpler, -# and we don't need xattr coverage for this with open('repo/config', 'a') as f: + # like the bit in libtest, but let's do it unconditionally since + # it's simpler, and we don't need xattr coverage for this f.write('disable-xattrs=true\n') + # Make any locking errors fail quickly instead of blocking the test + # for 30 seconds. + f.write('lock-timeout-secs=5\n') + def commit(v): tdir='tree{}'.format(v) cmd = ['ostree', '--repo=repo', 'commit', '--fsync=0', '-b', tdir, '--tree=dir='+tdir] From 2c228c8a171ce071e181331a03f6357ef25156e5 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 22 Apr 2021 09:10:15 -0600 Subject: [PATCH 7/7] tests: Add single process repo locking tests The semantics of multiple process locking are covered by test-concurrency.py, but the semantics of the repository locking from a single process aren't handled there. This checks how the repository locking is handled from a single thread with one OstreeRepo, a single thread with multiple OstreeRepos, and multiple threads sharing an OstreeRepo. (cherry picked from commit 06bb56be6d0376906433a8e34cb56bca85c39dc5) --- tests/test-repo.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 306 insertions(+) diff --git a/tests/test-repo.c b/tests/test-repo.c index ae221ac0a..0d79c1d7f 100644 --- a/tests/test-repo.c +++ b/tests/test-repo.c @@ -51,6 +51,29 @@ setup (Fixture *fixture, g_test_message ("Using temporary directory: %s", fixture->tmpdir.path); } +/* Common setup for locking tests. Create an archive repo in the tmpdir and + * set the locking timeout to 0 so lock failures don't block. + */ +static void +lock_setup (Fixture *fixture, + gconstpointer test_data) +{ + setup (fixture, test_data); + + g_autoptr(GError) error = NULL; + g_autoptr(OstreeRepo) repo = ostree_repo_create_at (fixture->tmpdir.fd, ".", + OSTREE_REPO_MODE_ARCHIVE, + NULL, + NULL, &error); + g_assert_no_error (error); + + /* Set the lock timeout to 0 so failures don't block the test */ + g_autoptr(GKeyFile) config = ostree_repo_copy_config (repo); + g_key_file_set_integer (config, "core", "lock-timeout-secs", 0); + ostree_repo_write_config (repo, config, &error); + g_assert_no_error (error); +} + static void teardown (Fixture *fixture, gconstpointer test_data) @@ -221,6 +244,277 @@ test_repo_autolock (Fixture *fixture, g_assert_no_error (error); } +/* Locking from single thread with a single OstreeRepo */ +static void +test_repo_lock_single (Fixture *fixture, + gconstpointer test_data) +{ + g_autoptr(GError) error = NULL; + g_autoptr(OstreeRepo) repo = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + + /* Single thread on a single repo can freely recurse in any state */ + ostree_repo_lock_push (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_push (repo, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_push (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); +} + +/* Unlocking without having ever locked */ +static void +test_repo_lock_unlock_never_locked (Fixture *fixture, + gconstpointer test_data) +{ + if (g_test_subprocess ()) + { + g_autoptr(GError) error = NULL; + g_autoptr(OstreeRepo) repo = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + + ostree_repo_lock_pop (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + + return; + } + + g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_assert_failed (); + g_test_trap_assert_stderr ("*ERROR*Cannot pop repo never locked repo lock\n"); +} + +/* Unlocking after already unlocked */ +static void +test_repo_lock_double_unlock (Fixture *fixture, + gconstpointer test_data) +{ + if (g_test_subprocess ()) + { + g_autoptr(GError) error = NULL; + g_autoptr(OstreeRepo) repo = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + + ostree_repo_lock_push (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + + return; + } + + g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_assert_failed (); + g_test_trap_assert_stderr ("*ERROR*Cannot pop already unlocked repo lock\n"); +} + +/* Unlocking the wrong type */ +static void +test_repo_lock_unlock_wrong_type (Fixture *fixture, + gconstpointer test_data) +{ + if (g_test_subprocess ()) + { + g_autoptr(GError) error = NULL; + g_autoptr(OstreeRepo) repo = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + + ostree_repo_lock_push (repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + + return; + } + + g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_assert_failed (); + g_test_trap_assert_stderr ("*ERROR*Repo exclusive lock pop requested, but none have been taken\n"); +} + +/* Locking with single thread and multiple OstreeRepos */ +static void +test_repo_lock_multi_repo (Fixture *fixture, + gconstpointer test_data) +{ + g_autoptr(GError) error = NULL; + + /* Open two OstreeRepo instances */ + g_autoptr(OstreeRepo) repo1 = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + g_autoptr(OstreeRepo) repo2 = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + + /* Single thread with multiple OstreeRepo's conflict */ + ostree_repo_lock_push (repo1, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_push (repo2, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_push (repo1, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK); + g_clear_error (&error); + ostree_repo_lock_pop (repo1, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo2, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + + /* Recursive lock should stay exclusive once acquired */ + ostree_repo_lock_push (repo1, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_push (repo1, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_push (repo2, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK); + g_clear_error (&error); + ostree_repo_lock_push (repo2, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK); + g_clear_error (&error); + ostree_repo_lock_pop (repo1, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + ostree_repo_lock_pop (repo1, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); +} + +/* Locking from multiple threads with a single OstreeRepo */ +typedef struct { + OstreeRepo *repo; + guint step; +} LockThreadData; + +static gpointer +lock_thread1 (gpointer thread_data) +{ + LockThreadData *data = thread_data; + g_autoptr(GError) error = NULL; + + /* Step 0: Take an exclusive lock */ + g_assert_cmpuint (data->step, ==, 0); + g_test_message ("Thread 1: Push exclusive lock"); + ostree_repo_lock_push (data->repo, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + data->step++; + + /* Step 2: Take a shared lock */ + while (data->step != 2) + g_thread_yield (); + g_test_message ("Thread 1: Push shared lock"); + ostree_repo_lock_push (data->repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + data->step++; + + /* Step 4: Pop both locks */ + while (data->step != 4) + g_thread_yield (); + g_test_message ("Thread 1: Pop shared lock"); + ostree_repo_lock_pop (data->repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + g_test_message ("Thread 1: Pop exclusive lock"); + ostree_repo_lock_pop (data->repo, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + data->step++; + + return NULL; +} + +static gpointer +lock_thread2 (gpointer thread_data) +{ + LockThreadData *data = thread_data; + g_autoptr(GError) error = NULL; + + /* Step 1: Wait for the other thread to acquire a lock and then take a + * shared lock. + */ + while (data->step != 1) + g_thread_yield (); + g_test_message ("Thread 2: Push shared lock"); + ostree_repo_lock_push (data->repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + data->step++; + + /* Step 6: Pop lock */ + while (data->step != 6) + g_thread_yield (); + g_test_message ("Thread 2: Pop shared lock"); + ostree_repo_lock_pop (data->repo, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + data->step++; + + return NULL; +} + +static void +test_repo_lock_multi_thread (Fixture *fixture, + gconstpointer test_data) +{ + g_autoptr(GError) error = NULL; + g_autoptr(OstreeRepo) repo1 = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + g_autoptr(OstreeRepo) repo2 = ostree_repo_open_at (fixture->tmpdir.fd, ".", + NULL, &error); + g_assert_no_error (error); + + LockThreadData thread_data = {repo1, 0}; + GThread *thread1 = g_thread_new ("lock-thread-1", lock_thread1, &thread_data); + GThread *thread2 = g_thread_new ("lock-thread-2", lock_thread2, &thread_data); + + /* Step 3: Try to take a shared lock on repo2. This should fail since + * thread1 still has an exclusive lock. + */ + while (thread_data.step != 3) + g_thread_yield (); + g_test_message ("Repo 2: Push failing shared lock"); + ostree_repo_lock_push (repo2, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK); + g_clear_error (&error); + thread_data.step++; + + /* Step 5: Try to a lock on repo2. A shared lock should succeed since + * thread1 has dropped its exclusive lock. + */ + while (thread_data.step != 5) + g_thread_yield (); + g_test_message ("Repo 2: Push shared lock"); + ostree_repo_lock_push (repo2, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + g_test_message ("Repo 2: Push failing exclusive lock"); + ostree_repo_lock_push (repo2, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK); + g_clear_error (&error); + thread_data.step++; + + /* Step 7: Now both threads have dropped their locks and taking an exclusive + * lock should succeed. + */ + while (thread_data.step != 7) + g_thread_yield (); + g_test_message ("Repo 2: Push exclusive lock"); + ostree_repo_lock_push (repo2, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + g_test_message ("Repo 2: Pop exclusive lock"); + ostree_repo_lock_pop (repo2, OSTREE_REPO_LOCK_EXCLUSIVE, NULL, &error); + g_assert_no_error (error); + g_test_message ("Repo 2: Pop shared lock"); + ostree_repo_lock_pop (repo2, OSTREE_REPO_LOCK_SHARED, NULL, &error); + g_assert_no_error (error); + thread_data.step++; + + g_thread_join (thread1); + g_thread_join (thread2); +} + int main (int argc, char **argv) @@ -238,6 +532,18 @@ main (int argc, test_repo_get_min_free_space, 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, + test_repo_lock_unlock_never_locked, teardown); + g_test_add ("/repo/lock/double-unlock", Fixture, NULL, lock_setup, + test_repo_lock_double_unlock, teardown); + g_test_add ("/repo/lock/unlock-wrong-type", Fixture, NULL, lock_setup, + test_repo_lock_unlock_wrong_type, teardown); + g_test_add ("/repo/lock/multi-repo", Fixture, NULL, lock_setup, + test_repo_lock_multi_repo, teardown); + g_test_add ("/repo/lock/multi-thread", Fixture, NULL, lock_setup, + test_repo_lock_multi_thread, teardown); return g_test_run (); }