From 8c1542134cf8a1a00d669c80da45c15c22fc0522 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 23 Apr 2018 17:53:04 -0400 Subject: [PATCH] lib/repo: Enable locking by default, but drop external API The code has been sitting around for a while but since I disabled it by default, I doubt anyone is really using it or relying on it. This patch and turns on locking by default, and also drops the API which was only public in the experimental API builds. Conceptually these are two distinct things, and we may actually want to split up the patches. I don't think this will break anyone, but it's hard to say for sure. It's also going to be hard to find out until we actually release I suspect... But anyone who is broken should be able to add `locking=false` into their repo config. On the flip side Endless has been shipping with this enabled and it is reported to help. The reason to drop the APIs: I'm a bit concerned about the interactions over time between libostree's use of the API and any apps that start using it. For example, if an app specifies a SHARED lock in their code, then later internally we decide to temporarily grab an `EXCLUSIVE`, but the app had a second thread/process that was `EXCLUSIVE` already, and that process was waiting on the first bit of code, then we could deadlock. I can't think of a real world situation where this would happen yet though. We are likely to in the future have say `fsck` take an external lock, `checkout` grab a shared one, etc. Closes: #1555 Approved by: jlebon --- apidoc/ostree-experimental-sections.txt | 6 --- src/libostree/libostree-experimental.sym | 4 -- src/libostree/ostree-autocleanups.h | 1 - src/libostree/ostree-repo-commit.c | 8 ++-- src/libostree/ostree-repo-private.h | 32 +++++++------- src/libostree/ostree-repo-prune.c | 9 ++-- src/libostree/ostree-repo.c | 53 +++++++++++------------- src/libostree/ostree-repo.h | 42 ------------------- tests/test-concurrency.py | 1 - 9 files changed, 48 insertions(+), 108 deletions(-) diff --git a/apidoc/ostree-experimental-sections.txt b/apidoc/ostree-experimental-sections.txt index 0d168406..60daaca5 100644 --- a/apidoc/ostree-experimental-sections.txt +++ b/apidoc/ostree-experimental-sections.txt @@ -90,12 +90,6 @@ ostree_repo_finder_override_get_type
ostree-misc-experimental -OstreeRepoLockType -ostree_repo_lock_push -ostree_repo_lock_pop -OstreeRepoAutoLock -ostree_repo_auto_lock_push -ostree_repo_auto_lock_cleanup ostree_repo_get_collection_id ostree_repo_set_collection_id ostree_validate_collection_id diff --git a/src/libostree/libostree-experimental.sym b/src/libostree/libostree-experimental.sym index 3f3454f3..b83ad1b0 100644 --- a/src/libostree/libostree-experimental.sym +++ b/src/libostree/libostree-experimental.sym @@ -94,8 +94,4 @@ LIBOSTREE_2017.14_EXPERIMENTAL { global: ostree_remote_get_type; ostree_remote_get_url; - ostree_repo_auto_lock_cleanup; - ostree_repo_auto_lock_push; - ostree_repo_lock_pop; - ostree_repo_lock_push; } LIBOSTREE_2017.13_EXPERIMENTAL; diff --git a/src/libostree/ostree-autocleanups.h b/src/libostree/ostree-autocleanups.h index 75b498fc..504954e0 100644 --- a/src/libostree/ostree-autocleanups.h +++ b/src/libostree/ostree-autocleanups.h @@ -61,7 +61,6 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeSysrootUpgrader, g_object_unref) G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC (OstreeRepoCommitTraverseIter, ostree_repo_commit_traverse_iter_clear) #ifdef OSTREE_ENABLE_EXPERIMENTAL_API -G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRepoAutoLock, ostree_repo_auto_lock_cleanup) G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeCollectionRef, ostree_collection_ref_free) G_DEFINE_AUTO_CLEANUP_FREE_FUNC (OstreeCollectionRefv, ostree_collection_ref_freev, NULL) G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRemote, ostree_remote_unref) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index c171b3da..6eb645be 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -1567,8 +1567,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; @@ -2136,7 +2136,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; } @@ -2189,7 +2189,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 3078a9e2..77203638 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -435,34 +435,36 @@ _ostree_repo_get_remote_inherited (OstreeRepo *self, const char *name, GError **error); -#ifndef OSTREE_ENABLE_EXPERIMENTAL_API - -/* All the locking APIs below are duplicated in ostree-repo.h. Remove the ones - * here once it's no longer experimental. +/* 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, +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); +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) +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) +#ifndef OSTREE_ENABLE_EXPERIMENTAL_API + +/* These APIs are duplicated in the public headers when doing an + * experimental-API build. + */ const gchar * ostree_repo_get_collection_id (OstreeRepo *self); gboolean ostree_repo_set_collection_id (OstreeRepo *self, const gchar *collection_id, diff --git a/src/libostree/ostree-repo-prune.c b/src/libostree/ostree-repo-prune.c index f0c0a974..2ffd6948 100644 --- a/src/libostree/ostree-repo-prune.c +++ b/src/libostree/ostree-repo-prune.c @@ -201,8 +201,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; @@ -340,8 +339,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; @@ -452,8 +450,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.c b/src/libostree/ostree-repo.c index 7d593f50..79006a6b 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -436,7 +436,7 @@ pop_repo_lock (OstreeRepo *self, return TRUE; } -/** +/* * ostree_repo_lock_push: * @self: a #OstreeRepo * @lock_type: the type of lock to acquire @@ -462,13 +462,12 @@ pop_repo_lock (OstreeRepo *self, * %TRUE is returned. * * Returns: %TRUE on success, otherwise %FALSE with @error set - * Since: 2017.14 */ 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); @@ -531,8 +530,8 @@ ostree_repo_lock_push (OstreeRepo *self, } } -/** - * ostree_repo_lock_pop: +/* + * _ostree_repo_lock_pop: * @self: a #OstreeRepo * @cancellable: a #GCancellable * @error: a #GError @@ -553,12 +552,11 @@ ostree_repo_lock_push (OstreeRepo *self, * %TRUE is returned. * * Returns: %TRUE on success, otherwise %FALSE with @error set - * Since: 2017.14 */ 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); @@ -621,8 +619,8 @@ 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 @@ -636,37 +634,34 @@ 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: 2017.14 */ 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: 2017.14 */ void -ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock) +_ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock) { OstreeRepo *repo = lock; if (repo) @@ -674,7 +669,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; @@ -2742,10 +2737,10 @@ reload_core_config (OstreeRepo *self, self->tmp_expiry_seconds = g_ascii_strtoull (tmp_expiry_seconds, NULL, 10); } - /* Disable locking by default for now */ { gboolean locking; + /* Enabled by default in 2018.05 */ if (!ot_keyfile_get_boolean_with_default (self->config, "core", "locking", - FALSE, &locking, error)) + TRUE, &locking, error)) return FALSE; if (!locking) { diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index 04b04416..8d3a7a6f 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -109,48 +109,6 @@ OstreeRepo * ostree_repo_create_at (int dfd, #ifdef OSTREE_ENABLE_EXPERIMENTAL_API -/** - * OstreeRepoLockType: - * @OSTREE_REPO_LOCK_SHARED: A shared lock - * @OSTREE_REPO_LOCK_EXCLUSIVE: An exclusive lock - * - * The type of repository lock to acquire. - * - * Since: 2017.14 - */ -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); - -/** - * OstreeRepoAutoLock: (skip) - * - * This is simply an alias to #OstreeRepo used for automatic lock cleanup. - * See ostree_repo_auto_lock_push() for its intended usage. - * - * Since: 2017.14 - */ -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); - _OSTREE_PUBLIC const gchar * ostree_repo_get_collection_id (OstreeRepo *self); _OSTREE_PUBLIC diff --git a/tests/test-concurrency.py b/tests/test-concurrency.py index 3ec3681c..e4ce21e9 100755 --- a/tests/test-concurrency.py +++ b/tests/test-concurrency.py @@ -44,7 +44,6 @@ subprocess.check_call(['ostree', '--repo=repo', 'init', '--mode=bare']) # and we don't need xattr coverage for this with open('repo/config', 'a') as f: f.write('disable-xattrs=true\n') - f.write('locking=true\n') def commit(v): tdir='tree{}'.format(v)