keremet/rpm-ostree
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

daemon: Drop internal mutexes for sysroot

Browse Source 
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.
This commit is contained in:
Colin Walters 2016-03-07 11:56:22 -05:00
parent 467ecf268d
commit 3e289ffab0
5 changed files with 55 additions and 171 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
src/daemon/rpmostreed-os.c
View File

@ -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);

52
src/daemon/rpmostreed-sysroot.c
View File

@ -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);
}
/* ---------------------------------------------------------------------------------------------------- */

4
src/daemon/rpmostreed-sysroot.h
View File

@ -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);

97
src/daemon/rpmostreed-transaction-types.c
View File

@ -20,6 +20,7 @@
#include "ostree.h"
#include <libglnx.h>
#include <libgsystem.h>
#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,7 +371,7 @@ 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,
if (!ostree_sysroot_write_deployments (sysroot,
new_deployments,
cancellable,
error))
@ -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,7 +491,7 @@ clear_rollback_transaction_execute (RpmostreedTransaction *transaction,
g_ptr_array_remove_index (deployments, rollback_index);
if (!safe_sysroot_write_deployments (sysroot,
if (!ostree_sysroot_write_deployments (sysroot,
deployments,
cancellable,
error))
@ -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,7 +993,7 @@ deploy_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)
@ -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)

41
src/daemon/rpmostreed-transaction.c
View File

@ -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));