Merge pull request #948 from champtar/fsfreeze_thaw_cycle

Add fsfreeze_thaw_cycle() + small cleanups

Author: cgwalters

src/backend/statefile.rs

@@ -1,6 +1,7 @@
 //! On-disk saved state.
 
 use crate::model::SavedState;
+use crate::util::SignalTerminationGuard;
 use anyhow::{bail, Context, Result};
 use fn_error_context::context;
 use fs2::FileExt;
@@ -9,25 +10,6 @@ use std::fs::File;
 use std::io::prelude::*;
 use std::path::Path;
 
-/// Suppress SIGTERM while active
-// TODO: In theory we could record if we got SIGTERM and exit
-// on drop, but in practice we don't care since we're going to exit anyways.
-#[derive(Debug)]
-struct SignalTerminationGuard(signal_hook_registry::SigId);
-
-impl SignalTerminationGuard {
-    pub(crate) fn new() -> Result<Self> {
-        let signal = unsafe { signal_hook_registry::register(libc::SIGTERM, || {})? };
-        Ok(Self(signal))
-    }
-}
-
-impl Drop for SignalTerminationGuard {
-    fn drop(&mut self) {
-        signal_hook_registry::unregister(self.0);
-    }
-}
-
 impl SavedState {
     /// System-wide bootupd write lock (relative to sysroot).
     const WRITE_LOCK_PATH: &'static str = "run/bootupd-lock";

src/bootupd.rs

@@ -9,6 +9,7 @@ use crate::coreos;
     target_arch = "riscv64"
 ))]
 use crate::efi;
+use crate::freezethaw::fsfreeze_thaw_cycle;
 use crate::model::{ComponentStatus, ComponentUpdatable, ContentMetadata, SavedState, Status};
 use crate::{ostreeutil, util};
 use anyhow::{anyhow, Context, Result};
@@ -17,7 +18,6 @@ use clap::crate_version;
 use fn_error_context::context;
 use libc::mode_t;
 use libc::{S_IRGRP, S_IROTH, S_IRUSR, S_IWUSR};
-use openat_ext::OpenatDirExt;
 use serde::{Deserialize, Serialize};
 use std::borrow::Cow;
 use std::collections::BTreeMap;
@@ -625,21 +625,14 @@ pub(crate) fn client_run_migrate_static_grub_config() -> Result<()> {
 
             strip_grub_config_file(content, &dirfd, stripped_config)?;
 
-            // Sync changes to the filesystem (ignore failures)
-            let _ = dirfd.syncfs();
-
-            // Atomically exchange the configs
+            // Atomically replace the symlink
             dirfd
-                .local_exchange(stripped_config, "grub.cfg")
-                .context("Failed to exchange symlink with current GRUB config")?;
+                .local_rename(stripped_config, "grub.cfg")
+                .context("Failed to replace symlink with current GRUB config")?;
 
-            // Sync changes to the filesystem (ignore failures)
-            let _ = dirfd.syncfs();
+            fsfreeze_thaw_cycle(dirfd.open_file(".")?)?;
 
             println!("GRUB config symlink successfully replaced with the current config");
-
-            // Remove the now unused symlink (optional cleanup, ignore any failures)
-            _ = dirfd.remove_file(stripped_config);
         }
     };
 
@@ -690,8 +683,10 @@ fn strip_grub_config_file(
     }
 
     writer
-        .flush()
-        .context("Failed to write stripped GRUB config")?;
+        .into_inner()
+        .context("Failed to flush stripped GRUB config")?
+        .sync_data()
+        .context("Failed to sync stripped GRUB config")?;
 
     Ok(())
 }

src/filetree.rs

@@ -4,6 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+use crate::freezethaw::fsfreeze_thaw_cycle;
 #[cfg(any(
     target_arch = "x86_64",
     target_arch = "aarch64",
@@ -330,23 +331,6 @@ pub(crate) struct ApplyUpdateOptions {
     pub(crate) skip_sync: bool,
 }
 
-// syncfs() is a Linux-specific system call, which doesn't seem
-// to be bound in nix today.  I found https://github.com/XuShaohua/nc
-// but that's a nontrivial dependency with not a lot of code review.
-// Let's just fork off a helper process for now.
-#[cfg(any(
-    target_arch = "x86_64",
-    target_arch = "aarch64",
-    target_arch = "riscv64"
-))]
-pub(crate) fn syncfs(d: &openat::Dir) -> Result<()> {
-    use rustix::fs::{Mode, OFlags};
-    let d = unsafe { BorrowedFd::borrow_raw(d.as_raw_fd()) };
-    let oflags = OFlags::RDONLY | OFlags::CLOEXEC | OFlags::DIRECTORY;
-    let d = rustix::fs::openat(d, ".", oflags, Mode::empty())?;
-    rustix::fs::syncfs(d).map_err(Into::into)
-}
-
 /// Copy from src to dst at root dir
 #[cfg(any(
     target_arch = "x86_64",
@@ -475,7 +459,7 @@ pub(crate) fn apply_diff(
     }
     // Ensure all of the updates & changes are written persistently to disk
     if !opts.skip_sync {
-        syncfs(destdir)?;
+        destdir.syncfs()?;
     }
 
     // finally remove the temp dir
@@ -486,7 +470,7 @@ pub(crate) fn apply_diff(
     // A second full filesystem sync to narrow any races rather than
     // waiting for writeback to kick in.
     if !opts.skip_sync {
-        syncfs(destdir)?;
+        fsfreeze_thaw_cycle(destdir.open_file(".")?)?;
     }
     Ok(())
 }

src/freezethaw.rs

@@ -0,0 +1,50 @@
+use rustix::fd::AsFd;
+use rustix::ffi as c;
+use rustix::io::Errno;
+use rustix::ioctl::opcode;
+use rustix::{io, ioctl};
+
+use crate::util::SignalTerminationGuard;
+
+fn ioctl_fifreeze<Fd: AsFd>(fd: Fd) -> io::Result<()> {
+    // SAFETY: `FIFREEZE` is a no-argument opcode.
+    // `FIFREEZE` is defined as `_IOWR('X', 119, int)`.
+    unsafe {
+        let ctl = ioctl::NoArg::<{ opcode::read_write::<c::c_int>(b'X', 119) }>::new();
+        ioctl::ioctl(fd, ctl)
+    }
+}
+
+fn ioctl_fithaw<Fd: AsFd>(fd: Fd) -> io::Result<()> {
+    // SAFETY: `FITHAW` is a no-argument opcode.
+    // `FITHAW` is defined as `_IOWR('X', 120, int)`.
+    unsafe {
+        let ctl = ioctl::NoArg::<{ opcode::read_write::<c::c_int>(b'X', 120) }>::new();
+        ioctl::ioctl(fd, ctl)
+    }
+}
+
+/// syncfs() doesn't flush the journal on XFS,
+/// and since GRUB2 can't read the XFS journal, the system
+/// could fail to boot.
+///
+/// http://marc.info/?l=linux-fsdevel&m=149520244919284&w=2
+/// https://github.com/ostreedev/ostree/pull/1049
+///
+/// This function always call syncfs() first, then calls
+/// `ioctl(FIFREEZE)` and `ioctl(FITHAW)`, ignoring `EOPNOTSUPP` and `EPERM`
+pub(crate) fn fsfreeze_thaw_cycle<Fd: AsFd>(fd: Fd) -> anyhow::Result<()> {
+    rustix::fs::syncfs(&fd)?;
+
+    let _guard = SignalTerminationGuard::new()?;
+
+    let freeze = ioctl_fifreeze(&fd);
+    match freeze {
+        // Ignore permissions errors (tests)
+        Err(Errno::PERM) => Ok(()),
+        // Ignore unsupported FS
+        Err(Errno::NOTSUP) => Ok(()),
+        Ok(()) => Ok(ioctl_fithaw(fd)?),
+        _ => Ok(freeze?),
+    }
+}

src/grubconfigs.rs

@@ -7,6 +7,8 @@ use bootc_utils::CommandRunExt;
 use fn_error_context::context;
 use openat_ext::OpenatDirExt;
 
+use crate::freezethaw::fsfreeze_thaw_cycle;
+
 /// The subdirectory of /boot we use
 const GRUB2DIR: &str = "grub2";
 const CONFIGDIR: &str = "/usr/lib/bootupd/grub2-static";
@@ -60,8 +62,9 @@ pub(crate) fn install(
         println!("Added {name}");
     }
 
-    bootdir
-        .write_file_contents(format!("{GRUB2DIR}/grub.cfg"), 0o644, config.as_bytes())
+    let grub2dir = bootdir.sub_dir(GRUB2DIR)?;
+    grub2dir
+        .write_file_contents("grub.cfg", 0o644, config.as_bytes())
         .context("Copying grub-static.cfg")?;
     println!("Installed: grub.cfg");
 
@@ -74,15 +77,18 @@ pub(crate) fn install(
             .uuid
             .ok_or_else(|| anyhow::anyhow!("Failed to find UUID for boot"))?;
         let grub2_uuid_contents = format!("set BOOT_UUID=\"{bootfs_uuid}\"\n");
-        let uuid_path = format!("{GRUB2DIR}/bootuuid.cfg");
-        bootdir
-            .write_file_contents(&uuid_path, 0o644, grub2_uuid_contents)
+        let uuid_path = "bootuuid.cfg";
+        grub2dir
+            .write_file_contents(uuid_path, 0o644, grub2_uuid_contents)
             .context("Writing bootuuid.cfg")?;
+        println!("Installed: bootuuid.cfg");
         Some(uuid_path)
     } else {
         None
     };
 
+    fsfreeze_thaw_cycle(grub2dir.open_file(".")?)?;
+
     if let Some(vendordir) = installed_efi_vendor {
         log::debug!("vendordir={:?}", &vendordir);
         let vendor = PathBuf::from(vendordir);
@@ -96,13 +102,13 @@ pub(crate) fn install(
                 .context("Copying static EFI")?;
             println!("Installed: {target:?}");
             if let Some(uuid_path) = uuid_path {
-                // SAFETY: we always have a filename
-                let filename = Path::new(&uuid_path).file_name().unwrap();
-                let target = &vendor.join(filename);
-                bootdir
+                let target = &vendor.join(uuid_path);
+                grub2dir
                     .copy_file_at(uuid_path, &efidir, target)
                     .context("Writing bootuuid.cfg to efi dir")?;
+                println!("Installed: {target:?}");
             }
+            fsfreeze_thaw_cycle(efidir.open_file(".")?)?;
         }
     }
 

src/main.rs

@@ -33,6 +33,7 @@ mod efi;
 mod failpoints;
 mod filesystem;
 mod filetree;
+mod freezethaw;
 #[cfg(any(
     target_arch = "x86_64",
     target_arch = "aarch64",

src/util.rs

@@ -115,3 +115,22 @@ pub fn running_in_container() -> bool {
     }
     false
 }
+
+/// Suppress SIGTERM while active
+// TODO: In theory we could record if we got SIGTERM and exit
+// on drop, but in practice we don't care since we're going to exit anyways.
+#[derive(Debug)]
+pub(crate) struct SignalTerminationGuard(signal_hook_registry::SigId);
+
+impl SignalTerminationGuard {
+    pub(crate) fn new() -> Result<Self> {
+        let signal = unsafe { signal_hook_registry::register(libc::SIGTERM, || {})? };
+        Ok(Self(signal))
+    }
+}
+
+impl Drop for SignalTerminationGuard {
+    fn drop(&mut self) {
+        signal_hook_registry::unregister(self.0);
+    }
+}