From 3e289ffab0f18af4a59fb084dd2d4e39b79fdc7a Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 7 Mar 2016 11:56:22 -0500 Subject: [PATCH] daemon: Drop internal mutexes for sysroot Now that the internal reading methods operate on the mainloop, and we know there can only be one write transaction at a time, it should be safe to drop the internal mutexes (and multithreading). Updates to the `OstreeSysroot` instance and DBus API all happen off the mainloop now. The write transactions now use a separate `OstreeSysroot` instance, and do not perform any changes to process state on their own. We always reload state from disk. I think this is a lot simpler to reason about from a correctness point of view, at a likely negligble loss in performance for read transactions. --- src/daemon/rpmostreed-os.c | 18 ---- src/daemon/rpmostreed-sysroot.c | 52 ---------- src/daemon/rpmostreed-sysroot.h | 4 - src/daemon/rpmostreed-transaction-types.c | 111 +++++----------------- src/daemon/rpmostreed-transaction.c | 41 +++++--- 5 files changed, 55 insertions(+), 171 deletions(-) diff --git a/src/daemon/rpmostreed-os.c b/src/daemon/rpmostreed-os.c index 3a2d3fc4..b6206e9b 100644 --- a/src/daemon/rpmostreed-os.c +++ b/src/daemon/rpmostreed-os.c @@ -149,8 +149,6 @@ os_handle_get_deployments_rpm_diff (RPMOSTreeOS *interface, global_sysroot = rpmostreed_sysroot_get (); - rpmostreed_sysroot_reader_lock (global_sysroot); - ot_sysroot = rpmostreed_sysroot_get_root (global_sysroot); ot_repo = rpmostreed_sysroot_get_repo (global_sysroot); @@ -183,8 +181,6 @@ os_handle_get_deployments_rpm_diff (RPMOSTreeOS *interface, &local_error); out: - rpmostreed_sysroot_reader_unlock (global_sysroot); - if (local_error != NULL) { g_dbus_method_invocation_take_error (invocation, local_error); @@ -216,8 +212,6 @@ os_handle_get_cached_update_rpm_diff (RPMOSTreeOS *interface, global_sysroot = rpmostreed_sysroot_get (); - rpmostreed_sysroot_reader_lock (global_sysroot); - ot_sysroot = rpmostreed_sysroot_get_root (global_sysroot); ot_repo = rpmostreed_sysroot_get_repo (global_sysroot); @@ -266,8 +260,6 @@ os_handle_get_cached_update_rpm_diff (RPMOSTreeOS *interface, comp_ref); out: - rpmostreed_sysroot_reader_unlock (global_sysroot); - if (local_error != NULL) { g_dbus_method_invocation_take_error (invocation, local_error); @@ -735,8 +727,6 @@ os_handle_get_cached_rebase_rpm_diff (RPMOSTreeOS *interface, global_sysroot = rpmostreed_sysroot_get (); - rpmostreed_sysroot_reader_lock (global_sysroot); - ot_sysroot = rpmostreed_sysroot_get_root (global_sysroot); ot_repo = rpmostreed_sysroot_get_repo (global_sysroot); @@ -769,7 +759,6 @@ os_handle_get_cached_rebase_rpm_diff (RPMOSTreeOS *interface, comp_ref); out: - rpmostreed_sysroot_reader_unlock (global_sysroot); if (local_error == NULL) { g_dbus_method_invocation_return_value (invocation, new_variant_diff_result (value, details)); @@ -851,7 +840,6 @@ os_handle_get_cached_deploy_rpm_diff (RPMOSTreeOS *interface, const char *arg_revision, const char * const *arg_packages) { - RpmostreedSysroot *global_sysroot; const char *base_checksum; const char *osname; OstreeSysroot *ot_sysroot = NULL; @@ -867,10 +855,6 @@ os_handle_get_cached_deploy_rpm_diff (RPMOSTreeOS *interface, /* XXX Ignoring arg_packages for now. */ - global_sysroot = rpmostreed_sysroot_get (); - - rpmostreed_sysroot_reader_lock (global_sysroot); - ot_sysroot = rpmostreed_sysroot_get_root (rpmostreed_sysroot_get ()); ot_repo = rpmostreed_sysroot_get_repo (rpmostreed_sysroot_get ()); @@ -916,8 +900,6 @@ os_handle_get_cached_deploy_rpm_diff (RPMOSTreeOS *interface, NULL); out: - rpmostreed_sysroot_reader_unlock (global_sysroot); - if (local_error != NULL) { g_dbus_method_invocation_take_error (invocation, local_error); diff --git a/src/daemon/rpmostreed-sysroot.c b/src/daemon/rpmostreed-sysroot.c index 8bd83737..58b3c947 100644 --- a/src/daemon/rpmostreed-sysroot.c +++ b/src/daemon/rpmostreed-sysroot.c @@ -59,7 +59,6 @@ struct _RpmostreedSysroot { RpmostreedTransactionMonitor *transaction_monitor; GHashTable *os_interfaces; - GRWLock children_lock; /* The OS interface's various diff methods can run concurrently with * transactions, which is safe except when the transaction is writing @@ -333,14 +332,10 @@ handle_get_os (RPMOSTreeSysroot *object, goto out; } - g_rw_lock_reader_lock (&self->children_lock); - os_interface = g_hash_table_lookup (self->os_interfaces, arg_name); if (os_interface != NULL) g_object_ref (os_interface); - g_rw_lock_reader_unlock (&self->children_lock); - if (os_interface != NULL) { const char *object_path; @@ -522,16 +517,12 @@ sysroot_dispose (GObject *object) } } - g_rw_lock_writer_lock (&self->children_lock); - /* Tracked os paths are responsible to unpublish themselves */ g_hash_table_iter_init (&iter, self->os_interfaces); while (g_hash_table_iter_next (&iter, NULL, &value)) g_object_run_dispose (value); g_hash_table_remove_all (self->os_interfaces); - g_rw_lock_writer_unlock (&self->children_lock); - g_clear_object (&self->transaction_monitor); G_OBJECT_CLASS (rpmostreed_sysroot_parent_class)->dispose (object); @@ -545,9 +536,6 @@ sysroot_finalize (GObject *object) g_hash_table_unref (self->os_interfaces); - g_rw_lock_clear (&self->children_lock); - g_rw_lock_clear (&self->method_rw_lock); - g_clear_object (&self->cancellable); g_clear_object (&self->monitor); @@ -568,9 +556,6 @@ rpmostreed_sysroot_init (RpmostreedSysroot *self) self->os_interfaces = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify) g_object_unref); - g_rw_lock_init (&self->children_lock); - g_rw_lock_init (&self->method_rw_lock); - self->monitor = NULL; self->transaction_monitor = rpmostreed_transaction_monitor_new (); @@ -630,14 +615,11 @@ rpmostreed_sysroot_reload (RpmostreedSysroot *self, gboolean ret = FALSE; gboolean did_change; - g_rw_lock_writer_lock (&self->children_lock); - if (!sysroot_populate_deployments_unlocked (self, &did_change, error)) goto out; ret = TRUE; out: - g_rw_lock_writer_unlock (&self->children_lock); if (ret && did_change) g_signal_emit (self, signals[UPDATED], 0); return ret; @@ -762,37 +744,3 @@ rpmostreed_sysroot_get (void) g_assert (_sysroot_instance); return _sysroot_instance; } - -void -rpmostreed_sysroot_reader_lock (RpmostreedSysroot *self) -{ - g_return_if_fail (RPMOSTREED_IS_SYSROOT (self)); - - g_rw_lock_reader_lock (&self->method_rw_lock); -} - -void -rpmostreed_sysroot_reader_unlock (RpmostreedSysroot *self) -{ - g_return_if_fail (RPMOSTREED_IS_SYSROOT (self)); - - g_rw_lock_reader_unlock (&self->method_rw_lock); -} - -void -rpmostreed_sysroot_writer_lock (RpmostreedSysroot *self) -{ - g_return_if_fail (RPMOSTREED_IS_SYSROOT (self)); - - g_rw_lock_writer_lock (&self->method_rw_lock); -} - -void -rpmostreed_sysroot_writer_unlock (RpmostreedSysroot *self) -{ - g_return_if_fail (RPMOSTREED_IS_SYSROOT (self)); - - g_rw_lock_writer_unlock (&self->method_rw_lock); -} - -/* ---------------------------------------------------------------------------------------------------- */ diff --git a/src/daemon/rpmostreed-sysroot.h b/src/daemon/rpmostreed-sysroot.h index eea2a4a6..dc54a365 100644 --- a/src/daemon/rpmostreed-sysroot.h +++ b/src/daemon/rpmostreed-sysroot.h @@ -47,7 +47,3 @@ gboolean rpmostreed_sysroot_load_state (RpmostreedSysroot *self GError **error); void rpmostreed_sysroot_emit_update (RpmostreedSysroot *self); -void rpmostreed_sysroot_reader_lock (RpmostreedSysroot *self); -void rpmostreed_sysroot_reader_unlock (RpmostreedSysroot *self); -void rpmostreed_sysroot_writer_lock (RpmostreedSysroot *self); -void rpmostreed_sysroot_writer_unlock (RpmostreedSysroot *self); diff --git a/src/daemon/rpmostreed-transaction-types.c b/src/daemon/rpmostreed-transaction-types.c index 37a97c70..e80eccdd 100644 --- a/src/daemon/rpmostreed-transaction-types.c +++ b/src/daemon/rpmostreed-transaction-types.c @@ -20,6 +20,7 @@ #include "ostree.h" #include +#include #include "rpmostreed-transaction-types.h" #include "rpmostreed-transaction.h" @@ -78,70 +79,6 @@ out: return ret; } -static gboolean -safe_sysroot_upgrader_deploy (OstreeSysrootUpgrader *upgrader, - GCancellable *cancellable, - GError **error) -{ - RpmostreedSysroot *global_sysroot = rpmostreed_sysroot_get (); - gboolean success; - - rpmostreed_sysroot_writer_lock (global_sysroot); - - success = ostree_sysroot_upgrader_deploy (upgrader, cancellable, error); - - rpmostreed_sysroot_writer_unlock (global_sysroot); - - return success; -} - -static gboolean -safe_sysroot_upgrader_pull_package_diff (OstreeSysrootUpgrader *upgrader, - OstreeAsyncProgress *progress, - OstreeSysrootUpgraderPullFlags upgrader_flags, - gboolean *out_changed, - GCancellable *cancellable, - GError **error) -{ - RpmostreedSysroot *global_sysroot = rpmostreed_sysroot_get (); - gboolean success; - - rpmostreed_sysroot_writer_lock (global_sysroot); - - success = ostree_sysroot_upgrader_pull_one_dir (upgrader, - "/usr/share/rpm", - 0, upgrader_flags, - progress, - out_changed, - cancellable, - error); - - rpmostreed_sysroot_writer_unlock (global_sysroot); - - return success; -} - -static gboolean -safe_sysroot_write_deployments (OstreeSysroot *sysroot, - GPtrArray *deployments, - GCancellable *cancellable, - GError **error) -{ - RpmostreedSysroot *global_sysroot = rpmostreed_sysroot_get (); - gboolean success; - - rpmostreed_sysroot_writer_lock (global_sysroot); - - success = ostree_sysroot_write_deployments (sysroot, - deployments, - cancellable, - error); - - rpmostreed_sysroot_writer_unlock (global_sysroot); - - return success; -} - /* ============================= Package Diff ============================= */ typedef struct { @@ -286,9 +223,13 @@ package_diff_transaction_execute (RpmostreedTransaction *transaction, "Updating from: %s", origin_description); - if (!safe_sysroot_upgrader_pull_package_diff (upgrader, progress, - upgrader_flags, &changed, - cancellable, error)) + if (!ostree_sysroot_upgrader_pull_one_dir (upgrader, + "/usr/share/rpm", + 0, upgrader_flags, + progress, + &changed, + cancellable, + error)) goto out; rpmostree_transaction_emit_progress_end (RPMOSTREE_TRANSACTION (transaction)); @@ -342,7 +283,7 @@ rpmostreed_transaction_new_package_diff (GDBusMethodInvocation *invocation, self = g_initable_new (package_diff_transaction_get_type (), cancellable, error, "invocation", invocation, - "sysroot", sysroot, + "sysroot-path", gs_file_get_path_cached (ostree_sysroot_get_path (sysroot)), NULL); if (self != NULL) @@ -430,10 +371,10 @@ rollback_transaction_execute (RpmostreedTransaction *transaction, /* if default changed write it */ if (old_deployments->pdata[0] != new_deployments->pdata[0]) { - if (!safe_sysroot_write_deployments (sysroot, - new_deployments, - cancellable, - error)) + if (!ostree_sysroot_write_deployments (sysroot, + new_deployments, + cancellable, + error)) goto out; } @@ -479,7 +420,7 @@ rpmostreed_transaction_new_rollback (GDBusMethodInvocation *invocation, self = g_initable_new (rollback_transaction_get_type (), cancellable, error, "invocation", invocation, - "sysroot", sysroot, + "sysroot-path", gs_file_get_path_cached (ostree_sysroot_get_path (sysroot)), NULL); if (self != NULL) @@ -550,10 +491,10 @@ clear_rollback_transaction_execute (RpmostreedTransaction *transaction, g_ptr_array_remove_index (deployments, rollback_index); - if (!safe_sysroot_write_deployments (sysroot, - deployments, - cancellable, - error)) + if (!ostree_sysroot_write_deployments (sysroot, + deployments, + cancellable, + error)) goto out; if (self->reboot) @@ -598,7 +539,7 @@ rpmostreed_transaction_new_clear_rollback (GDBusMethodInvocation *invocation, self = g_initable_new (clear_rollback_transaction_get_type (), cancellable, error, "invocation", invocation, - "sysroot", sysroot, + "sysroot-path", gs_file_get_path_cached (ostree_sysroot_get_path (sysroot)), NULL); if (self != NULL) @@ -705,7 +646,7 @@ upgrade_transaction_execute (RpmostreedTransaction *transaction, if (changed) { - if (!safe_sysroot_upgrader_deploy (upgrader, cancellable, error)) + if (!ostree_sysroot_upgrader_deploy (upgrader, cancellable, error)) goto out; if (self->reboot) @@ -755,7 +696,7 @@ rpmostreed_transaction_new_upgrade (GDBusMethodInvocation *invocation, self = g_initable_new (upgrade_transaction_get_type (), cancellable, error, "invocation", invocation, - "sysroot", sysroot, + "sysroot-path", gs_file_get_path_cached (ostree_sysroot_get_path (sysroot)), NULL); if (self != NULL) @@ -848,7 +789,7 @@ rebase_transaction_execute (RpmostreedTransaction *transaction, rpmostree_transaction_emit_progress_end (RPMOSTREE_TRANSACTION (transaction)); - if (!safe_sysroot_upgrader_deploy (upgrader, cancellable, error)) + if (!ostree_sysroot_upgrader_deploy (upgrader, cancellable, error)) goto out; if (!self->skip_purge) @@ -911,7 +852,7 @@ rpmostreed_transaction_new_rebase (GDBusMethodInvocation *invocation, self = g_initable_new (rebase_transaction_get_type (), cancellable, error, "invocation", invocation, - "sysroot", sysroot, + "sysroot-path", gs_file_get_path_cached (ostree_sysroot_get_path (sysroot)), NULL); if (self != NULL) @@ -1052,8 +993,8 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, if (changed) { - if (!safe_sysroot_upgrader_deploy (upgrader, cancellable, error)) - goto out; + if (!ostree_sysroot_upgrader_deploy (upgrader, cancellable, error)) + goto out; if (self->reboot) rpmostreed_reboot (cancellable, error); @@ -1103,7 +1044,7 @@ rpmostreed_transaction_new_deploy (GDBusMethodInvocation *invocation, self = g_initable_new (deploy_transaction_get_type (), cancellable, error, "invocation", invocation, - "sysroot", sysroot, + "sysroot-path", gs_file_get_path_cached (ostree_sysroot_get_path (sysroot)), NULL); if (self != NULL) diff --git a/src/daemon/rpmostreed-transaction.c b/src/daemon/rpmostreed-transaction.c index 79da919e..878168fd 100644 --- a/src/daemon/rpmostreed-transaction.c +++ b/src/daemon/rpmostreed-transaction.c @@ -29,7 +29,11 @@ struct _RpmostreedTransactionPrivate { GDBusMethodInvocation *invocation; GCancellable *cancellable; - /* Locked for the duration of the transaction. */ + /* For the duration of the transaction, we hold a ref to a new + * OstreeSysroot instance (to avoid any threading issues), and we + * also lock it. + */ + char *sysroot_path; OstreeSysroot *sysroot; GDBusServer *server; @@ -45,7 +49,7 @@ enum { PROP_0, PROP_ACTIVE, PROP_INVOCATION, - PROP_SYSROOT + PROP_SYSROOT_PATH }; enum { @@ -358,8 +362,9 @@ transaction_set_property (GObject *object, { case PROP_INVOCATION: priv->invocation = g_value_dup_object (value); - case PROP_SYSROOT: - priv->sysroot = g_value_dup_object (value); + break; + case PROP_SYSROOT_PATH: + priv->sysroot_path = g_value_dup_string (value); break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); @@ -381,8 +386,8 @@ transaction_get_property (GObject *object, case PROP_INVOCATION: g_value_set_object (value, priv->invocation); break; - case PROP_SYSROOT: - g_value_set_object (value, priv->sysroot); + case PROP_SYSROOT_PATH: + g_value_set_string (value, priv->sysroot_path); break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); @@ -405,6 +410,7 @@ transaction_dispose (GObject *object) g_clear_object (&priv->cancellable); g_clear_object (&priv->sysroot); g_clear_object (&priv->server); + g_clear_pointer (&priv->sysroot_path, g_free); g_clear_pointer (&priv->finished_params, (GDestroyNotify) g_variant_unref); @@ -490,10 +496,21 @@ transaction_initable_init (GInitable *initable, G_CALLBACK (transaction_new_connection_cb), self, 0); - if (priv->sysroot != NULL) + if (priv->sysroot_path != NULL) { + g_autoptr(GFile) tmp_path = g_file_new_for_path (priv->sysroot_path); gboolean lock_acquired = FALSE; + /* We create a *new* sysroot to avoid threading issues like data + * races - OstreeSysroot has no internal locking. Efficiency + * could be improved with a "clone" operation to avoid reloading + * everything from disk. + */ + priv->sysroot = ostree_sysroot_new (tmp_path); + + if (!ostree_sysroot_load (priv->sysroot, cancellable, error)) + goto out; + if (!ostree_sysroot_try_lock (priv->sysroot, &lock_acquired, error)) goto out; @@ -630,11 +647,11 @@ rpmostreed_transaction_class_init (RpmostreedTransactionClass *class) G_PARAM_STATIC_STRINGS)); g_object_class_install_property (object_class, - PROP_SYSROOT, - g_param_spec_object ("sysroot", - "Sysroot", - "An OstreeSysroot instance", - OSTREE_TYPE_SYSROOT, + PROP_SYSROOT_PATH, + g_param_spec_string ("sysroot-path", + "Sysroot path", + "An OstreeSysroot path", + "", G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS));