Skip to content

Commit

Permalink
deploy: Remove global sync by default
Browse files Browse the repository at this point in the history
Our previous change here was not actually sufficient for
the ceph case, because what (I think) is happening is that
our other `syncfs()` invocation is getting blocked on some
kernel mutexes that are used in `sync`, and that's causing the
process to fully block.

We should not be dependent on a full filesystem `sync`, only
on the sync of the sysroot and boot filesystems.

Anyone who *does* want this behavior could inject an override
for `ostree-finalize-staged.service` that overrides `ExecStop`
to add a run of `sync`.
  • Loading branch information
cgwalters committed Aug 3, 2023
1 parent 09160c1 commit aea3c2e
Showing 1 changed file with 1 addition and 85 deletions.
86 changes: 1 addition & 85 deletions src/libostree/ostree-sysroot-deploy.c
Original file line number Diff line number Diff line change
Expand Up @@ -1580,45 +1580,8 @@ typedef struct
{
guint64 root_syncfs_msec;
guint64 boot_syncfs_msec;
guint64 extra_syncfs_msec;
} SyncStats;

typedef struct
{
int ref_count; /* atomic */
bool success;
GMutex mutex;
GCond cond;
} SyncData;

static void
sync_data_unref (SyncData *data)
{
if (!g_atomic_int_dec_and_test (&data->ref_count))
return;
g_mutex_clear (&data->mutex);
g_cond_clear (&data->cond);
g_free (data);
}

// An asynchronous sync() call. See below.
static void *
sync_in_thread (void *ptr)
{
SyncData *syncdata = ptr;
// Ensure that the caller is blocked waiting
g_mutex_lock (&syncdata->mutex);
ot_journal_print (LOG_INFO, "Starting global sync()");
sync ();
ot_journal_print (LOG_INFO, "Completed global sync()");
// Signal success
syncdata->success = true;
g_cond_broadcast (&syncdata->cond);
g_mutex_unlock (&syncdata->mutex);
sync_data_unref (syncdata);
return NULL;
}

/* First, sync the root directory as well as /var and /boot which may
* be separate mount points. Then *in addition*, do a global
* `sync()`.
Expand Down Expand Up @@ -1649,52 +1612,6 @@ full_system_sync (OstreeSysroot *self, SyncStats *out_stats, GCancellable *cance
end_msec = g_get_monotonic_time () / 1000;
out_stats->boot_syncfs_msec = (end_msec - start_msec);

/* And now out of an excess of conservativism, we still invoke
* sync(). The advantage of still using `syncfs()` above is that we
* actually get error codes out of that API, and we more clearly
* delineate what we actually want to sync in the future when this
* global sync call is removed.
*
* Due to https://bugzilla.redhat.com/show_bug.cgi?id=2003532 which is
* a bug in the interaction between Ceph and systemd, we have a capped
* timeout of 5s on the sync here. In general, we don't actually want
* to "own" flushing data on *all* mounted filesystems as part of
* our process. Again, we're only keeping the `sync` here out of
* conservatisim. We could likely just remove it and e.g. systemd
* would take care of sync as part of unmounting individual filesystems.
*/
start_msec = g_get_monotonic_time () / 1000;
if (!(self->opt_flags & OSTREE_SYSROOT_GLOBAL_OPT_SKIP_SYNC))
{
SyncData *syncdata = g_new (SyncData, 1);
syncdata->ref_count = 2; // One for us, one for thread
syncdata->success = FALSE;
g_mutex_init (&syncdata->mutex);
g_cond_init (&syncdata->cond);
g_mutex_lock (&syncdata->mutex);
GThread *worker = g_thread_new ("ostree sync", sync_in_thread, syncdata);
// Wait up to 5 seconds for sync() to finish
gint64 end_time = g_get_monotonic_time () + (5 * G_TIME_SPAN_SECOND);
while (!syncdata->success)
{
if (!g_cond_wait_until (&syncdata->cond, &syncdata->mutex, end_time))
{
ot_journal_print (LOG_INFO, "Timed out waiting for global sync()");
break;
}
}
g_mutex_unlock (&syncdata->mutex);
sync_data_unref (syncdata);
// We can't join the thread here, for two reasons. First, the whole
// point here is to avoid blocking on `sync`. The other reason is
// today there is no return value on success from `sync`, so there's
// no point to joining the thread even if we had a nonblocking mechanism
// to do so.
g_thread_unref (worker);
}
end_msec = g_get_monotonic_time () / 1000;
out_stats->extra_syncfs_msec = (end_msec - start_msec);

return TRUE;
}

Expand Down Expand Up @@ -2965,8 +2882,7 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n
bootloader_is_atomic ? "yes" : "no", "OSTREE_DID_BOOTSWAP=%s",
requires_new_bootversion ? "yes" : "no", "OSTREE_N_DEPLOYMENTS=%u", new_deployments->len,
"OSTREE_SYNCFS_ROOT_MSEC=%" G_GUINT64_FORMAT, syncstats.root_syncfs_msec,
"OSTREE_SYNCFS_BOOT_MSEC=%" G_GUINT64_FORMAT, syncstats.boot_syncfs_msec,
"OSTREE_SYNCFS_EXTRA_MSEC=%" G_GUINT64_FORMAT, syncstats.extra_syncfs_msec, NULL);
"OSTREE_SYNCFS_BOOT_MSEC=%" G_GUINT64_FORMAT, syncstats.boot_syncfs_msec, NULL);
_ostree_sysroot_emit_journal_msg (self, msg);
}

Expand Down

0 comments on commit aea3c2e

Please sign in to comment.