From e297478fb39fa78d829eaf4b7cee72c102764f3f Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 16 Oct 2017 15:51:40 +0000 Subject: [PATCH 01/28] build: Define OSTREE_ENABLE_EXPERIMENTAL_API for g-ir-scanner When compiling libostree, OSTREE_ENABLE_EXPERIMENTAL_API is managed via config.h. However, g-ir-scanner doesn't use that and won't include the experimental APIs unless the macro is defined through another means. Without this, none of the experimental APIs were being included in the gir data. --- Makefile-libostree.am | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Makefile-libostree.am b/Makefile-libostree.am index e2ebae3a80..42b8bcecec 100644 --- a/Makefile-libostree.am +++ b/Makefile-libostree.am @@ -261,6 +261,10 @@ OSTree-1.0.gir: libostree-1.la Makefile OSTree_1_0_gir_EXPORT_PACKAGES = ostree-1 OSTree_1_0_gir_INCLUDES = Gio-2.0 OSTree_1_0_gir_CFLAGS = $(libostree_1_la_CFLAGS) +if ENABLE_EXPERIMENTAL_API +# When compiling this is set via config.h, but g-ir-scanner doesn't use that +OSTree_1_0_gir_CFLAGS += -DOSTREE_ENABLE_EXPERIMENTAL_API=1 +endif OSTree_1_0_gir_LIBS = libostree-1.la OSTree_1_0_gir_SCANNERFLAGS = --warn-all --identifier-prefix=Ostree --symbol-prefix=ostree $(GI_SCANNERFLAGS) OSTree_1_0_gir_FILES = $(libostreeinclude_HEADERS) $(filter-out %-private.h %/ostree-soup-uri.h $(libostree_experimental_headers),$(libostree_1_la_SOURCES)) From 07b17fb476615cf57708fce1bbc5b21da4782597 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 19 Oct 2017 19:18:44 +0000 Subject: [PATCH 02/28] Skip cmdprivate components in GIR Both of these were inadvertently being included in the GIR. The OstreeCmdPrivateVTable declaration ended up causing a g-ir-compiler error because I wanted to add a function that had a return type that was marked as skipped (OstreeRepoAutoLock). The ostree_cmd__private__ documentation just had the wrong name. --- src/libostree/ostree-cmdprivate.c | 2 +- src/libostree/ostree-cmdprivate.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-cmdprivate.c b/src/libostree/ostree-cmdprivate.c index 3e60b125a4..d0c7e98e8e 100644 --- a/src/libostree/ostree-cmdprivate.c +++ b/src/libostree/ostree-cmdprivate.c @@ -35,7 +35,7 @@ impl_ostree_generate_grub2_config (OstreeSysroot *sysroot, int bootversion, int } /** - * ostree_cmdprivate: (skip) + * ostree_cmd__private__: (skip) * * Do not call this function; it is used to share private API between * the OSTree commandline and the library. diff --git a/src/libostree/ostree-cmdprivate.h b/src/libostree/ostree-cmdprivate.h index f636ab15e6..4e49cef39a 100644 --- a/src/libostree/ostree-cmdprivate.h +++ b/src/libostree/ostree-cmdprivate.h @@ -25,6 +25,12 @@ G_BEGIN_DECLS gboolean _ostree_impl_system_generator (const char *ostree_cmdline, const char *normal_dir, const char *early_dir, const char *late_dir, GError **error); +/** + * OstreeCmdPrivateVTable: (skip) + * + * Function table to share private API between the OSTree commandline and the + * library. Don't use this. + */ typedef struct { gboolean (* ostree_system_generator) (const char *ostree_cmdline, const char *normal_dir, const char *early_dir, const char *late_dir, GError **error); gboolean (* ostree_generate_grub2_config) (OstreeSysroot *sysroot, int bootversion, int target_fd, GCancellable *cancellable, GError **error); From 50bfa38360347c57e511ff2d4d6bd66305b8b4cc Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 5 Oct 2017 15:25:11 -0500 Subject: [PATCH 03/28] lib/repo: Add repo locking mechanism Currently ostree has no method of guarding against concurrent pruning. When there are multiple repo writers, it's possible to have a pull or commit race against a prune and end up with missing objects. This adds an file based repo locking mechanism. The intention is to take a shared lock when writing objects and an exclusive lock when deleting them. In order to make use of the locking throughout the library in a fine grained fashion, the lock acts recursively with a stack of lock states. If the lock becomes exclusive, it will stay in that state until the stack is unwound past the initial exclusive push. The file locking is similar to GLnxLockFile in that it uses open file descriptor locks but falls back to flock when needed. The lock also attempts to be thread safe by storing the lock state in thread local storage with GPrivate. This means that each thread will have an independent lock for each repository it opens. There are some drawbacks to that, but it seemed impossible to manage the lock state coherently in the face of multithreaded access. The API is a push/pop interface in accordance with the recursive nature of the locking. The push interface uses an enum that's translated to LOCK_SH or LOCK_EX as needed. Both interfaces use an internal timeout field to decide whether to manage the lock in a blocking or non-blocking fashion. The intention is to allow ostree applications as well as administrators to control this timeout. For now, the default is a 30 second timeout. Note that the timeout is handled synchronously in thread since the lock is maintained in thread local storage. I.e., the thread that acquires the lock needs to be the same thread that runs the operation. There may be a way to offer an asynchronous version, but it's not clear exactly how that would work since it would likely involve a separate thread that invokes a callback when the locking operation completes. https://bugzilla.gnome.org/show_bug.cgi?id=759442 --- apidoc/ostree-experimental-sections.txt | 2 + src/libostree/libostree-experimental.sym | 2 + src/libostree/ostree-repo-private.h | 16 + src/libostree/ostree-repo.c | 453 +++++++++++++++++++++++ src/libostree/ostree-repo.h | 21 ++ 5 files changed, 494 insertions(+) diff --git a/apidoc/ostree-experimental-sections.txt b/apidoc/ostree-experimental-sections.txt index 309d07fb3a..5859fa512e 100644 --- a/apidoc/ostree-experimental-sections.txt +++ b/apidoc/ostree-experimental-sections.txt @@ -87,6 +87,8 @@ ostree_repo_finder_override_get_type
ostree-misc-experimental +ostree_repo_lock_push +ostree_repo_lock_pop 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 cbe373cf66..d02efa3bb8 100644 --- a/src/libostree/libostree-experimental.sym +++ b/src/libostree/libostree-experimental.sym @@ -88,4 +88,6 @@ global: ostree_repo_finder_override_add_uri; ostree_repo_finder_override_get_type; ostree_repo_finder_override_new; + ostree_repo_lock_pop; + ostree_repo_lock_push; } LIBOSTREE_2017.12_EXPERIMENTAL; diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index d8d16e1a53..20fb35e343 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -37,6 +37,8 @@ G_BEGIN_DECLS #define _OSTREE_MAX_OUTSTANDING_FETCHER_REQUESTS 8 #define _OSTREE_MAX_OUTSTANDING_DELTAPART_REQUESTS 2 +#define _OSTREE_DEFAULT_LOCK_TIMEOUT_SECONDS 30 + /* In most cases, writing to disk should be much faster than * fetching from the network, so we shouldn't actually hit * this. But if using pipelining and e.g. pulling over LAN @@ -153,6 +155,7 @@ struct OstreeRepo { guint64 tmp_expiry_seconds; gchar *collection_id; gboolean add_remotes_config_dir; /* Add new remotes in remotes.d dir */ + gint lock_timeout_seconds; OstreeRepo *parent_repo; }; @@ -415,6 +418,19 @@ _ostree_repo_get_remote_inherited (OstreeRepo *self, #ifndef OSTREE_ENABLE_EXPERIMENTAL_API +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); + 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.c b/src/libostree/ostree-repo.c index 7ee7892a16..a5a094975c 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -156,6 +156,454 @@ G_DEFINE_TYPE (OstreeRepo, ostree_repo, G_TYPE_OBJECT) #define SYSCONF_REMOTES SHORTENED_SYSCONFDIR "/ostree/remotes.d" +/* 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 + * 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 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. + * + * 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. + */ + +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; + GQueue *stack; +} OstreeRepoLock; + +static void +free_repo_lock (gpointer data) +{ + OstreeRepoLock *lock = data; + + if (lock != NULL) + { + guint stack_len; + const char *cur_state_name; + + if (lock->stack) + { + int cur_state; + + stack_len = g_queue_get_length (lock->stack); + if (stack_len == 0) + { + cur_state = LOCK_UN; + cur_state_name = "unlocked"; + } + else + { + cur_state = GPOINTER_TO_INT (g_queue_peek_head (lock->stack)); + cur_state_name = (cur_state == LOCK_EX) ? "exclusive" : "shared"; + } + } + else + { + stack_len = 0; + cur_state_name = "null"; + } + + g_debug ("Free lock: state=%s, depth=%u", cur_state_name, stack_len); + g_queue_free (lock->stack); + 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, + int flags) +{ + int res; + +#ifdef F_OFD_SETLK + struct flock fl = { + .l_type = (flags & ~LOCK_NB) == LOCK_EX ? F_WRLCK : F_RDLCK, + .l_whence = SEEK_SET, + }; + + res = fcntl (fd, (flags & LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW, &fl); +#else + res = -1; + errno = EINVAL; +#endif + + /* Fallback to flock when OFD locks not available */ + if (res < 0) + { + if (errno == EINVAL) + res = flock (fd, flags); + if (res < 0) + return FALSE; + } + + return TRUE; +} + +/* Wrapper to handle flock vs OFD unlocking based on GLnxLockFile */ +static gboolean +do_repo_unlock(int fd, + int flags) +{ + int res; + +#ifdef F_OFD_SETLK + struct flock fl = { + .l_type = F_UNLCK, + .l_whence = SEEK_SET, + }; + + res = fcntl (fd, (flags & LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW, &fl); +#else + res = -1; + errno = EINVAL; +#endif + + /* Fallback to flock when OFD locks not available */ + if (res < 0) + { + if (errno == EINVAL) + res = flock (fd, LOCK_UN | flags); + if (res < 0) + return FALSE; + } + + return TRUE; +} + +static gboolean +push_repo_lock (OstreeRepo *self, + OstreeRepoLockType lock_type, + gboolean blocking, + GError **error) +{ + int flags = (lock_type == OSTREE_REPO_LOCK_EXCLUSIVE) ? LOCK_EX : LOCK_SH; + 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); + } + + OstreeRepoLock *lock = g_hash_table_lookup (lock_table, self); + if (lock == NULL) + { + lock = g_new (OstreeRepoLock, 1); + lock->stack = g_queue_new (); + g_debug ("Opening repo lock file"); + lock->fd = openat (self->repo_dir_fd, ".lock", + O_CREAT | O_RDWR | O_CLOEXEC, 0600); + 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); + } + + guint stack_len = g_queue_get_length (lock->stack); + int cur_state; + const char *cur_state_name; + if (stack_len == 0) + { + cur_state = LOCK_UN; + cur_state_name = "unlocked"; + } + else + { + cur_state = GPOINTER_TO_INT (g_queue_peek_head (lock->stack)); + cur_state_name = (cur_state == LOCK_EX) ? "exclusive" : "shared"; + } + g_debug ("Push lock: state=%s, depth=%u", cur_state_name, stack_len); + + if (cur_state == LOCK_EX) + { + g_debug ("Repo already locked exclusively, extending stack"); + g_queue_push_head (lock->stack, GINT_TO_POINTER (LOCK_EX)); + } + else + { + int next_state = (flags & LOCK_EX) ? LOCK_EX : LOCK_SH; + const char *next_state_name = (flags & LOCK_EX) ? "exclusive" : "shared"; + + 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)); + } + + return TRUE; +} + +static gboolean +pop_repo_lock (OstreeRepo *self, + gboolean blocking, + GError **error) +{ + 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->stack != NULL, FALSE); + g_return_val_if_fail (lock->fd != -1, FALSE); + + guint stack_len = g_queue_get_length (lock->stack); + g_return_val_if_fail (stack_len > 0, FALSE); + + int cur_state = GPOINTER_TO_INT (g_queue_peek_head (lock->stack)); + g_debug ("Pop lock: state=%s, depth=%u", + (cur_state == LOCK_EX) ? "exclusive" : "shared", + stack_len); + if (stack_len > 1) + { + int next_state = GPOINTER_TO_INT (g_queue_peek_nth (lock->stack, 1)); + + /* Drop back to the previous lock state if it differs */ + if (next_state != cur_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", + (next_state == LOCK_EX) ? "exclusive" : "shared"); + } + else + { + /* 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"); + } + + g_queue_pop_head (lock->stack); + + return TRUE; +} + +/** + * ostree_repo_lock_push + * @self: a #OstreeRepo + * @lock_type: the type of lock to acquire + * @cancellable: a #GCancellable + * @error: a #GError + * + * Takes a lock on the repository and adds it to the lock stack. 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. shared lock is + * used. 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 unchanged or would represent a downgrade (exclusive + * to shared), the lock state is not changed and the stack is simply updated. + * + * ostree_repo_lock_push() waits for the lock depending on the repository's + * lock-timeout configuration. When lock-timeout is -1, a blocking lock is + * attempted. Otherwise, the lock is taken non-blocking and + * ostree_repo_lock_push() will sleep synchronously up to lock-timeout seconds + * attempting to acquire the lock. If the lock cannot be acquired within the + * timeout, a %G_IO_ERROR_WOULD_BLOCK error is returned. + * + * If @self is not writable by the user, then no locking is attempted and + * %TRUE is returned. + * + * Returns: %TRUE on success, otherwise %FALSE with @error set + * Since: 2017.13 + */ +gboolean +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 (self->inited, FALSE); + + if (!self->writable) + return TRUE; + + if (self->lock_timeout_seconds == -1) + { + g_debug ("Pushing lock blocking"); + return push_repo_lock (self, lock_type, TRUE, error); + } + else + { + gint waited = 0; + g_debug ("Pushing lock non-blocking with timeout %d", + self->lock_timeout_seconds); + for (;;) + { + if (g_cancellable_set_error_if_cancelled (cancellable, error)) + return FALSE; + + g_autoptr(GError) local_error = NULL; + if (push_repo_lock (self, lock_type, FALSE, &local_error)) + return TRUE; + + if (!g_error_matches (local_error, G_IO_ERROR, + G_IO_ERROR_WOULD_BLOCK)) + { + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; + } + + if (waited >= self->lock_timeout_seconds) + { + g_debug ("Push lock: Could not acquire lock within %d seconds", + self->lock_timeout_seconds); + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; + } + + /* Sleep 1 second and try again */ + if (waited % 60 == 0) + { + gint remaining = self->lock_timeout_seconds - waited; + g_debug ("Push lock: Waiting %d more second%s to acquire lock", + remaining, (remaining == 1) ? "" : "s"); + } + waited++; + sleep (1); + } + } +} + +/** + * ostree_repo_lock_pop + * @self: a #OstreeRepo + * @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. + * + * ostree_repo_lock_pop() waits for the lock depending on the repository's + * lock-timeout configuration. When lock-timeout is -1, a blocking lock is + * attempted. Otherwise, the lock is removed non-blocking and + * ostree_repo_lock_pop() will sleep synchronously up to lock-timeout seconds + * attempting to remove the lock. If the lock cannot be removed within the + * timeout, a %G_IO_ERROR_WOULD_BLOCK error is returned. + * + * If @self is not writable by the user, then no unlocking is attempted and + * %TRUE is returned. + * + * Returns: %TRUE on success, otherwise %FALSE with @error set + * Since: 2017.13 + */ +gboolean +ostree_repo_lock_pop (OstreeRepo *self, + GCancellable *cancellable, + GError **error) +{ + g_return_val_if_fail (self != NULL, FALSE); + g_return_val_if_fail (self->inited, FALSE); + + if (!self->writable) + return TRUE; + + if (self->lock_timeout_seconds == -1) + { + g_debug ("Popping lock blocking"); + return pop_repo_lock (self, TRUE, error); + } + else + { + gint waited = 0; + g_debug ("Popping lock non-blocking with timeout %d", + self->lock_timeout_seconds); + for (;;) + { + if (g_cancellable_set_error_if_cancelled (cancellable, error)) + return FALSE; + + g_autoptr(GError) local_error = NULL; + if (pop_repo_lock (self, FALSE, &local_error)) + return TRUE; + + if (!g_error_matches (local_error, G_IO_ERROR, + G_IO_ERROR_WOULD_BLOCK)) + { + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; + } + + if (waited >= self->lock_timeout_seconds) + { + g_debug ("Pop lock: Could not remove lock within %d seconds", + self->lock_timeout_seconds); + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; + } + + /* Sleep 1 second and try again */ + if (waited % 60 == 0) + { + gint remaining = self->lock_timeout_seconds - waited; + g_debug ("Pop lock: Waiting %d more second%s to remove lock", + remaining, (remaining == 1) ? "" : "s"); + } + waited++; + sleep (1); + } + } +} + static GFile * get_remotes_d_dir (OstreeRepo *self, GFile *sysroot); @@ -517,6 +965,10 @@ 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); + G_OBJECT_CLASS (ostree_repo_parent_class)->finalize (object); } @@ -685,6 +1137,7 @@ ostree_repo_init (OstreeRepo *self) self->objects_dir_fd = -1; self->uncompressed_objects_dir_fd = -1; self->sysroot_kind = OSTREE_REPO_SYSROOT_KIND_UNKNOWN; + self->lock_timeout_seconds = _OSTREE_DEFAULT_LOCK_TIMEOUT_SECONDS; } /** diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index 15e5f94e8c..26fa4e695a 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -107,6 +107,27 @@ OstreeRepo * ostree_repo_create_at (int dfd, #ifdef OSTREE_ENABLE_EXPERIMENTAL_API +/** + * OstreeRepoLockType: + * The type of repository lock to acquire. + * @OSTREE_REPO_LOCK_SHARED: A shared lock + * @OSTREE_REPO_LOCK_EXCLUSIVE: An exclusive lock + */ +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); + _OSTREE_PUBLIC const gchar * ostree_repo_get_collection_id (OstreeRepo *self); _OSTREE_PUBLIC From bb6ab8e7259fb5cb23bb14b120bc07d7ab1cebe5 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 13 Oct 2017 19:31:35 +0000 Subject: [PATCH 04/28] lib/repo: Add locking auto cleanup handler Define an auto cleanup handler for use with repo locking. This is based on the existing auto transaction cleanup. A wrapper for ostree_repo_lock_push() is added with it. The intended usage is like so: g_autoptr(OstreeRepoAutoLock) lock = NULL; lock = ostree_repo_auto_lock_push (repo, lock_type, cancellable, error); if (!lock) return FALSE; The functions and type are marked to be skipped by introspection since I can't see them being usable from bindings. --- apidoc/ostree-experimental-sections.txt | 3 ++ src/libostree/libostree-experimental.sym | 2 + src/libostree/ostree-autocleanups.h | 1 + src/libostree/ostree-repo-private.h | 9 ++++ src/libostree/ostree-repo.c | 59 ++++++++++++++++++++++++ src/libostree/ostree-repo.h | 18 ++++++++ 6 files changed, 92 insertions(+) diff --git a/apidoc/ostree-experimental-sections.txt b/apidoc/ostree-experimental-sections.txt index 5859fa512e..0e85d095cb 100644 --- a/apidoc/ostree-experimental-sections.txt +++ b/apidoc/ostree-experimental-sections.txt @@ -89,6 +89,9 @@ ostree_repo_finder_override_get_type ostree-misc-experimental 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 d02efa3bb8..b8aa277336 100644 --- a/src/libostree/libostree-experimental.sym +++ b/src/libostree/libostree-experimental.sym @@ -88,6 +88,8 @@ global: ostree_repo_finder_override_add_uri; ostree_repo_finder_override_get_type; ostree_repo_finder_override_new; + ostree_repo_auto_lock_cleanup; + ostree_repo_auto_lock_push; ostree_repo_lock_pop; ostree_repo_lock_push; } LIBOSTREE_2017.12_EXPERIMENTAL; diff --git a/src/libostree/ostree-autocleanups.h b/src/libostree/ostree-autocleanups.h index c8e8a8572d..b3974317fa 100644 --- a/src/libostree/ostree-autocleanups.h +++ b/src/libostree/ostree-autocleanups.h @@ -59,6 +59,7 @@ 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-private.h b/src/libostree/ostree-repo-private.h index 20fb35e343..cb349d008d 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -431,6 +431,15 @@ 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) + 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.c b/src/libostree/ostree-repo.c index a5a094975c..749d229156 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -604,6 +604,65 @@ ostree_repo_lock_pop (OstreeRepo *self, } } +/** + * 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. + * + * |[ + * g_autoptr(OstreeRepoAutoLock) lock = NULL; + * lock = ostree_repo_auto_lock_push (repo, lock_type, cancellable, error); + * if (!lock) + * return FALSE; + * ]| + * + * Returns: @repo on success, otherwise %NULL with @error set + * Since: 2017.13 + */ +OstreeRepoAutoLock * +ostree_repo_auto_lock_push (OstreeRepo *self, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error) +{ + if (!ostree_repo_lock_push (self, lock_type, cancellable, error)) + return NULL; + return (OstreeRepoAutoLock *)self; +} + +/** + * 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. + * + * Since: 2017.13 + */ +void +ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock) +{ + OstreeRepo *repo = lock; + if (repo) + { + g_autoptr(GError) error = NULL; + int errsv = errno; + + if (!ostree_repo_lock_pop (repo, NULL, &error)) + g_critical ("Cleanup repo lock failed: %s", error->message); + + errno = errsv; + } +} + static GFile * get_remotes_d_dir (OstreeRepo *self, GFile *sysroot); diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index 26fa4e695a..e891819897 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -128,6 +128,24 @@ 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.13 + */ +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 From 7be3051b3c34730dcc0c8a229e97efa7fbf5e7c1 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 6 Oct 2017 10:56:09 +0000 Subject: [PATCH 05/28] lib/commit: Add repository locking during transactions Take a shared repo lock during a transaction to ensure that another process doesn't delete objects. --- src/libostree/ostree-repo-commit.c | 23 ++++++++++++++++++++++- src/libostree/ostree-repo-private.h | 1 + 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index cf1a513f03..54ffe4ad6c 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -1203,7 +1203,9 @@ ostree_repo_scan_hardlinks (OstreeRepo *self, * ostree_repo_abort_transaction(). * * Currently, transactions are not atomic, and aborting a transaction - * will not erase any data you write during the transaction. + * will not erase any data you write during the transaction. + * + * This function takes a shared lock on the @self repository. */ gboolean ostree_repo_prepare_transaction (OstreeRepo *self, @@ -1214,6 +1216,11 @@ ostree_repo_prepare_transaction (OstreeRepo *self, g_return_val_if_fail (self->in_transaction == FALSE, FALSE); + self->txn_locked = ostree_repo_lock_push (self, OSTREE_REPO_LOCK_SHARED, + cancellable, error); + if (!self->txn_locked) + return FALSE; + memset (&self->txn_stats, 0, sizeof (OstreeRepoTransactionStats)); self->in_transaction = TRUE; @@ -1699,6 +1706,13 @@ ostree_repo_commit_transaction (OstreeRepo *self, if (!ot_ensure_unlinked_at (self->repo_dir_fd, "transaction", 0)) return FALSE; + if (self->txn_locked) + { + if (!ostree_repo_lock_pop (self, cancellable, error)) + return FALSE; + self->txn_locked = FALSE; + } + if (out_stats) *out_stats = self->txn_stats; @@ -1739,6 +1753,13 @@ ostree_repo_abort_transaction (OstreeRepo *self, self->in_transaction = FALSE; + if (self->txn_locked) + { + if (!ostree_repo_lock_pop (self, cancellable, error)) + return FALSE; + self->txn_locked = FALSE; + } + return TRUE; } diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index cb349d008d..5d2d712c65 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -111,6 +111,7 @@ struct OstreeRepo { GWeakRef sysroot; /* Weak to avoid a circular ref; see also `is_system` */ char *remotes_config_dir; + gboolean txn_locked; GHashTable *txn_refs; /* (element-type utf8 utf8) */ GHashTable *txn_collection_refs; /* (element-type OstreeCollectionRef utf8) */ GMutex txn_stats_lock; From d98bc522c3c465abe42fa7db9af37618b3e4c575 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 6 Oct 2017 11:04:22 +0000 Subject: [PATCH 06/28] lib/prune: Take exclusive repository lock Add exclusive repository locking to all the pruning entry points. This ensures that objects and deltas will not be removed while another process is writing to the repository. --- src/libostree/ostree-repo-prune.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/libostree/ostree-repo-prune.c b/src/libostree/ostree-repo-prune.c index 88c52abfe3..dba80fda56 100644 --- a/src/libostree/ostree-repo-prune.c +++ b/src/libostree/ostree-repo-prune.c @@ -23,6 +23,7 @@ #include "ostree-core-private.h" #include "ostree-repo-private.h" +#include "ostree-autocleanups.h" #include "otutil.h" typedef struct { @@ -160,12 +161,20 @@ _ostree_repo_prune_tmp (OstreeRepo *self, * Prune static deltas, if COMMIT is specified then delete static delta files only * targeting that commit; otherwise any static delta of non existing commits are * deleted. + * + * This function takes an exclusive lock on the @self repository. */ gboolean ostree_repo_prune_static_deltas (OstreeRepo *self, const char *commit, GCancellable *cancellable, GError **error) { + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); + if (!lock) + return FALSE; + g_autoptr(GPtrArray) deltas = NULL; if (!ostree_repo_list_static_delta_names (self, &deltas, cancellable, error)) @@ -286,6 +295,8 @@ repo_prune_internal (OstreeRepo *self, * Use the %OSTREE_REPO_PRUNE_FLAGS_NO_PRUNE to just determine * statistics on objects that would be deleted, without actually * deleting them. + * + * This function takes an exclusive lock on the @self repository. */ gboolean ostree_repo_prune (OstreeRepo *self, @@ -297,6 +308,12 @@ ostree_repo_prune (OstreeRepo *self, GCancellable *cancellable, GError **error) { + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); + if (!lock) + return FALSE; + g_autoptr(GHashTable) objects = NULL; gboolean refs_only = flags & OSTREE_REPO_PRUNE_FLAGS_REFS_ONLY; @@ -391,6 +408,8 @@ ostree_repo_prune (OstreeRepo *self, * * The %OSTREE_REPO_PRUNE_FLAGS_NO_PRUNE flag may be specified to just determine * statistics on objects that would be deleted, without actually deleting them. + * + * This function takes an exclusive lock on the @self repository. */ gboolean ostree_repo_prune_from_reachable (OstreeRepo *self, @@ -401,6 +420,12 @@ ostree_repo_prune_from_reachable (OstreeRepo *self, GCancellable *cancellable, GError **error) { + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); + if (!lock) + return FALSE; + g_autoptr(GHashTable) objects = NULL; if (!ostree_repo_list_objects (self, OSTREE_REPO_LIST_OBJECTS_ALL | OSTREE_REPO_LIST_OBJECTS_NO_PARENTS, From 19a4dbfb208d13057e84e02f0300cfb5297db73d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 13 Oct 2017 19:50:28 -0500 Subject: [PATCH 07/28] tests: Test concurrent operations Test that concurrent commits and prunes can succeed. Mostly this is a check that the new locking works correctly and the concurrent processes will properly wait until they've acquired the appropriate repository lock. --- Makefile-tests.am | 1 + tests/test-concurrency.py | 93 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100755 tests/test-concurrency.py diff --git a/Makefile-tests.am b/Makefile-tests.am index 42e73fa09f..cb2c2c97c1 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -106,6 +106,7 @@ _installed_or_uninstalled_test_scripts = \ tests/test-xattrs.sh \ tests/test-auto-summary.sh \ tests/test-prune.sh \ + tests/test-concurrency.py \ tests/test-refs.sh \ tests/test-demo-buildsystem.sh \ tests/test-switchroot.sh \ diff --git a/tests/test-concurrency.py b/tests/test-concurrency.py new file mode 100755 index 0000000000..c397308b5a --- /dev/null +++ b/tests/test-concurrency.py @@ -0,0 +1,93 @@ +#!/usr/bin/env python +# +# Copyright (C) 2017 Colin Walters +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the +# Free Software Foundation, Inc., 59 Temple Place - Suite 330, +# Boston, MA 02111-1307, USA. + +from __future__ import print_function +import os +import sys +import shutil +import subprocess +from multiprocessing import cpu_count + +def fatal(msg): + sys.stderr.write(msg) + sys.stderr.write('\n') + sys.exit(1) + +# Create 20 files with content based on @dname + a serial, basically to have +# different files with different checksums. +def mktree(dname, serial=0): + print('Creating tree', dname, file=sys.stderr) + os.mkdir(dname, 0755) + for v in xrange(20): + with open('{}/{}'.format(dname, v), 'w') as f: + 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: + f.write('disable-xattrs=true\n') + +def commit(v): + tdir='tree{}'.format(v) + cmd = ['ostree', '--repo=repo', 'commit', '--fsync=0', '-b', tdir, '--tree=dir='+tdir] + proc = subprocess.Popen(cmd) + print('PID {}'.format(proc.pid), *cmd, file=sys.stderr) + return proc +def prune(): + cmd = ['ostree', '--repo=repo', 'prune', '--refs-only'] + proc = subprocess.Popen(cmd) + print('PID {}:'.format(proc.pid), *cmd, file=sys.stderr) + return proc + +def wait_check(proc): + proc.wait() + if proc.returncode != 0: + fatal("process {} exited with code {}".format(proc.pid, proc.returncode)) + +print("1..2") + +def run(n_committers, n_pruners): + committers = set() + pruners = set() + + print('n_committers', n_committers, 'n_pruners', n_pruners, file=sys.stderr) + n_trees = n_committers / 2 + for v in xrange(n_trees): + mktree('tree{}'.format(v)) + + for v in xrange(n_committers): + committers.add(commit(v / 2)) + for v in xrange(n_pruners): + pruners.add(prune()) + + for committer in committers: + wait_check(committer) + for pruner in pruners: + wait_check(pruner) + + for v in xrange(n_trees): + shutil.rmtree('tree{}'.format(v)) + +# No concurrent pruning +run(cpu_count()/2 + 2, 0) +print("ok no concurrent prunes") + +run(cpu_count()/2 + 4, 3) +print("ok concurrent prunes") From d613ae63ce3e02b88795fe192de36090193f9c48 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 13 Oct 2017 19:52:04 -0500 Subject: [PATCH 08/28] tests: Run python tests with stdout unbuffered Set the PYTHONUNBUFFERED environment variable during tests so that python leaves stdout unbuffered. This is helpful when reading logs for failures since the interleaved stdout and stderr will generally come out in the right order. It's not perfect since tap-driver.sh does some special redirection to the log file, but it's an improvement. --- Makefile-tests.am | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile-tests.am b/Makefile-tests.am index cb2c2c97c1..30c59585da 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -35,6 +35,7 @@ AM_TESTS_ENVIRONMENT += OT_TESTS_DEBUG=1 \ LD_LIBRARY_PATH=$$(cd $(top_builddir)/.libs && pwd)$${LD_LIBRARY_PATH:+:$${LD_LIBRARY_PATH}} \ PATH=$$(cd $(top_builddir)/tests && pwd):$${PATH} \ OSTREE_FEATURES="$(OSTREE_FEATURES)" \ + PYTHONUNBUFFERED=1 \ $(NULL) if BUILDOPT_ASAN AM_TESTS_ENVIRONMENT += OT_SKIP_READDIR_RAND=1 G_SLICE=always-malloc From e9b586e2eb63f8b390f0304587ad134ddcf9ec2f Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 6 Oct 2017 15:48:30 +0000 Subject: [PATCH 09/28] lib/checkout: Lock repository during checkout operations When checking out objects, take a shared lock to ensure objects aren't deleted. If the repo isn't writable, the locking is a noop. This ensures that the repository owner can make robust checkouts, but it still allows other users to make a likely to be successful checkout if the repo mode supports it. During garbage collection, take an exclusive lock while deleting uncompressed objects. --- src/libostree/ostree-repo-checkout.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/libostree/ostree-repo-checkout.c b/src/libostree/ostree-repo-checkout.c index 962b503d12..8e8dac6276 100644 --- a/src/libostree/ostree-repo-checkout.c +++ b/src/libostree/ostree-repo-checkout.c @@ -31,6 +31,7 @@ #include "ostree-sepolicy-private.h" #include "ostree-core-private.h" #include "ostree-repo-private.h" +#include "ostree-autocleanups.h" #define WHITEOUT_PREFIX ".wh." @@ -991,6 +992,16 @@ checkout_tree_at (OstreeRepo *self, g_assert (options->force_copy); } + /* Take a shared repo lock to try to ensure objects aren't deleted. If the + * repo is not writable, this will be a noop and we just hope for the + * best... + */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_SHARED, + cancellable, error); + if (!lock) + return FALSE; + /* Special case handling for subpath of a non-directory */ if (g_file_info_get_file_type (source_info) != G_FILE_TYPE_DIRECTORY) { @@ -1062,6 +1073,8 @@ canonicalize_options (OstreeRepo *self, * physical filesystem. @source may be any subdirectory of a given * commit. The @mode and @overwrite_mode allow control over how the * files are checked out. + * + * This function takes a shared lock on the @self repository. */ gboolean ostree_repo_checkout_tree (OstreeRepo *self, @@ -1105,6 +1118,8 @@ ostree_repo_checkout_tree (OstreeRepo *self, * default is not to use the repository-internal uncompressed objects * cache. * + * This function takes a shared lock on the @self repository. + * * This function is deprecated. Use ostree_repo_checkout_at() instead. */ gboolean @@ -1150,6 +1165,8 @@ ostree_repo_checkout_tree_at (OstreeRepo *self, * Note in addition that unlike ostree_repo_checkout_tree(), the * default is not to use the repository-internal uncompressed objects * cache. + * + * This function takes a shared lock on the @self repository. */ gboolean ostree_repo_checkout_at (OstreeRepo *self, @@ -1273,12 +1290,21 @@ ostree_repo_devino_cache_new (void) * Call this after finishing a succession of checkout operations; it * will delete any currently-unused uncompressed objects from the * cache. + * + * This function takes an exclusive lock on the @self repository. */ gboolean ostree_repo_checkout_gc (OstreeRepo *self, GCancellable *cancellable, GError **error) { + /* Take an exclusive repo lock while deleting uncompressed objects */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); + if (!lock) + return FALSE; + g_autoptr(GHashTable) to_clean_dirs = NULL; g_mutex_lock (&self->cache_lock); From 532199858ce283a7e8528a1f2affc7c8069a43fc Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 13 Oct 2017 18:26:21 +0000 Subject: [PATCH 10/28] repo: Add config option for default lock timeout Add a repository option, core.lock-timeout that controls the default lock timeout used. This allows administrators to have some influence over locking operations. For instance, critical repositories where operations should not fail could be configured to block when managing locks by setting lock-timeout to -1. --- man/ostree.repo-config.xml | 16 +++++++++++++++ src/libostree/ostree-repo-private.h | 1 + src/libostree/ostree-repo.c | 32 +++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/man/ostree.repo-config.xml b/man/ostree.repo-config.xml index 388b9e627f..83e44d6bfa 100644 --- a/man/ostree.repo-config.xml +++ b/man/ostree.repo-config.xml @@ -143,6 +143,22 @@ Boston, MA 02111-1307, USA. + + lock-timeout + + + Integer value controlling the default repository locking timeout + in seconds. This controls how long OSTree will wait attempting to + acquire or remove the repository lock. If the value is + 0, then one attempt to acquire or remove the + lock will be made without waiting to try again. If the value is + -1, then ostree will block until the lock can + be acquired or removed. All other negative values are an error. + The default is 30 seconds. + + + + diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 5d2d712c65..eb786adaaf 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -37,6 +37,7 @@ G_BEGIN_DECLS #define _OSTREE_MAX_OUTSTANDING_FETCHER_REQUESTS 8 #define _OSTREE_MAX_OUTSTANDING_DELTAPART_REQUESTS 2 +/* Keep this in sync with the man page for the core.lock-timeout option. */ #define _OSTREE_DEFAULT_LOCK_TIMEOUT_SECONDS 30 /* In most cases, writing to disk should be much faster than diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 749d229156..cd02f2ba8e 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -2783,6 +2783,38 @@ reload_core_config (OstreeRepo *self, return FALSE; } + { g_autofree char *lock_timeout_str = NULL; + g_autoptr(GError) local_error = NULL; + + /* Default is set at repo_init() time so that the timeout can be + * configured by ostree programs without being overwritten when the + * configuration is reloaded. + */ + lock_timeout_str = g_key_file_get_value (self->config, "core", "lock-timeout", + &local_error); + if (lock_timeout_str == NULL) + { + if (g_error_matches (local_error, G_KEY_FILE_ERROR, + G_KEY_FILE_ERROR_KEY_NOT_FOUND)) + g_clear_error (&local_error); + else + { + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; + } + } + else + { + char *endptr = NULL; + gint64 lock_timeout = g_ascii_strtoll (lock_timeout_str, &endptr, 0); + if ((lock_timeout < -1) || + (lock_timeout > G_MAXINT) || + (lock_timeout == 0 && endptr == lock_timeout_str)) + return glnx_throw (error, "Invalid lock-timeout '%s'", lock_timeout_str); + self->lock_timeout_seconds = (gint)lock_timeout; + } + } + return TRUE; } From 0ab32a4e4258af0c592f99e1ec96841dd7f8ba15 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 16 Oct 2017 19:08:21 +0000 Subject: [PATCH 11/28] tests: Add tests for repo locking APIs Exercise the ostree_repo_lock_push() and ostree_repo_lock_pop() APIs directly via pygi. This is a better unit test than relying on concurrent commit and prunes to check that the locking works correctly. --- Makefile-tests.am | 1 + tests/test-repo-locking.py | 175 +++++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100755 tests/test-repo-locking.py diff --git a/Makefile-tests.am b/Makefile-tests.am index 30c59585da..3ee34dae7f 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -108,6 +108,7 @@ _installed_or_uninstalled_test_scripts = \ tests/test-auto-summary.sh \ tests/test-prune.sh \ tests/test-concurrency.py \ + tests/test-repo-locking.py \ tests/test-refs.sh \ tests/test-demo-buildsystem.sh \ tests/test-switchroot.sh \ diff --git a/tests/test-repo-locking.py b/tests/test-repo-locking.py new file mode 100755 index 0000000000..771d58fe62 --- /dev/null +++ b/tests/test-repo-locking.py @@ -0,0 +1,175 @@ +#!/usr/bin/env python +# +# Copyright (C) 2017 Dan Nicholson +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the +# Free Software Foundation, Inc., 59 Temple Place - Suite 330, +# Boston, MA 02111-1307, USA. + +from __future__ import print_function +from contextlib import contextmanager +import os +import multiprocessing +import subprocess +import sys +import time +import yaml + +try: + import gi +except ImportError: + print('1..0 # SKIP no python gi module') + sys.exit(0) + +gi.require_version('OSTree', '1.0') +from gi.repository import Gio, GLib, OSTree + +# See if the experimental API support is included +proc = subprocess.Popen(['ostree', '--version'], stdout=subprocess.PIPE) +data = yaml.safe_load(proc.stdout) +proc.communicate() +if proc.returncode != 0: + raise subprocess.CalledProcessError('ostree --version failed') +if 'experimental' not in data['libostree']['Features']: + print('1..0 # SKIP no experimental API') + sys.exit(0) + +@contextmanager +def repo_lock(repo, exclusive, cancellable=None): + """Ensure lock popped after action""" + repo.lock_push(exclusive, cancellable) + try: + yield + finally: + repo.lock_pop(cancellable) + +print('1..11') + +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 +subprocess.check_call(['ostree', '--repo=repo', 'config', 'set', + 'core.disable-xattrs', 'true']) + +# Set the default lock timeout to 0 seconds to speed up the tests +subprocess.check_call(['ostree', '--repo=repo', 'config', 'set', + 'core.lock-timeout', '0']) + +# Open 2 repo objects to operate on concurrently +repo_file = Gio.File.new_for_path('repo') +repo1 = OSTree.Repo.new(repo_file) +repo1.open() +repo2 = OSTree.Repo.new(repo_file) +repo2.open() + +# Recursive locking +with repo_lock(repo1, False), repo_lock(repo1, False): + pass +print('ok recursive shared lock') + +with repo_lock(repo1, True), repo_lock(repo1, True): + pass +print('ok recursive exclusive lock') + +with repo_lock(repo1, False), repo_lock(repo1, True): + pass +print('ok recursive upgrade lock') + +with repo_lock(repo1, True), repo_lock(repo1, False): + pass +print('ok recursive non-upgrade lock') + +# Both repos can get shared locks +with repo_lock(repo1, False), repo_lock(repo2, False): + pass +print('ok concurrent shared locks') + +# Both repos cannot get exclusive locks +with repo_lock(repo1, True): + try: + with repo_lock(repo2, True): + pass + raise Exception('Both repos took exclusive locks') + except GLib.Error as err: + if not err.matches(Gio.io_error_quark(), + Gio.IOErrorEnum.WOULD_BLOCK): + raise +print('ok concurrent exclusive locks fails') + +# Repo cannot get exclusive lock while other is shared and vice-versa +with repo_lock(repo1, True): + try: + with repo_lock(repo2, False): + pass + raise Exception('repo2 got shared lock when repo1 exclusive') + except GLib.Error as err: + if not err.matches(Gio.io_error_quark(), + Gio.IOErrorEnum.WOULD_BLOCK): + raise +print('ok concurrent shared while exclusive fails') + +with repo_lock(repo1, False): + try: + with repo_lock(repo2, True): + pass + raise Exception('repo2 got exclusive lock when repo1 shared') + except GLib.Error as err: + if not err.matches(Gio.io_error_quark(), + Gio.IOErrorEnum.WOULD_BLOCK): + raise +print('ok concurrent exclusive while shared fails') + +# Make sure recursive lock doesn't downgrade +with repo_lock(repo1, True), repo_lock(repo1, False): + try: + with repo_lock(repo2, False): + pass + raise Exception('repo2 got shared lock when repo1 exclusive') + except GLib.Error as err: + if not err.matches(Gio.io_error_quark(), + Gio.IOErrorEnum.WOULD_BLOCK): + raise +print('ok recursive lock does not downgrade') + +# Make sure recursive lock downgrades when unwinding +with repo_lock(repo1, False): + with repo_lock(repo1, True): + pass + with repo_lock(repo2, False): + pass +print('ok recursive lock downgraded when popped') + +# Set the default lock timeout to 3 seconds to test the lock timeout +# behavior +subprocess.check_call(['ostree', '--repo=repo', 'config', 'set', + 'core.lock-timeout', '3']) +repo1.reload_config() +repo2.reload_config() + +def repo_lock_sleep(exclusive=True, sleep=0, cancellable=None): + repo = OSTree.Repo.new(repo_file) + repo.open() + with repo_lock(repo, exclusive, cancellable): + if sleep > 0: + time.sleep(sleep) + +proc1 = multiprocessing.Process(target=repo_lock_sleep, kwargs={'sleep': 1}) +proc2 = multiprocessing.Process(target=repo_lock_sleep) +proc1.start() +proc2.start() +proc1.join() +proc2.join() +assert proc1.exitcode == 0 +assert proc2.exitcode == 0 +print('ok concurrent exclusive locks wait') From fec6b1fcb1cb645ab7897f90221d6610541bc1cd Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 16 Oct 2017 20:33:17 +0000 Subject: [PATCH 12/28] lib/commit: Take shared lock when writing commit detached metadata Ensure that the commit object doesn't get deleted while writing out the detached metadata. This is primarily to guard against dangling commitmeta files if racing with a prune since the commit object could still be updated while this is happening. --- src/libostree/ostree-repo-commit.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 54ffe4ad6c..3a7d37be6e 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2376,6 +2376,8 @@ ostree_repo_read_commit_detached_metadata (OstreeRepo *self, * Replace any existing metadata associated with commit referred to by * @checksum with @metadata. If @metadata is %NULL, then existing * data will be deleted. + * + * This function takes a shared lock on the @self repository. */ gboolean ostree_repo_write_commit_detached_metadata (OstreeRepo *self, @@ -2384,6 +2386,13 @@ ostree_repo_write_commit_detached_metadata (OstreeRepo *self, GCancellable *cancellable, GError **error) { + /* Take shared lock so associated commit doesn't get deleted */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_SHARED, + cancellable, error); + if (!lock) + return FALSE; + int dest_dfd; if (self->in_transaction) dest_dfd = self->commit_stagedir.fd; From eba8a175e9b9a8241b0e5b6eec5a29621d250df0 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 16 Oct 2017 21:26:52 +0000 Subject: [PATCH 13/28] lib/repo: Take shared lock while generating summary This ensures that the commits and deltas don't get deleted while generating the summary metadata. --- src/libostree/ostree-repo.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index cd02f2ba8e..672ac6e332 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -5235,6 +5235,8 @@ summary_add_ref_entry (OstreeRepo *self, * listed under the %OSTREE_SUMMARY_COLLECTION_MAP key. Collection IDs and refs * in %OSTREE_SUMMARY_COLLECTION_MAP are guaranteed to be in lexicographic * order. + * + * This function takes a shared lock on the @self repository. */ gboolean ostree_repo_regenerate_summary (OstreeRepo *self, @@ -5242,6 +5244,15 @@ ostree_repo_regenerate_summary (OstreeRepo *self, GCancellable *cancellable, GError **error) { + /* Take a shared lock. This makes sure the commits and deltas don't get + * deleted while generating the summary. + */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_SHARED, + cancellable, error); + if (!lock) + return FALSE; + g_auto(GVariantDict) additional_metadata_builder = OT_VARIANT_BUILDER_INITIALIZER; g_variant_dict_init (&additional_metadata_builder, additional_metadata); g_autoptr(GVariantBuilder) refs_builder = g_variant_builder_new (G_VARIANT_TYPE ("a(s(taya{sv}))")); From 23b7aa4182e9d581dc06f7cbd8c28cd8a3aa0dce Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 16 Oct 2017 21:31:53 +0000 Subject: [PATCH 14/28] lib/repo: Take exclusive lock while signing summary Make sure that the summary file does not get replaced while generating summary.sig by taking an exclusive lock. It would be bad to publish a mismatching signature. --- src/libostree/ostree-repo.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 672ac6e332..279db27624 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4711,6 +4711,8 @@ ostree_repo_sign_delta (OstreeRepo *self, * @error: a #GError * * Add a GPG signature to a summary file. + * + * This function takes an exclusive lock on the @self repository. */ gboolean ostree_repo_add_gpg_signature_summary (OstreeRepo *self, @@ -4719,6 +4721,15 @@ ostree_repo_add_gpg_signature_summary (OstreeRepo *self, GCancellable *cancellable, GError **error) { + /* Take an exclusive lock so that the summary doesn't get replaced + * while calculating the signatures. + */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); + if (!lock) + return FALSE; + glnx_autofd int fd = -1; if (!glnx_openat_rdonly (self->repo_dir_fd, "summary", TRUE, &fd, error)) return FALSE; @@ -5245,7 +5256,9 @@ ostree_repo_regenerate_summary (OstreeRepo *self, GError **error) { /* Take a shared lock. This makes sure the commits and deltas don't get - * deleted while generating the summary. + * deleted while generating the summary. It also makes sure the summary + * doesn't get replaced while it's being signed since that's done with an + * exclusive lock. */ g_autoptr(OstreeRepoAutoLock) lock = NULL; lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_SHARED, From 4b8d03edafefa84bff6ddcf2ca321c67f07c61d6 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Tue, 17 Oct 2017 11:04:26 +0000 Subject: [PATCH 15/28] lib/deltas: Take shared lock while generating Make sure that neither commit nor any of the objects they reference are deleted while generating a delta. --- src/libostree/ostree-repo-static-delta-compilation.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/libostree/ostree-repo-static-delta-compilation.c b/src/libostree/ostree-repo-static-delta-compilation.c index 67bfc96e3e..d2955cb5cb 100644 --- a/src/libostree/ostree-repo-static-delta-compilation.c +++ b/src/libostree/ostree-repo-static-delta-compilation.c @@ -30,6 +30,7 @@ #include "ostree-repo-static-delta-private.h" #include "ostree-diff.h" #include "ostree-rollsum.h" +#include "ostree-autocleanups.h" #include "otutil.h" #include "libglnx.h" #include "ostree-varint.h" @@ -1182,6 +1183,8 @@ variant_to_inputstream (GVariant *variant) * - verbose: b: Print diagnostic messages. Default FALSE. * - endianness: b: Deltas use host byte order by default; this option allows choosing (G_BIG_ENDIAN or G_LITTLE_ENDIAN) * - filename: ay: Save delta superblock to this filename, and parts in the same directory. Default saves to repository. + * + * This function takes a shared lock on the @self repository. */ gboolean ostree_repo_static_delta_generate (OstreeRepo *self, @@ -1193,6 +1196,13 @@ ostree_repo_static_delta_generate (OstreeRepo *self, GCancellable *cancellable, GError **error) { + /* Take a shared lock while generating so neither commit gets deleted */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_SHARED, + cancellable, error); + if (!lock) + return FALSE; + gboolean ret = FALSE; OstreeStaticDeltaBuilder builder = { 0, }; guint i; From 2567a016ad37181ae273326b888e377ceabaf84c Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Tue, 17 Oct 2017 18:53:47 +0000 Subject: [PATCH 16/28] lib/repo: Take lock when deleting objects Ideally this would always be an exclusive lock, but this is also called when a commit object is written. Taking an exclusive lock there would prevent commit and pull concurrency. Refactor ostree_repo_delete_object to allow an internal version that only takes a shared lock and use that when writing the commit. --- src/libostree/ostree-repo-commit.c | 17 ++++--- src/libostree/ostree-repo-private.h | 8 ++++ src/libostree/ostree-repo.c | 71 +++++++++++++++++++++-------- 3 files changed, 71 insertions(+), 25 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 3a7d37be6e..cd20ea7374 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -986,13 +986,16 @@ write_metadata_object (OstreeRepo *self, if (objtype == OSTREE_OBJECT_TYPE_COMMIT) { GError *local_error = NULL; - /* If we are writing a commit, be sure there is no tombstone for it. - We may have deleted the commit and now we are trying to pull it again. */ - if (!ostree_repo_delete_object (self, - OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT, - actual_checksum, - cancellable, - &local_error)) + /* If we are writing a commit, be sure there is no tombstone for it. We + * may have deleted the commit and now we are trying to pull it again. + * Delete with a shared lock here so concurrent commits and pulls aren't + * prevented. + */ + if (!_ostree_repo_delete_object_shared (self, + OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT, + actual_checksum, + cancellable, + &local_error)) { if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) g_clear_error (&local_error); diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index eb786adaaf..6c94e8adb7 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -418,6 +418,14 @@ _ostree_repo_get_remote_inherited (OstreeRepo *self, const char *name, GError **error); +gboolean +_ostree_repo_delete_object_shared (OstreeRepo *self, + OstreeObjectType objtype, + const char *sha256, + GCancellable *cancellable, + GError **error); + + #ifndef OSTREE_ENABLE_EXPERIMENTAL_API typedef enum { diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 279db27624..a9a89d394e 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -3851,25 +3851,19 @@ ostree_repo_has_object (OstreeRepo *self, return TRUE; } -/** - * ostree_repo_delete_object: - * @self: Repo - * @objtype: Object type - * @sha256: Checksum - * @cancellable: Cancellable - * @error: Error - * - * Remove the object of type @objtype with checksum @sha256 - * from the repository. An error of type %G_IO_ERROR_NOT_FOUND - * is thrown if the object does not exist. - */ -gboolean -ostree_repo_delete_object (OstreeRepo *self, - OstreeObjectType objtype, - const char *sha256, - GCancellable *cancellable, - GError **error) +static gboolean +delete_object (OstreeRepo *self, + OstreeObjectType objtype, + const char *sha256, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error) { + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, lock_type, cancellable, error); + if (!lock) + return FALSE; + char loose_path[_OSTREE_LOOSE_PATH_MAX]; _ostree_loose_path (loose_path, sha256, objtype, self->mode); @@ -3916,6 +3910,47 @@ ostree_repo_delete_object (OstreeRepo *self, return TRUE; } +/* Like ostree_repo_delete_object, but with a shared repo lock instead of + * exclusive. Ideally, objects would always be deleted with an exclusive lock, + * but that would prevent concurrency when the object to delete is not + * critical. + */ +gboolean +_ostree_repo_delete_object_shared (OstreeRepo *self, + OstreeObjectType objtype, + const char *sha256, + GCancellable *cancellable, + GError **error) +{ + return delete_object (self, objtype, sha256, OSTREE_REPO_LOCK_SHARED, + cancellable, error); +} + +/** + * ostree_repo_delete_object: + * @self: Repo + * @objtype: Object type + * @sha256: Checksum + * @cancellable: Cancellable + * @error: Error + * + * Remove the object of type @objtype with checksum @sha256 + * from the repository. An error of type %G_IO_ERROR_NOT_FOUND + * is thrown if the object does not exist. + * + * This function takes an exclusive lock on the @self repository. + */ +gboolean +ostree_repo_delete_object (OstreeRepo *self, + OstreeObjectType objtype, + const char *sha256, + GCancellable *cancellable, + GError **error) +{ + return delete_object (self, objtype, sha256, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); +} + /** * ostree_repo_import_object_from: * @self: Destination repo From 39a72117f0844a69c5af690e306343a9ad582be6 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Wed, 18 Oct 2017 06:41:24 -0500 Subject: [PATCH 17/28] lib/traverse: Take shared repo lock Prevent objects from being deleted during commit traversals by taking a shared repository lock. --- src/libostree/ostree-repo-traverse.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libostree/ostree-repo-traverse.c b/src/libostree/ostree-repo-traverse.c index 0d9ddbd99b..eeec707925 100644 --- a/src/libostree/ostree-repo-traverse.c +++ b/src/libostree/ostree-repo-traverse.c @@ -23,6 +23,7 @@ #include "libglnx.h" #include "ostree.h" +#include "ostree-repo-private.h" #include "otutil.h" struct _OstreeRepoRealCommitTraverseIter { @@ -437,6 +438,8 @@ traverse_dirtree (OstreeRepo *repo, * * Update the set @inout_reachable containing all objects reachable * from @commit_checksum, traversing @maxdepth parent commits. + * + * This function takes a shared lock on the @repo repository. */ gboolean ostree_repo_traverse_commit_union (OstreeRepo *repo, @@ -446,6 +449,12 @@ ostree_repo_traverse_commit_union (OstreeRepo *repo, GCancellable *cancellable, GError **error) { + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_SHARED, + cancellable, error); + if (!lock) + return FALSE; + gboolean ret = FALSE; g_autofree char *tmp_checksum = NULL; @@ -527,6 +536,8 @@ ostree_repo_traverse_commit_union (OstreeRepo *repo, * * Create a new set @out_reachable containing all objects reachable * from @commit_checksum, traversing @maxdepth parent commits. + * + * This function takes a shared lock on the @repo repository. */ gboolean ostree_repo_traverse_commit (OstreeRepo *repo, From 1c87dfbd328ce44f70af5d2a8c012bf8dd92ddca Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Wed, 18 Oct 2017 13:38:01 +0000 Subject: [PATCH 18/28] lib/deltas: Take exclusive lock when deleting delta Like when pruning static deltas, take an exclusive lock when deleting a delta directly. --- src/libostree/ostree-repo-static-delta-core.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libostree/ostree-repo-static-delta-core.c b/src/libostree/ostree-repo-static-delta-core.c index b2595366fa..4b1884a80f 100644 --- a/src/libostree/ostree-repo-static-delta-core.c +++ b/src/libostree/ostree-repo-static-delta-core.c @@ -28,6 +28,7 @@ #include "ostree-cmdprivate.h" #include "ostree-checksum-input-stream.h" #include "ostree-repo-static-delta-private.h" +#include "ostree-autocleanups.h" #include "otutil.h" gboolean @@ -696,6 +697,13 @@ _ostree_repo_static_delta_delete (OstreeRepo *self, if (!_ostree_parse_delta_name (delta_id, &from, &to, error)) return FALSE; + /* Take exclusive lock while deleting deltas */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); + if (!lock) + return FALSE; + g_autofree char *deltadir = _ostree_get_relative_static_delta_path (from, to, NULL); struct stat buf; if (fstatat (self->repo_dir_fd, deltadir, &buf, 0) != 0) From c2e091aec8b6bb30363d8b2024e30cd2941fa8db Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Tue, 17 Oct 2017 13:44:26 +0000 Subject: [PATCH 19/28] lib/repo: Add accessors for lock timeout Allow ostree applications to access and override the lock timeout. This gives them some control over how locking is managed. If the repo configuration is reloaded, it will be reset if core.lock-timeout is set. --- apidoc/ostree-experimental-sections.txt | 2 ++ src/libostree/libostree-experimental.sym | 2 ++ src/libostree/ostree-repo-private.h | 4 +++ src/libostree/ostree-repo.c | 44 ++++++++++++++++++++++++ src/libostree/ostree-repo.h | 6 ++++ tests/test-repo-locking.py | 17 ++++++++- 6 files changed, 74 insertions(+), 1 deletion(-) diff --git a/apidoc/ostree-experimental-sections.txt b/apidoc/ostree-experimental-sections.txt index 0e85d095cb..1dcf3aef11 100644 --- a/apidoc/ostree-experimental-sections.txt +++ b/apidoc/ostree-experimental-sections.txt @@ -92,6 +92,8 @@ ostree_repo_lock_pop OstreeRepoAutoLock ostree_repo_auto_lock_push ostree_repo_auto_lock_cleanup +ostree_repo_get_lock_timeout +ostree_repo_set_lock_timeout 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 b8aa277336..9ebd2487de 100644 --- a/src/libostree/libostree-experimental.sym +++ b/src/libostree/libostree-experimental.sym @@ -90,6 +90,8 @@ global: ostree_repo_finder_override_new; ostree_repo_auto_lock_cleanup; ostree_repo_auto_lock_push; + ostree_repo_get_lock_timeout; ostree_repo_lock_pop; ostree_repo_lock_push; + ostree_repo_set_lock_timeout; } LIBOSTREE_2017.12_EXPERIMENTAL; diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 6c94e8adb7..8d728a3e31 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -450,6 +450,10 @@ OstreeRepoAutoLock * ostree_repo_auto_lock_push (OstreeRepo *self, void ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock); G_DEFINE_AUTOPTR_CLEANUP_FUNC (OstreeRepoAutoLock, ostree_repo_auto_lock_cleanup) +gint ostree_repo_get_lock_timeout (OstreeRepo *self); +gboolean ostree_repo_set_lock_timeout (OstreeRepo *self, + gint timeout); + 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.c b/src/libostree/ostree-repo.c index a9a89d394e..5ec923dcb5 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -5702,3 +5702,47 @@ ostree_repo_set_collection_id (OstreeRepo *self, return TRUE; } + +/** + * ostree_repo_get_lock_timeout: + * @self: an #OstreeRepo + * + * Get the current lock timeout for the repository. A timeout of -1 indicates + * that the application will block until the lock is acquired. Otherwise, it + * represents the number of seconds the application will sleep attempting to + * acquire the repository lock. + * + * Returns: lock timeout for the repository + * Since: 2017.13 + */ +gint +ostree_repo_get_lock_timeout (OstreeRepo *self) +{ + return self->lock_timeout_seconds; +} + +/** + * ostree_repo_set_lock_timeout: + * @self: an #OstreeRepo + * @timeout: new lock timeout in seconds + * + * Set the lock timeout for the repository. A timeout of -1 indicates that the + * application will block until the lock is acquired. Otherwise, it represents + * the number of seconds the application will sleep attempting to acquire the + * repository lock. + * + * Note that if the repository configuration is reloaded, then a timeout + * configured in the core.lock-timeout option will replace this. + * + * Returns: %TRUE on success, %FALSE otherwise + * Since: 2017.13 + */ +gboolean +ostree_repo_set_lock_timeout (OstreeRepo *self, + gint timeout) +{ + g_return_val_if_fail (timeout >= -1, FALSE); + + self->lock_timeout_seconds = timeout; + return TRUE; +} diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index e891819897..3126cc71c2 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -146,6 +146,12 @@ OstreeRepoAutoLock * ostree_repo_auto_lock_push (OstreeRepo *self, _OSTREE_PUBLIC void ostree_repo_auto_lock_cleanup (OstreeRepoAutoLock *lock); +_OSTREE_PUBLIC +gint ostree_repo_get_lock_timeout (OstreeRepo *self); +_OSTREE_PUBLIC +gboolean ostree_repo_set_lock_timeout (OstreeRepo *self, + gint timeout); + _OSTREE_PUBLIC const gchar * ostree_repo_get_collection_id (OstreeRepo *self); _OSTREE_PUBLIC diff --git a/tests/test-repo-locking.py b/tests/test-repo-locking.py index 771d58fe62..d5aacb2c2f 100755 --- a/tests/test-repo-locking.py +++ b/tests/test-repo-locking.py @@ -54,7 +54,7 @@ def repo_lock(repo, exclusive, cancellable=None): finally: repo.lock_pop(cancellable) -print('1..11') +print('1..14') subprocess.check_call(['ostree', '--repo=repo', 'init', '--mode=bare']) # like the bit in libtest, but let's do it unconditionally since it's simpler, @@ -173,3 +173,18 @@ def repo_lock_sleep(exclusive=True, sleep=0, cancellable=None): assert proc1.exitcode == 0 assert proc2.exitcode == 0 print('ok concurrent exclusive locks wait') + +# Test lock timeout accessors +cur_timeout = repo1.get_lock_timeout() +assert cur_timeout == 3 +print('ok get_lock_timeout()') + +repo1.set_lock_timeout(0) +cur_timeout = repo1.get_lock_timeout() +assert cur_timeout == 0 +print('ok set_lock_timeout()') + +repo1.reload_config() +cur_timeout = repo1.get_lock_timeout() +assert cur_timeout == 3 +print('ok lock_timeout reset by core.lock-timeout') From 615b949c3f4f7f342663a6e85fe6874d6d3d01d0 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 19 Oct 2017 15:54:02 +0000 Subject: [PATCH 20/28] lib/cmdprivate: Add private access to ostree_repo_set_lock_timeout Allow the ostree commands to use ostree_repo_set_lock_timeout() even when ostree is built without experimental API. When it's no longer experimental, this should be removed. --- src/libostree/ostree-cmdprivate.c | 4 +++- src/libostree/ostree-cmdprivate.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-cmdprivate.c b/src/libostree/ostree-cmdprivate.c index d0c7e98e8e..6a4e5ad859 100644 --- a/src/libostree/ostree-cmdprivate.c +++ b/src/libostree/ostree-cmdprivate.c @@ -48,7 +48,9 @@ ostree_cmd__private__ (void) impl_ostree_generate_grub2_config, _ostree_repo_static_delta_dump, _ostree_repo_static_delta_query_exists, - _ostree_repo_static_delta_delete + _ostree_repo_static_delta_delete, + /* Remove this when ostree_repo_set_lock_timeout is no longer experimental */ + ostree_repo_set_lock_timeout, }; return &table; diff --git a/src/libostree/ostree-cmdprivate.h b/src/libostree/ostree-cmdprivate.h index 4e49cef39a..1626875325 100644 --- a/src/libostree/ostree-cmdprivate.h +++ b/src/libostree/ostree-cmdprivate.h @@ -37,6 +37,8 @@ typedef struct { gboolean (* ostree_static_delta_dump) (OstreeRepo *repo, const char *delta_id, GCancellable *cancellable, GError **error); gboolean (* ostree_static_delta_query_exists) (OstreeRepo *repo, const char *delta_id, gboolean *out_exists, GCancellable *cancellable, GError **error); gboolean (* ostree_static_delta_delete) (OstreeRepo *repo, const char *delta_id, GCancellable *cancellable, GError **error); + /* Remove this when ostree_repo_set_lock_timeout is no longer experimental */ + gboolean (* ostree_repo_set_lock_timeout) (OstreeRepo *repo, gint timeout); } OstreeCmdPrivateVTable; /* Note this not really "public", we just export the symbol, but not the header */ From cf2824ac61334cd6264cf398219c3246f078e0bf Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Tue, 17 Oct 2017 15:17:00 +0000 Subject: [PATCH 21/28] bin/main: Add --lock-timeout option Builtins that require repository locking can add options with the OSTREE_BUILTIN_FLAG_LOCKING flag set to add a --lock-timeout option. This allows the user to control how long to wait to acquire the repository lock. The cmdprivate version of ostree_repo_set_lock_timeout() is used until it's no longer experimental. --- man/ostree.xml | 16 ++++++++++++++++ src/ostree/ot-main.c | 34 ++++++++++++++++++++++++++++++++++ src/ostree/ot-main.h | 3 ++- 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/man/ostree.xml b/man/ostree.xml index 940d81d7d0..f3395a834c 100644 --- a/man/ostree.xml +++ b/man/ostree.xml @@ -120,6 +120,22 @@ Boston, MA 02111-1307, USA. + + =TIMEOUT + + + + Some ostree commands require the + repository to be locked while operating. This option sets + the repository lock timeout to TIMEOUT seconds. If TIMEOUT + is 0, then one attempt to acquire or + remove the lock will be made without waiting to try again. + If TIMEOUT is -1, then the locking will + block indefinitely. All other negative values are an error. + + + + diff --git a/src/ostree/ot-main.c b/src/ostree/ot-main.c index 22166c5472..5824e95b28 100644 --- a/src/ostree/ot-main.c +++ b/src/ostree/ot-main.c @@ -28,11 +28,13 @@ #include "ot-main.h" #include "ostree.h" +#include "ostree-cmdprivate.h" #include "ot-admin-functions.h" #include "otutil.h" static char *opt_repo; static char *opt_sysroot; +static gint opt_lock_timeout = -2; /* Invalid value to detect when not set */ static gboolean opt_verbose; static gboolean opt_version; static gboolean opt_print_current_dir; @@ -48,6 +50,28 @@ static GOptionEntry repo_entry[] = { { NULL } }; +static gboolean +parse_lock_timeout (const char *option_name, + const char *value, + gpointer data, + GError **error) +{ + char *endptr = NULL; + gint64 lock_timeout = g_ascii_strtoll (value, &endptr, 0); + if ((lock_timeout < -1) || + (lock_timeout > G_MAXINT) || + (lock_timeout == 0 && endptr == value)) + return glnx_throw (error, "Invalid lock timeout '%s'", value); + + opt_lock_timeout = (gint)lock_timeout; + return TRUE; +} + +static GOptionEntry lock_timeout_entry[] = { + { "lock-timeout", 0, 0, G_OPTION_ARG_CALLBACK, parse_lock_timeout, "Repository locking timeout in seconds (-1=indefinitely)", "TIMEOUT" }, + { NULL } +}; + static GOptionEntry global_admin_entries[] = { /* No description since it's hidden from --help output. */ { "print-current-dir", 0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_NONE, &opt_print_current_dir, NULL, NULL }, @@ -338,6 +362,9 @@ ostree_option_context_parse (GOptionContext *context, if (main_entries != NULL) g_option_context_add_main_entries (context, main_entries, NULL); + if (flags & OSTREE_BUILTIN_FLAG_LOCKING) + g_option_context_add_main_entries (context, lock_timeout_entry, NULL); + g_option_context_add_main_entries (context, global_entries, NULL); if (!g_option_context_parse (context, argc, argv, error)) @@ -370,6 +397,13 @@ ostree_option_context_parse (GOptionContext *context, cancellable, error); if (!repo) return FALSE; + + if ((flags & OSTREE_BUILTIN_FLAG_LOCKING) && opt_lock_timeout >= -1) + { + if (!ostree_cmd__private__ ()->ostree_repo_set_lock_timeout (repo, + opt_lock_timeout)) + return FALSE; + } } if (out_repo) diff --git a/src/ostree/ot-main.h b/src/ostree/ot-main.h index 26f9f6e1de..4873cb876f 100644 --- a/src/ostree/ot-main.h +++ b/src/ostree/ot-main.h @@ -27,7 +27,8 @@ typedef enum { OSTREE_BUILTIN_FLAG_NONE = 0, OSTREE_BUILTIN_FLAG_NO_REPO = 1 << 0, - OSTREE_BUILTIN_FLAG_NO_CHECK = 1 << 1 + OSTREE_BUILTIN_FLAG_NO_CHECK = 1 << 1, + OSTREE_BUILTIN_FLAG_LOCKING = 1 << 2, } OstreeBuiltinFlags; typedef enum { From 1ccd3dbdeaadaec9edb7c1f4d560ec4f4745f7d1 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Tue, 17 Oct 2017 17:13:56 +0000 Subject: [PATCH 22/28] bin: Add --lock-timeout option to relevant commands All of these commands call one of the libostree functions that take locks, so add the --lock-timeout option to them. --- src/ostree/main.c | 20 ++++++++++---------- src/ostree/ot-builtin-static-delta.c | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/ostree/main.c b/src/ostree/main.c index d50c2490c5..942fd4a8be 100644 --- a/src/ostree/main.c +++ b/src/ostree/main.c @@ -43,19 +43,19 @@ static OstreeCommand commands[] = { { "cat", OSTREE_BUILTIN_FLAG_NONE, ostree_builtin_cat, "Concatenate contents of files"}, - { "checkout", OSTREE_BUILTIN_FLAG_NONE, + { "checkout", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_checkout, "Check out a commit into a filesystem tree" }, { "checksum", OSTREE_BUILTIN_FLAG_NO_REPO, ostree_builtin_checksum, "Checksum a file or directory" }, - { "commit", OSTREE_BUILTIN_FLAG_NONE, + { "commit", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_commit, "Commit a new revision" }, { "config", OSTREE_BUILTIN_FLAG_NONE, ostree_builtin_config, "Change repo configuration settings" }, - { "diff", OSTREE_BUILTIN_FLAG_NONE, + { "diff", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_diff, "Compare directory TARGETDIR against revision REV"}, { "export", OSTREE_BUILTIN_FLAG_NONE, @@ -69,10 +69,10 @@ static OstreeCommand commands[] = { ostree_builtin_create_usb, "Copy the refs to a USB stick" }, #endif - { "fsck", OSTREE_BUILTIN_FLAG_NONE, + { "fsck", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_fsck, "Check the repository for consistency" }, - { "gpg-sign", OSTREE_BUILTIN_FLAG_NONE, + { "gpg-sign", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_gpg_sign, "Sign a commit" }, { "init", OSTREE_BUILTIN_FLAG_NO_CHECK, @@ -84,14 +84,14 @@ static OstreeCommand commands[] = { { "ls", OSTREE_BUILTIN_FLAG_NONE, ostree_builtin_ls, "List file paths" }, - { "prune", OSTREE_BUILTIN_FLAG_NONE, + { "prune", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_prune, "Search for unreachable objects" }, - { "pull-local", OSTREE_BUILTIN_FLAG_NONE, + { "pull-local", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_pull_local, "Copy data from SRC_REPO" }, #ifdef HAVE_LIBCURL_OR_LIBSOUP - { "pull", OSTREE_BUILTIN_FLAG_NONE, + { "pull", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_pull, "Download data from remote repository" }, #endif @@ -101,7 +101,7 @@ static OstreeCommand commands[] = { { "remote", OSTREE_BUILTIN_FLAG_NO_REPO, ostree_builtin_remote, "Remote commands that may involve internet access" }, - { "reset", OSTREE_BUILTIN_FLAG_NONE, + { "reset", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_reset, "Reset a REF to a previous COMMIT" }, { "rev-parse", OSTREE_BUILTIN_FLAG_NONE, @@ -113,7 +113,7 @@ static OstreeCommand commands[] = { { "static-delta", OSTREE_BUILTIN_FLAG_NONE, ostree_builtin_static_delta, "Static delta related commands" }, - { "summary", OSTREE_BUILTIN_FLAG_NONE, + { "summary", OSTREE_BUILTIN_FLAG_LOCKING, ostree_builtin_summary, "Manage summary metadata" }, #if defined(HAVE_LIBSOUP) && defined(BUILDOPT_ENABLE_TRIVIAL_HTTPD_CMDLINE) diff --git a/src/ostree/ot-builtin-static-delta.c b/src/ostree/ot-builtin-static-delta.c index d053500a23..0c4d1f9270 100644 --- a/src/ostree/ot-builtin-static-delta.c +++ b/src/ostree/ot-builtin-static-delta.c @@ -56,13 +56,13 @@ static OstreeCommand static_delta_subcommands[] = { { "show", OSTREE_BUILTIN_FLAG_NONE, ot_static_delta_builtin_show, "Dump information on a delta" }, - { "delete", OSTREE_BUILTIN_FLAG_NONE, + { "delete", OSTREE_BUILTIN_FLAG_LOCKING, ot_static_delta_builtin_delete, "Remove a delta" }, - { "generate", OSTREE_BUILTIN_FLAG_NONE, + { "generate", OSTREE_BUILTIN_FLAG_LOCKING, ot_static_delta_builtin_generate, "Generate static delta files" }, - { "apply-offline", OSTREE_BUILTIN_FLAG_NONE, + { "apply-offline", OSTREE_BUILTIN_FLAG_LOCKING, ot_static_delta_builtin_apply_offline, "Apply static delta file" }, { NULL, 0, NULL, NULL } From d31b39edb85251f1542538ca32550ddaa5b49e09 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 19 Oct 2017 17:33:45 +0000 Subject: [PATCH 23/28] lib/cmdprivate: Add private access to ostree_repo_auto_lock_* Allow the ostree commands to use ostree_repo_auto_lock_push() and ostree_repo_auto_lock_cleanup() even when ostree is built without experimental API. When it's no longer experimental, this should be removed. Unfortunately, this requires including ostree-repo-private.h from ostree-cmdprivate.h. That includes otutil.h, which the systemd generator build wasn't prepared to handle. --- Makefile-switchroot.am | 2 +- src/libostree/ostree-cmdprivate.c | 6 ++++++ src/libostree/ostree-cmdprivate.h | 9 +++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/Makefile-switchroot.am b/Makefile-switchroot.am index 70aa1c8786..b37a73beb0 100644 --- a/Makefile-switchroot.am +++ b/Makefile-switchroot.am @@ -65,7 +65,7 @@ systemdsystemgenerator_PROGRAMS = ostree-system-generator GITIGNOREFILES += $(systemdsystemgenerator_PROGRAMS) ostree_system_generator_SOURCES = src/switchroot/ostree-mount-util.h \ src/switchroot/ostree-system-generator.c -ostree_system_generator_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/libglnx -I$(srcdir)/src/libostree +ostree_system_generator_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/libglnx -I$(srcdir)/src/libostree -I$(srcdir)/src/libotutil ostree_system_generator_CFLAGS = $(AM_CFLAGS) $(OT_INTERNAL_GIO_UNIX_CFLAGS) ostree_system_generator_LDADD = $(AM_LDFLAGS) libglnx.la libostree-1.la $(OT_INTERNAL_GIO_UNIX_LIBS) diff --git a/src/libostree/ostree-cmdprivate.c b/src/libostree/ostree-cmdprivate.c index 6a4e5ad859..4d9b073da1 100644 --- a/src/libostree/ostree-cmdprivate.c +++ b/src/libostree/ostree-cmdprivate.c @@ -1,3 +1,4 @@ + /* * Copyright (C) 2014 Colin Walters * @@ -51,6 +52,11 @@ ostree_cmd__private__ (void) _ostree_repo_static_delta_delete, /* Remove this when ostree_repo_set_lock_timeout is no longer experimental */ ostree_repo_set_lock_timeout, + /* Remove these when ostree_repo_auto_lock_push and + * ostree_repo_auto_lock_cleanup are no longer experimental + */ + ostree_repo_auto_lock_push, + ostree_repo_auto_lock_cleanup, }; return &table; diff --git a/src/libostree/ostree-cmdprivate.h b/src/libostree/ostree-cmdprivate.h index 1626875325..863b7704e4 100644 --- a/src/libostree/ostree-cmdprivate.h +++ b/src/libostree/ostree-cmdprivate.h @@ -20,6 +20,10 @@ #pragma once #include "ostree-types.h" +/* This is only needed to get the OstreeRepoLockType and OstreeRepoAutoLock + * types. This should be removed once they're no longer experimental. + */ +#include "ostree-repo-private.h" G_BEGIN_DECLS @@ -39,6 +43,11 @@ typedef struct { gboolean (* ostree_static_delta_delete) (OstreeRepo *repo, const char *delta_id, GCancellable *cancellable, GError **error); /* Remove this when ostree_repo_set_lock_timeout is no longer experimental */ gboolean (* ostree_repo_set_lock_timeout) (OstreeRepo *repo, gint timeout); + /* Remove these when ostree_repo_auto_lock_push and + * ostree_repo_auto_lock_cleanup are no longer experimental + */ + OstreeRepoAutoLock * (* ostree_repo_auto_lock_push) (OstreeRepo *repo, OstreeRepoLockType lock_type, GCancellable *cancellable, GError **error); + void (* ostree_repo_auto_lock_cleanup) (OstreeRepoAutoLock *lock); } OstreeCmdPrivateVTable; /* Note this not really "public", we just export the symbol, but not the header */ From e3a84768703bbcc29f32655426cc6fcfc92bbc63 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 19 Oct 2017 18:39:17 +0000 Subject: [PATCH 24/28] bin/main: Provide repo lock autoptr implementation Since ostree_repo_auto_lock_cleanup is not directly available, a separate type, wrappers and autoptr declaration are needed. --- src/ostree/ot-main.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/ostree/ot-main.h b/src/ostree/ot-main.h index 4873cb876f..4169466cc5 100644 --- a/src/ostree/ot-main.h +++ b/src/ostree/ot-main.h @@ -23,6 +23,11 @@ #include "libglnx.h" #include "ostree.h" +/* Remove these once OstreeRepoLockType and OstreeRepoAutoLock are no longer + * experimental + */ +#include "ostree-repo-private.h" +#include "ostree-cmdprivate.h" typedef enum { OSTREE_BUILTIN_FLAG_NONE = 0, @@ -90,3 +95,29 @@ gboolean ostree_ensure_repo_writable (OstreeRepo *repo, GError **error); void ostree_print_gpg_verify_result (OstreeGpgVerifyResult *result); gboolean ot_enable_tombstone_commits (OstreeRepo *repo, GError **error); + +/* Duplicate of the OstreeRepoAutoLock cleanup implementation since we can't + * access ostree_repo_auto_lock_cleanup directly. A separate type is needed so + * that the autoptr declaration doesn't conflict. Remove this once + * ostree_repo_auto_lock_push and ostree_repo_auto_lock_cleanup are not + * experimental. + */ +typedef OstreeRepo OtRepoAutoLock; + +static inline OtRepoAutoLock * +ot_repo_auto_lock_push (OstreeRepo *repo, + OstreeRepoLockType lock_type, + GCancellable *cancellable, + GError **error) +{ + return ostree_cmd__private__ ()->ostree_repo_auto_lock_push (repo, lock_type, + cancellable, error); +} + +static inline void +ot_repo_auto_lock_cleanup (OtRepoAutoLock *lock) +{ + ostree_cmd__private__ ()->ostree_repo_auto_lock_cleanup (lock); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC (OtRepoAutoLock, ot_repo_auto_lock_cleanup) From 7c14891ca2e7d2be68a42534ee029ca89876e05e Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Wed, 18 Oct 2017 14:24:25 +0000 Subject: [PATCH 25/28] bin/fsck: Take exclusive lock for entire operation Although the libostree functions will take locks as needed, its best to ensure fsck has exclusive access to the repo for checking consistency. --- src/ostree/ot-builtin-fsck.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/ostree/ot-builtin-fsck.c b/src/ostree/ot-builtin-fsck.c index 116fdc6b38..2bd30c0982 100644 --- a/src/ostree/ot-builtin-fsck.c +++ b/src/ostree/ot-builtin-fsck.c @@ -223,6 +223,16 @@ ostree_builtin_fsck (int argc, char **argv, OstreeCommandInvocation *invocation, if (!ostree_option_context_parse (context, options, &argc, &argv, invocation, &repo, cancellable, error)) return FALSE; + /* Although the operations will obtain locks as needed, take an exclusive + * lock now to ensure no one else is changing the repo. + */ + if (!opt_quiet) + g_print ("Locking repository exclusively...\n"); + g_autoptr(OtRepoAutoLock) lock = NULL; + lock = ot_repo_auto_lock_push (repo, OSTREE_REPO_LOCK_EXCLUSIVE, cancellable, error); + if (!lock) + return FALSE; + if (!opt_quiet) g_print ("Validating refs...\n"); From e41524787696aacc4b17b20033a6c05fa6703011 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Wed, 18 Oct 2017 16:07:12 +0000 Subject: [PATCH 26/28] bin/admin: Add --lock-timeout option Admin builtins that require repository locking set the OSTREE_BUILTIN_FLAG_LOCKING flag to add a --lock-timeout option. This needs to be used in conjunction with OSTREE_BUILTIN_FLAG_NO_REPO since the the timeout can only be applied once the repo is gathered from the sysroot. This allows the user to control how long to wait to acquire the repository lock. The cmdprivate version of ostree_repo_set_lock_timeout() is used until it's no longer experimental. --- man/ostree-admin.xml | 16 ++++++++++++++++ src/ostree/ot-main.c | 10 ++++++++++ 2 files changed, 26 insertions(+) diff --git a/man/ostree-admin.xml b/man/ostree-admin.xml index 62cbbe93ce..5debe4e6f4 100644 --- a/man/ostree-admin.xml +++ b/man/ostree-admin.xml @@ -105,6 +105,22 @@ Boston, MA 02111-1307, USA. Prints the full path to the deployment directory for the currently active deployment in the specified sysroot to standard out. This is incompatible with specifying a subcommand. + + + =TIMEOUT + + + + Some ostree admin commands require the + repository to be locked while operating. This option sets + the repository lock timeout to TIMEOUT seconds. If TIMEOUT + is 0, then one attempt to acquire or + remove the lock will be made without waiting to try again. + If TIMEOUT is -1, then the locking will + block indefinitely. All other negative values are an error. + + + diff --git a/src/ostree/ot-main.c b/src/ostree/ot-main.c index 5824e95b28..58424afeaa 100644 --- a/src/ostree/ot-main.c +++ b/src/ostree/ot-main.c @@ -505,6 +505,16 @@ ostree_admin_option_context_parse (GOptionContext *context, exit (EXIT_SUCCESS); } + /* Now that we have the repo from the sysroot, apply the lock timeout */ + if ((invocation->command->flags & OSTREE_BUILTIN_FLAG_LOCKING) && + opt_lock_timeout >= -1) + { + OstreeRepo *repo = ostree_sysroot_repo (sysroot); + if (!ostree_cmd__private__ ()->ostree_repo_set_lock_timeout (repo, + opt_lock_timeout)) + return FALSE; + } + if (out_sysroot) *out_sysroot = g_steal_pointer (&sysroot); From 7ff4b938f134473ecbc1ca5b0cb2e20bb2716f0f Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Wed, 18 Oct 2017 16:09:20 +0000 Subject: [PATCH 27/28] bin/admin: Add --lock-timeout option to relevant commands All of these commands call one of the libostree functions that take locks, so add the --lock-timeout option to them. --- src/ostree/ot-builtin-admin.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ostree/ot-builtin-admin.c b/src/ostree/ot-builtin-admin.c index 17033ecf3b..d68a8b9878 100644 --- a/src/ostree/ot-builtin-admin.c +++ b/src/ostree/ot-builtin-admin.c @@ -31,13 +31,13 @@ #include static OstreeCommand admin_subcommands[] = { - { "cleanup", OSTREE_BUILTIN_FLAG_NO_REPO, + { "cleanup", OSTREE_BUILTIN_FLAG_NO_REPO | OSTREE_BUILTIN_FLAG_LOCKING, ot_admin_builtin_cleanup, "Delete untagged deployments and repository objects" }, { "config-diff", OSTREE_BUILTIN_FLAG_NO_REPO, ot_admin_builtin_diff, "Diff current /etc configuration versus default" }, - { "deploy", OSTREE_BUILTIN_FLAG_NO_REPO, + { "deploy", OSTREE_BUILTIN_FLAG_NO_REPO | OSTREE_BUILTIN_FLAG_LOCKING, ot_admin_builtin_deploy, "Checkout revision REFSPEC as the new default deployment" }, { "init-fs", OSTREE_BUILTIN_FLAG_NO_REPO, @@ -55,16 +55,16 @@ static OstreeCommand admin_subcommands[] = { { "status", OSTREE_BUILTIN_FLAG_NO_REPO, ot_admin_builtin_status, "List deployments" }, - { "switch", OSTREE_BUILTIN_FLAG_NO_REPO, + { "switch", OSTREE_BUILTIN_FLAG_NO_REPO | OSTREE_BUILTIN_FLAG_LOCKING, ot_admin_builtin_switch, "Construct new tree from REF and deploy it" }, - { "undeploy", OSTREE_BUILTIN_FLAG_NO_REPO, + { "undeploy", OSTREE_BUILTIN_FLAG_NO_REPO | OSTREE_BUILTIN_FLAG_LOCKING, ot_admin_builtin_undeploy, "Delete deployment INDEX" }, { "unlock", OSTREE_BUILTIN_FLAG_NO_REPO, ot_admin_builtin_unlock, "Make the current deployment mutable (as a hotfix or development)" }, - { "upgrade", OSTREE_BUILTIN_FLAG_NO_REPO, + { "upgrade", OSTREE_BUILTIN_FLAG_NO_REPO | OSTREE_BUILTIN_FLAG_LOCKING, ot_admin_builtin_upgrade, "Construct new tree from current origin and deploy it, if it changed" }, { NULL, 0, NULL, NULL } From 8c9f51816c4e5dcffe66b07e3f2d6cf7e3ff803e Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Tue, 24 Oct 2017 17:08:16 +0000 Subject: [PATCH 28/28] squash! tests: Test concurrent operations Make sure the number of committers is even since we only create half as many trees. --- tests/test-concurrency.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test-concurrency.py b/tests/test-concurrency.py index c397308b5a..299f9fa36a 100755 --- a/tests/test-concurrency.py +++ b/tests/test-concurrency.py @@ -64,6 +64,10 @@ def wait_check(proc): print("1..2") def run(n_committers, n_pruners): + # The number of committers needs to be even since we only create half as + # many trees + n_committers += n_committers % 2 + committers = set() pruners = set()