[ostree] sysroot: Write symlinks before calling fsync(), then rename after
- From: Colin Walters <walters src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [ostree] sysroot: Write symlinks before calling fsync(), then rename after
- Date: Sun, 25 Oct 2015 15:49:28 +0000 (UTC)
commit 723705b803480dfd9c4090000ffc69fdafbb5c82
Author: Colin Walters <walters verbum org>
Date: Mon Oct 12 21:33:23 2015 -0400
sysroot: Write symlinks before calling fsync(), then rename after
There might be a race here in that we create new symlink files *after*
calling `syncfs`, and they are not guaranteed to end up on disk.
Rework the code so that we create symlinks before, and then only
rename them after (and `fsync()` the directory for good measure).
Additional-fixes-by: Giuseppe Scrivano <gscrivan redhat com>
Tested-by: Giuseppe Scrivano <gscrivan redhat com>
This still needs verification that we're fixing a real bug; but I'm
fairly confident this won't make the fsync situation worse.
https://bugzilla.gnome.org/show_bug.cgi?id=755595
src/libostree/ostree-sysroot-deploy.c | 236 +++++++++++++++++++++++++++-----
src/libotutil/ot-gio-utils.c | 56 --------
src/libotutil/ot-gio-utils.h | 5 -
3 files changed, 199 insertions(+), 98 deletions(-)
---
diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c
index 3aeee1b..f7afe3d 100644
--- a/src/libostree/ostree-sysroot-deploy.c
+++ b/src/libostree/ostree-sysroot-deploy.c
@@ -33,6 +33,53 @@
#define OSTREE_CONFIGMERGE_ID "d3863baec13e4449ab0384684a8af3a7"
#define OSTREE_DEPLOYMENT_COMPLETE_ID "dd440e3e549083b63d0efc7dc15255f1"
+/*
+ * Like symlinkat() but overwrites (atomically) an existing
+ * symlink.
+ */
+static gboolean
+symlink_at_replace (const char *oldpath,
+ int parent_dfd,
+ const char *newpath,
+ GCancellable *cancellable,
+ GError **error)
+{
+ gboolean ret = FALSE;
+ int res;
+ /* Possibly in the future generate a temporary random name here,
+ * would need to move "generate a temporary name" code into
+ * libglnx or glib?
+ */
+ g_autofree char *temppath = g_strconcat (newpath, ".tmp", NULL);
+
+ /* Clean up any stale temporary links */
+ (void) unlinkat (parent_dfd, temppath, 0);
+
+ /* Create the temp link */
+ do
+ res = symlinkat (oldpath, parent_dfd, temppath);
+ while (G_UNLIKELY (res == -1 && errno == EINTR));
+ if (res == -1)
+ {
+ glnx_set_error_from_errno (error);
+ goto out;
+ }
+
+ /* Rename it into place */
+ do
+ res = renameat (parent_dfd, temppath, parent_dfd, newpath);
+ while (G_UNLIKELY (res == -1 && errno == EINTR));
+ if (res == -1)
+ {
+ glnx_set_error_from_errno (error);
+ goto out;
+ }
+
+ ret = TRUE;
+ out:
+ return ret;
+}
+
static gboolean
dirfd_copy_attributes_and_xattrs (int src_parent_dfd,
const char *src_name,
@@ -1046,21 +1093,25 @@ full_system_sync (OstreeSysroot *self,
}
static gboolean
-swap_bootlinks (OstreeSysroot *self,
- int bootversion,
- GPtrArray *new_deployments,
- GCancellable *cancellable,
- GError **error)
+create_new_bootlinks (OstreeSysroot *self,
+ int bootversion,
+ GPtrArray *new_deployments,
+ GCancellable *cancellable,
+ GError **error)
{
gboolean ret = FALSE;
guint i;
int old_subbootversion;
int new_subbootversion;
- g_autoptr(GFile) ostree_dir = g_file_get_child (self->path, "ostree");
- g_autofree char *ostree_bootdir_name = g_strdup_printf ("boot.%d", bootversion);
- g_autoptr(GFile) ostree_bootdir = g_file_resolve_relative_path (ostree_dir, ostree_bootdir_name);
+ glnx_fd_close int ostree_dfd = -1;
+ glnx_fd_close int ostree_subbootdir_dfd = -1;
+ g_autofree char *ostree_bootdir_name = NULL;
g_autofree char *ostree_subbootdir_name = NULL;
- g_autoptr(GFile) ostree_subbootdir = NULL;
+
+ if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_dfd, error))
+ goto out;
+
+ ostree_bootdir_name = g_strdup_printf ("boot.%d", bootversion);
if (bootversion != self->bootversion)
{
@@ -1073,40 +1124,79 @@ swap_bootlinks (OstreeSysroot *self,
new_subbootversion = old_subbootversion == 0 ? 1 : 0;
+ /* Create the "subbootdir", which is a directory holding a symlink farm pointing to
+ * deployments per-osname.
+ */
ostree_subbootdir_name = g_strdup_printf ("boot.%d.%d", bootversion, new_subbootversion);
- ostree_subbootdir = g_file_resolve_relative_path (ostree_dir, ostree_subbootdir_name);
-
- if (!gs_shutil_rm_rf (ostree_subbootdir, cancellable, error))
+ if (!glnx_shutil_rm_rf_at (ostree_dfd, ostree_subbootdir_name, cancellable, error))
goto out;
-
- if (!ot_util_ensure_directory_and_fsync (ostree_subbootdir, cancellable, error))
+ if (!glnx_shutil_mkdir_p_at (ostree_dfd, ostree_subbootdir_name, 0755, cancellable, error))
+ goto out;
+ if (!glnx_opendirat (ostree_dfd, ostree_subbootdir_name, FALSE, &ostree_subbootdir_dfd, error))
goto out;
for (i = 0; i < new_deployments->len; i++)
{
OstreeDeployment *deployment = new_deployments->pdata[i];
- g_autofree char *bootlink_pathname = g_strdup_printf ("%s/%s/%d",
- ostree_deployment_get_osname (deployment),
- ostree_deployment_get_bootcsum (deployment),
- ostree_deployment_get_bootserial (deployment));
+ g_autofree char *bootlink_parent = g_strconcat (ostree_deployment_get_osname (deployment),
+ "/",
+ ostree_deployment_get_bootcsum (deployment),
+ NULL);
+ g_autofree char *bootlink_pathname = g_strdup_printf ("%s/%d", bootlink_parent,
ostree_deployment_get_bootserial (deployment));
g_autofree char *bootlink_target = g_strdup_printf ("../../../deploy/%s/deploy/%s.%d",
- ostree_deployment_get_osname (deployment),
- ostree_deployment_get_csum (deployment),
- ostree_deployment_get_deployserial (deployment));
- g_autoptr(GFile) linkname = g_file_get_child (ostree_subbootdir, bootlink_pathname);
- g_autoptr(GFile) linkname_parent = g_file_get_parent (linkname);
+ ostree_deployment_get_osname (deployment),
+ ostree_deployment_get_csum (deployment),
+ ostree_deployment_get_deployserial (deployment));
- if (!ot_util_ensure_directory_and_fsync (linkname_parent, cancellable, error))
+ if (!glnx_shutil_mkdir_p_at (ostree_subbootdir_dfd, bootlink_parent, 0755, cancellable, error))
goto out;
-
- if (!g_file_make_symbolic_link (linkname, bootlink_target, cancellable, error))
+
+ if (!symlink_at_replace (bootlink_target, ostree_subbootdir_dfd, bootlink_pathname,
+ cancellable, error))
goto out;
}
- if (!ot_gfile_atomic_symlink_swap (ostree_bootdir, ostree_subbootdir_name,
- cancellable, error))
+ ret = TRUE;
+ out:
+ return ret;
+}
+
+static gboolean
+swap_bootlinks (OstreeSysroot *self,
+ int bootversion,
+ GPtrArray *new_deployments,
+ GCancellable *cancellable,
+ GError **error)
+{
+ gboolean ret = FALSE;
+ int old_subbootversion, new_subbootversion;
+ glnx_fd_close int ostree_dfd = -1;
+ glnx_fd_close int ostree_subbootdir_dfd = -1;
+ g_autofree char *ostree_bootdir_name = NULL;
+ g_autofree char *ostree_subbootdir_name = NULL;
+
+ if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_dfd, error))
goto out;
+ ostree_bootdir_name = g_strdup_printf ("boot.%d", bootversion);
+
+ if (bootversion != self->bootversion)
+ {
+ if (!_ostree_sysroot_read_current_subbootversion (self, bootversion, &old_subbootversion,
+ cancellable, error))
+ goto out;
+ }
+ else
+ old_subbootversion = self->subbootversion;
+
+ new_subbootversion = old_subbootversion == 0 ? 1 : 0;
+
+ ostree_subbootdir_name = g_strdup_printf ("boot.%d.%d", bootversion, new_subbootversion);
+
+ if (!symlink_at_replace (ostree_subbootdir_name, ostree_dfd, ostree_bootdir_name,
+ cancellable, error))
+ goto out;
+
ret = TRUE;
out:
return ret;
@@ -1375,6 +1465,32 @@ install_deployment_kernel (OstreeSysroot *sysroot,
}
static gboolean
+prepare_new_bootloader_link (OstreeSysroot *sysroot,
+ int current_bootversion,
+ int new_bootversion,
+ GCancellable *cancellable,
+ GError **error)
+{
+ gboolean ret = FALSE;
+ g_autofree char *new_target = NULL;
+
+ g_assert ((current_bootversion == 0 && new_bootversion == 1) ||
+ (current_bootversion == 1 && new_bootversion == 0));
+
+ new_target = g_strdup_printf ("loader.%d", new_bootversion);
+
+ /* We shouldn't actually need to replace but it's easier to reuse
+ that code */
+ if (!symlink_at_replace (new_target, sysroot->sysroot_fd, "boot/loader.tmp",
+ cancellable, error))
+ goto out;
+
+ ret = TRUE;
+ out:
+ return ret;
+}
+
+static gboolean
swap_bootloader (OstreeSysroot *sysroot,
int current_bootversion,
int new_bootversion,
@@ -1382,19 +1498,43 @@ swap_bootloader (OstreeSysroot *sysroot,
GError **error)
{
gboolean ret = FALSE;
- g_autoptr(GFile) boot_loader_link = NULL;
- g_autofree char *new_target = NULL;
+ glnx_fd_close int boot_dfd = -1;
+ int res;
g_assert ((current_bootversion == 0 && new_bootversion == 1) ||
(current_bootversion == 1 && new_bootversion == 0));
- boot_loader_link = g_file_resolve_relative_path (sysroot->path, "boot/loader");
- new_target = g_strdup_printf ("loader.%d", new_bootversion);
-
- if (!ot_gfile_atomic_symlink_swap (boot_loader_link, new_target,
- cancellable, error))
+ if (!glnx_opendirat (sysroot->sysroot_fd, "boot", TRUE, &boot_dfd, error))
goto out;
+ /* The symlink was already written, and we used syncfs() to ensure
+ * its data is in place. Renaming now should give us atomic semantics;
+ * see https://bugzilla.gnome.org/show_bug.cgi?id=755595
+ */
+ do
+ res = renameat (boot_dfd, "loader.tmp", boot_dfd, "loader");
+ while (G_UNLIKELY (res == -1 && errno == EINTR));
+ if (res == -1)
+ {
+ glnx_set_error_from_errno (error);
+ goto out;
+ }
+
+ /* Now we explicitly fsync this directory, even though it
+ * isn't required for atomicity, for two reasons:
+ * - It should be very cheap as we're just syncing whatever
+ * data was written since the last sync which was hopefully
+ * less than a second ago.
+ * - It should be sync'd before shutdown as that could crash
+ * for whatever reason, and we wouldn't want to confuse the
+ * admin by going back to the previous session.
+ */
+ if (fsync (boot_dfd) != 0)
+ {
+ glnx_set_error_from_errno (error);
+ goto out;
+ }
+
ret = TRUE;
out:
return ret;
@@ -1565,6 +1705,14 @@ ostree_sysroot_write_deployments (OstreeSysroot *self,
if (!requires_new_bootversion)
{
+ if (!create_new_bootlinks (self, self->bootversion,
+ new_deployments,
+ cancellable, error))
+ {
+ g_prefix_error (error, "Creating new current bootlinks: ");
+ goto out;
+ }
+
if (!full_system_sync (self, cancellable, error))
{
g_prefix_error (error, "Full sync: ");
@@ -1633,11 +1781,18 @@ ostree_sysroot_write_deployments (OstreeSysroot *self,
}
}
- /* Swap bootlinks for *new* version */
+ /* Create and swap bootlinks for *new* version */
+ if (!create_new_bootlinks (self, new_bootversion,
+ new_deployments,
+ cancellable, error))
+ {
+ g_prefix_error (error, "Creating new version bootlinks: ");
+ goto out;
+ }
if (!swap_bootlinks (self, new_bootversion, new_deployments,
cancellable, error))
{
- g_prefix_error (error, "Generating new bootlinks: ");
+ g_prefix_error (error, "Swapping new version bootlinks: ");
goto out;
}
@@ -1657,6 +1812,13 @@ ostree_sysroot_write_deployments (OstreeSysroot *self,
}
}
+ if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion,
+ cancellable, error))
+ {
+ g_prefix_error (error, "Preparing final bootloader swap: ");
+ goto out;
+ }
+
if (!full_system_sync (self, cancellable, error))
{
g_prefix_error (error, "Full sync: ");
diff --git a/src/libotutil/ot-gio-utils.c b/src/libotutil/ot-gio-utils.c
index 1143ca9..b10c04c 100644
--- a/src/libotutil/ot-gio-utils.c
+++ b/src/libotutil/ot-gio-utils.c
@@ -508,59 +508,3 @@ ot_util_ensure_directory_and_fsync (GFile *dir,
(void) close (parentfd);
return ret;
}
-
-/**
- * ot_gfile_atomic_symlink_swap:
- * @path: Replace the contents of this symbolic link
- * @target: New symbolic link target
- * @cancellable:
- * @error
- *
- * Create a new temporary symbolic link, then use the Unix rename()
- * function to atomically replace @path with the new symbolic link.
- * Do not use this function inside directories such as /tmp as it uses
- * a predicatable file name.
- */
-gboolean
-ot_gfile_atomic_symlink_swap (GFile *path,
- const char *target,
- GCancellable *cancellable,
- GError **error)
-{
- gboolean ret = FALSE;
- g_autoptr(GFile) parent = g_file_get_parent (path);
- g_autofree char *tmpname = g_strconcat (gs_file_get_basename_cached (path), ".tmp", NULL);
- g_autoptr(GFile) tmppath = g_file_get_child (parent, tmpname);
- int parent_dfd = -1;
-
- if (!ot_gfile_ensure_unlinked (tmppath, cancellable, error))
- goto out;
-
- if (!g_file_make_symbolic_link (tmppath, target, cancellable, error))
- goto out;
-
- if (!gs_file_open_dir_fd (parent, &parent_dfd, cancellable, error))
- goto out;
-
- /* Ensure the link has hit disk */
- if (fsync (parent_dfd) != 0)
- {
- gs_set_error_from_errno (error, errno);
- goto out;
- }
-
- if (!gs_file_rename (tmppath, path, cancellable, error))
- goto out;
-
- /* And sync again for good measure */
- if (fsync (parent_dfd) != 0)
- {
- gs_set_error_from_errno (error, errno);
- goto out;
- }
-
- ret = TRUE;
- out:
- if (parent_dfd != -1) (void) close (parent_dfd);
- return ret;
-}
diff --git a/src/libotutil/ot-gio-utils.h b/src/libotutil/ot-gio-utils.h
index bd0ef5c..355073a 100644
--- a/src/libotutil/ot-gio-utils.h
+++ b/src/libotutil/ot-gio-utils.h
@@ -85,11 +85,6 @@ gboolean ot_gfile_ensure_unlinked (GFile *path,
GCancellable *cancellable,
GError **error);
-gboolean ot_gfile_atomic_symlink_swap (GFile *path,
- const char *target,
- GCancellable *cancellable,
- GError **error);
-
gboolean ot_util_ensure_directory_and_fsync (GFile *dir,
GCancellable *cancellable,
GError **error);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
