Skip to content

Commit

Permalink
lib/repo: Add MT support for transaction_set_ref(), clarify MT rules
Browse files Browse the repository at this point in the history 
For rpm-ostree I'd like to do importing in parallel with threads; the code is
*almost* ready for that except today it calls
`ostree_repo_transaction_set_ref()`.

Looking at the code, there's really a "transaction" struct here,
not just stats.  Let's lift that struct out, and move the refs
into it under the existing lock.

Clarify the documentation around multithreading for various functions.

Closes: #1358
Approved by: jlebon
  • Loading branch information
@cgwalters @rh-atomic-bot
cgwalters authored and rh-atomic-bot committed Dec 4, 2017
1 parent 8d6b30c commit 89a57bb
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 69 deletions.
151 changes: 93 additions & 58 deletions src/libostree/ostree-repo-commit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -658,19 +658,19 @@ write_content_object (OstreeRepo *self,
/* Free space check; only applies during transactions */
if (self->min_free_space_percent > 0 && self->in_transaction)
{
g_mutex_lock (&self->txn_stats_lock);
g_assert_cmpint (self->txn_blocksize, >, 0);
const fsblkcnt_t object_blocks = (size / self->txn_blocksize) + 1;
if (object_blocks > self->max_txn_blocks)
g_mutex_lock (&self->txn_lock);
g_assert_cmpint (self->txn.blocksize, >, 0);
const fsblkcnt_t object_blocks = (size / self->txn.blocksize) + 1;
if (object_blocks > self->txn.max_blocks)
{
g_mutex_unlock (&self->txn_stats_lock);
g_autofree char *formatted_required = g_format_size ((guint64)object_blocks * self->txn_blocksize);
g_mutex_unlock (&self->txn_lock);
g_autofree char *formatted_required = g_format_size ((guint64)object_blocks * self->txn.blocksize);
return glnx_throw (error, "min-free-space-percent '%u%%' would be exceeded, %s more required",
self->min_free_space_percent, formatted_required);
}
/* This is the main bit that needs mutex protection */
self->max_txn_blocks -= object_blocks;
g_mutex_unlock (&self->txn_stats_lock);
self->txn.max_blocks -= object_blocks;
g_mutex_unlock (&self->txn_lock);
}

/* For regular files, we create them with default mode, and only
Expand Down Expand Up @@ -777,9 +777,9 @@ write_content_object (OstreeRepo *self,
/* If we already have it, just update the stats. */
if (have_obj)
{
g_mutex_lock (&self->txn_stats_lock);
self->txn_stats.content_objects_total++;
g_mutex_unlock (&self->txn_stats_lock);
g_mutex_lock (&self->txn_lock);
self->txn.stats.content_objects_total++;
g_mutex_unlock (&self->txn_lock);
if (out_csum)
*out_csum = ostree_checksum_to_bytes (actual_checksum);
/* Note early return */
Expand Down Expand Up @@ -849,11 +849,11 @@ write_content_object (OstreeRepo *self,
}

/* Update statistics */
g_mutex_lock (&self->txn_stats_lock);
self->txn_stats.content_objects_written++;
self->txn_stats.content_bytes_written += file_object_length;
self->txn_stats.content_objects_total++;
g_mutex_unlock (&self->txn_stats_lock);
g_mutex_lock (&self->txn_lock);
self->txn.stats.content_objects_written++;
self->txn.stats.content_bytes_written += file_object_length;
self->txn.stats.content_objects_total++;
g_mutex_unlock (&self->txn_lock);

if (out_csum)
{
Expand Down Expand Up @@ -1018,9 +1018,9 @@ write_metadata_object (OstreeRepo *self,
*/
if (have_obj)
{
g_mutex_lock (&self->txn_stats_lock);
self->txn_stats.metadata_objects_total++;
g_mutex_unlock (&self->txn_stats_lock);
g_mutex_lock (&self->txn_lock);
self->txn.stats.metadata_objects_total++;
g_mutex_unlock (&self->txn_lock);

if (out_csum)
*out_csum = ostree_checksum_to_bytes (actual_checksum);
Expand Down Expand Up @@ -1090,10 +1090,10 @@ write_metadata_object (OstreeRepo *self,
}

/* Update the stats, note we both wrote one and add to total */
g_mutex_lock (&self->txn_stats_lock);
self->txn_stats.metadata_objects_written++;
self->txn_stats.metadata_objects_total++;
g_mutex_unlock (&self->txn_stats_lock);
g_mutex_lock (&self->txn_lock);
self->txn.stats.metadata_objects_written++;
self->txn.stats.metadata_objects_total++;
g_mutex_unlock (&self->txn_lock);

if (out_csum)
*out_csum = ostree_checksum_to_bytes (actual_checksum);
Expand Down Expand Up @@ -1259,6 +1259,8 @@ devino_cache_lookup (OstreeRepo *self,
* existing ostree objects, then this will speed up considerably, so call it
* before you call ostree_write_directory_to_mtree() or similar. However,
* ostree_repo_devino_cache_new() is better as it avoids scanning all objects.
*
* Multithreading: This function is *not* MT safe.
*/
gboolean
ostree_repo_scan_hardlinks (OstreeRepo *self,
Expand Down Expand Up @@ -1287,8 +1289,17 @@ ostree_repo_scan_hardlinks (OstreeRepo *self,
* ostree_repo_commit_transaction(), or abort the transaction with
* ostree_repo_abort_transaction().
*
* Currently, transactions are not atomic, and aborting a transaction
* will not erase any data you write during the transaction.
* Currently, transactions may result in partial commits or data in the target
* repository if interrupted during ostree_repo_commit_transaction(), and
* further writing refs is also not currently atomic.
*
* There can be at most one transaction active on a repo at a time per instance
* of `OstreeRepo`; however, it is safe to have multiple threads writing objects
* on a single `OstreeRepo` instance as long as their lifetime is bounded by the
* transaction.
*
* Multithreading: This function is *not* MT safe; only one transaction can be
* active at a time.
*/
gboolean
ostree_repo_prepare_transaction (OstreeRepo *self,
Expand All @@ -1299,31 +1310,31 @@ ostree_repo_prepare_transaction (OstreeRepo *self,

g_return_val_if_fail (self->in_transaction == FALSE, FALSE);

memset (&self->txn_stats, 0, sizeof (OstreeRepoTransactionStats));
memset (&self->txn.stats, 0, sizeof (OstreeRepoTransactionStats));

self->in_transaction = TRUE;
if (self->min_free_space_percent > 0)
{
struct statvfs stvfsbuf;
if (TEMP_FAILURE_RETRY (fstatvfs (self->repo_dir_fd, &stvfsbuf)) < 0)
return glnx_throw_errno_prefix (error, "fstatvfs");
g_mutex_lock (&self->txn_stats_lock);
self->txn_blocksize = stvfsbuf.f_bsize;
g_mutex_lock (&self->txn_lock);
self->txn.blocksize = stvfsbuf.f_bsize;
/* Convert fragment to blocks to compute the total */
guint64 total_blocks = (stvfsbuf.f_frsize * stvfsbuf.f_blocks) / stvfsbuf.f_bsize;
/* Use the appropriate free block count if we're unprivileged */
guint64 bfree = (getuid () != 0 ? stvfsbuf.f_bavail : stvfsbuf.f_bfree);
guint64 reserved_blocks = ((double)total_blocks) * (self->min_free_space_percent/100.0);
if (bfree > reserved_blocks)
self->max_txn_blocks = bfree - reserved_blocks;
self->txn.max_blocks = bfree - reserved_blocks;
else
{
g_mutex_unlock (&self->txn_stats_lock);
g_autofree char *formatted_free = g_format_size (bfree * self->txn_blocksize);
g_mutex_unlock (&self->txn_lock);
g_autofree char *formatted_free = g_format_size (bfree * self->txn.blocksize);
return glnx_throw (error, "min-free-space-percent '%u%%' would be exceeded, %s available",
self->min_free_space_percent, formatted_free);
}
g_mutex_unlock (&self->txn_stats_lock);
g_mutex_unlock (&self->txn_lock);
}

gboolean ret_transaction_resume = FALSE;
Expand Down Expand Up @@ -1547,10 +1558,10 @@ cleanup_tmpdir (OstreeRepo *self,
static void
ensure_txn_refs (OstreeRepo *self)
{
if (self->txn_refs == NULL)
self->txn_refs = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
if (self->txn_collection_refs == NULL)
self->txn_collection_refs = g_hash_table_new_full (ostree_collection_ref_hash,
if (self->txn.refs == NULL)
self->txn.refs = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
if (self->txn.collection_refs == NULL)
self->txn.collection_refs = g_hash_table_new_full (ostree_collection_ref_hash,
ostree_collection_ref_equal,
(GDestroyNotify) ostree_collection_ref_free,
g_free);
Expand All @@ -1565,6 +1576,8 @@ ensure_txn_refs (OstreeRepo *self)
* Like ostree_repo_transaction_set_ref(), but takes concatenated
* @refspec format as input instead of separate remote and name
* arguments.
*
* Multithreading: Since v2017.15 this function is MT safe.
*/
void
ostree_repo_transaction_set_refspec (OstreeRepo *self,
Expand All @@ -1573,9 +1586,10 @@ ostree_repo_transaction_set_refspec (OstreeRepo *self,
{
g_return_if_fail (self->in_transaction == TRUE);

g_mutex_lock (&self->txn_lock);
ensure_txn_refs (self);

g_hash_table_replace (self->txn_refs, g_strdup (refspec), g_strdup (checksum));
g_hash_table_replace (self->txn.refs, g_strdup (refspec), g_strdup (checksum));
g_mutex_unlock (&self->txn_lock);
}

/**
Expand All @@ -1592,29 +1606,39 @@ ostree_repo_transaction_set_refspec (OstreeRepo *self,
* Otherwise, if @checksum is %NULL, then record that the ref should
* be deleted.
*
* The change will not be written out immediately, but when the transaction
* is completed with ostree_repo_commit_transaction(). If the transaction
* is instead aborted with ostree_repo_abort_transaction(), no changes will
* be made to the repository.
* The change will be written when the transaction is completed with
* ostree_repo_commit_transaction(); that function takes care of writing all of
* the objects (such as the commit referred to by @checksum) before updating the
* refs. If the transaction is instead aborted with
* ostree_repo_abort_transaction(), no changes to the ref will be made to the
* repository.
*
* Note however that currently writing *multiple* refs is not truly atomic; if
* the process or system is terminated during
* ostree_repo_commit_transaction(), it is possible that just some of the refs
* will have been updated. Your application should take care to handle this
* case.
*
* Multithreading: Since v2017.15 this function is MT safe.
*/
void
ostree_repo_transaction_set_ref (OstreeRepo *self,
const char *remote,
const char *ref,
const char *checksum)
{
char *refspec;

g_return_if_fail (self->in_transaction == TRUE);

ensure_txn_refs (self);

char *refspec;
if (remote)
refspec = g_strdup_printf ("%s:%s", remote, ref);
else
refspec = g_strdup (ref);

g_hash_table_replace (self->txn_refs, refspec, g_strdup (checksum));
g_mutex_lock (&self->txn_lock);
ensure_txn_refs (self);
g_hash_table_replace (self->txn.refs, refspec, g_strdup (checksum));
g_mutex_unlock (&self->txn_lock);
}

/**
Expand All @@ -1634,6 +1658,8 @@ ostree_repo_transaction_set_ref (OstreeRepo *self,
* is instead aborted with ostree_repo_abort_transaction(), no changes will
* be made to the repository.
*
* Multithreading: Since v2017.15 this function is MT safe.
*
* Since: 2017.8
*/
void
Expand All @@ -1646,10 +1672,11 @@ ostree_repo_transaction_set_collection_ref (OstreeRepo *self,
g_return_if_fail (ref != NULL);
g_return_if_fail (checksum == NULL || ostree_validate_checksum_string (checksum, NULL));

g_mutex_lock (&self->txn_lock);
ensure_txn_refs (self);

g_hash_table_replace (self->txn_collection_refs,
g_hash_table_replace (self->txn.collection_refs,
ostree_collection_ref_dup (ref), g_strdup (checksum));
g_mutex_unlock (&self->txn_lock);
}

/**
Expand All @@ -1664,6 +1691,8 @@ ostree_repo_transaction_set_collection_ref (OstreeRepo *self,
* This is like ostree_repo_transaction_set_ref(), except it may be
* invoked outside of a transaction. This is presently safe for the
* case where we're creating or overwriting an existing ref.
*
* Multithreading: This function is MT safe.
*/
gboolean
ostree_repo_set_ref_immediate (OstreeRepo *self,
Expand Down Expand Up @@ -1745,6 +1774,12 @@ ostree_repo_set_collection_ref_immediate (OstreeRepo *self,
* Complete the transaction. Any refs set with
* ostree_repo_transaction_set_ref() or
* ostree_repo_transaction_set_refspec() will be written out.
*
* Note that if multiple threads are performing writes, all such threads must
* have terminated before this function is invoked.
*
* Multithreading: This function is *not* MT safe; only one transaction can be
* active at a time.
*/
gboolean
ostree_repo_commit_transaction (OstreeRepo *self,
Expand Down Expand Up @@ -1782,23 +1817,23 @@ ostree_repo_commit_transaction (OstreeRepo *self,
if (self->loose_object_devino_hash)
g_hash_table_remove_all (self->loose_object_devino_hash);

if (self->txn_refs)
if (!_ostree_repo_update_refs (self, self->txn_refs, cancellable, error))
if (self->txn.refs)
if (!_ostree_repo_update_refs (self, self->txn.refs, cancellable, error))
return FALSE;
g_clear_pointer (&self->txn_refs, g_hash_table_destroy);
g_clear_pointer (&self->txn.refs, g_hash_table_destroy);

if (self->txn_collection_refs)
if (!_ostree_repo_update_collection_refs (self, self->txn_collection_refs, cancellable, error))
if (self->txn.collection_refs)
if (!_ostree_repo_update_collection_refs (self, self->txn.collection_refs, cancellable, error))
return FALSE;
g_clear_pointer (&self->txn_collection_refs, g_hash_table_destroy);
g_clear_pointer (&self->txn.collection_refs, g_hash_table_destroy);

self->in_transaction = FALSE;

if (!ot_ensure_unlinked_at (self->repo_dir_fd, "transaction", 0))
return FALSE;

if (out_stats)
*out_stats = self->txn_stats;
*out_stats = self->txn.stats;

return TRUE;
}
Expand Down Expand Up @@ -1829,8 +1864,8 @@ ostree_repo_abort_transaction (OstreeRepo *self,
if (self->loose_object_devino_hash)
g_hash_table_remove_all (self->loose_object_devino_hash);

g_clear_pointer (&self->txn_refs, g_hash_table_destroy);
g_clear_pointer (&self->txn_collection_refs, g_hash_table_destroy);
g_clear_pointer (&self->txn.refs, g_hash_table_destroy);
g_clear_pointer (&self->txn.collection_refs, g_hash_table_destroy);

glnx_tmpdir_unset (&self->commit_stagedir);
glnx_release_lock_file (&self->commit_stagedir_lock);
Expand Down
18 changes: 11 additions & 7 deletions src/libostree/ostree-repo-private.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,15 @@ typedef enum {
OSTREE_REPO_SYSROOT_KIND_IS_SYSROOT_OSTREE, /* We match /ostree/repo */
} OstreeRepoSysrootKind;

typedef struct {
GHashTable *refs; /* (element-type utf8 utf8) */
GHashTable *collection_refs; /* (element-type OstreeCollectionRef utf8) */
OstreeRepoTransactionStats stats;
/* Implementation of min-free-space-percent */
gulong blocksize;
fsblkcnt_t max_blocks;
} OstreeRepoTxn;

/**
* OstreeRepo:
*
Expand Down Expand Up @@ -109,13 +118,8 @@ struct OstreeRepo {
GWeakRef sysroot; /* Weak to avoid a circular ref; see also `is_system` */
char *remotes_config_dir;

GHashTable *txn_refs; /* (element-type utf8 utf8) */
GHashTable *txn_collection_refs; /* (element-type OstreeCollectionRef utf8) */
GMutex txn_stats_lock;
OstreeRepoTransactionStats txn_stats;
/* Implementation of min-free-space-percent */
gulong txn_blocksize;
fsblkcnt_t max_txn_blocks;
GMutex txn_lock;
OstreeRepoTxn txn;

GMutex cache_lock;
guint dirmeta_cache_refcount;
Expand Down
8 changes: 4 additions & 4 deletions src/libostree/ostree-repo.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -505,13 +505,13 @@ ostree_repo_finalize (GObject *object)
g_hash_table_destroy (self->updated_uncompressed_dirs);
if (self->config)
g_key_file_free (self->config);
g_clear_pointer (&self->txn_refs, g_hash_table_destroy);
g_clear_pointer (&self->txn_collection_refs, g_hash_table_destroy);
g_clear_pointer (&self->txn.refs, g_hash_table_destroy);
g_clear_pointer (&self->txn.collection_refs, g_hash_table_destroy);
g_clear_error (&self->writable_error);
g_clear_pointer (&self->object_sizes, (GDestroyNotify) g_hash_table_unref);
g_clear_pointer (&self->dirmeta_cache, (GDestroyNotify) g_hash_table_unref);
g_mutex_clear (&self->cache_lock);
g_mutex_clear (&self->txn_stats_lock);
g_mutex_clear (&self->txn_lock);
g_free (self->collection_id);

g_clear_pointer (&self->remotes, g_hash_table_destroy);
Expand Down Expand Up @@ -672,7 +672,7 @@ ostree_repo_init (OstreeRepo *self)
test_error_keys, G_N_ELEMENTS (test_error_keys));

g_mutex_init (&self->cache_lock);
g_mutex_init (&self->txn_stats_lock);
g_mutex_init (&self->txn_lock);

self->remotes = g_hash_table_new_full (g_str_hash, g_str_equal,
(GDestroyNotify) NULL,
Expand Down

0 comments on commit 89a57bb

Please sign in to comment.