From b0edaf5f63b017bdda8af12b59e344ff1b375a84 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Mon, 16 Jun 2025 18:23:01 +0200 Subject: [PATCH 1/3] Repository: Handle non-existing directories in garbage collection For example, if there has been no streams created, we don't want to fail with some random ENOENT. Signed-off-by: Alexander Larsson --- crates/composefs/src/repository.rs | 31 +++++++++++++++++++++--------- crates/composefs/src/util.rs | 12 ++++++++++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/crates/composefs/src/repository.rs b/crates/composefs/src/repository.rs index 7898d79d..8fc17a96 100644 --- a/crates/composefs/src/repository.rs +++ b/crates/composefs/src/repository.rs @@ -26,7 +26,7 @@ use crate::{ }, mount::{composefs_fsmount, mount_at}, splitstream::{DigestMap, SplitStreamReader, SplitStreamWriter}, - util::{proc_self_fd, Sha256Digest}, + util::{filter_errno, proc_self_fd, Sha256Digest}, }; /// Call openat() on the named subdirectory of "dirfd", possibly creating it first. @@ -552,15 +552,28 @@ impl Repository { fn gc_category(&self, category: &str) -> Result> { let mut objects = HashSet::new(); - let category_fd = self.openat(category, OFlags::RDONLY | OFlags::DIRECTORY)?; + let Some(category_fd) = filter_errno( + self.openat(category, OFlags::RDONLY | OFlags::DIRECTORY), + Errno::NOENT, + ) + .context("Opening {category} dir in repository")? + else { + return Ok(objects); + }; - let refs = openat( - &category_fd, - "refs", - OFlags::RDONLY | OFlags::DIRECTORY, - Mode::empty(), - )?; - Self::walk_symlinkdir(refs, &mut objects)?; + if let Some(refs) = filter_errno( + openat( + &category_fd, + "refs", + OFlags::RDONLY | OFlags::DIRECTORY, + Mode::empty(), + ), + Errno::NOENT, + ) + .context("Opening {category}/refs dir in repository")? + { + Self::walk_symlinkdir(refs, &mut objects)?; + } for item in Dir::read_from(&category_fd)? { let entry = item?; diff --git a/crates/composefs/src/util.rs b/crates/composefs/src/util.rs index d3c2bee6..9a3bba11 100644 --- a/crates/composefs/src/util.rs +++ b/crates/composefs/src/util.rs @@ -3,6 +3,7 @@ use std::{ os::fd::{AsFd, AsRawFd}, }; +use rustix::io::{Errno, Result as ErrnoResult}; use tokio::io::{AsyncRead, AsyncReadExt}; /// Formats a string like "/proc/self/fd/3" for the given fd. This can be used to work with kernel @@ -97,6 +98,17 @@ pub fn parse_sha256(string: impl AsRef) -> Result { Ok(value) } +pub(crate) fn filter_errno( + result: rustix::io::Result, + ignored: Errno, +) -> ErrnoResult> { + match result { + Ok(result) => Ok(Some(result)), + Err(err) if err == ignored => Ok(None), + Err(err) => Err(err), + } +} + #[cfg(test)] mod test { use similar_asserts::assert_eq; From 85587ed69b4b5147c884a7b6f0b5c4b67b9fe29a Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Wed, 18 Jun 2025 17:21:36 +0200 Subject: [PATCH 2/3] repository: Atomically replace refs/ symlinks Naming a new version of an image the same as the old version is pretty standard, and this is currently giving EEXIST errors. We fix this by doing an atomic symlink + rename operation to replace the symlink if it doesn't already have the expected value. Drop our existing Repository::ensure_symlink() function and use the new function for that as well. Co-authored-by: Alexander Larsson Signed-off-by: Alexander Larsson Signed-off-by: Allison Karlitskaya --- crates/composefs/Cargo.toml | 1 + crates/composefs/src/repository.rs | 22 ++++------- crates/composefs/src/util.rs | 61 +++++++++++++++++++++++++++++- 3 files changed, 68 insertions(+), 16 deletions(-) diff --git a/crates/composefs/Cargo.toml b/crates/composefs/Cargo.toml index 8bd5c085..61be2192 100644 --- a/crates/composefs/Cargo.toml +++ b/crates/composefs/Cargo.toml @@ -29,6 +29,7 @@ tempfile = { version = "3.8.0", optional = true, default-features = false } xxhash-rust = { version = "0.8.2", default-features = false, features = ["xxh32"] } zerocopy = { version = "0.8.0", default-features = false, features = ["derive", "std"] } zstd = { version = "0.13.0", default-features = false } +rand = { version = "0.9.1", default-features = true } [dev-dependencies] insta = "1.42.2" diff --git a/crates/composefs/src/repository.rs b/crates/composefs/src/repository.rs index 8fc17a96..b0169897 100644 --- a/crates/composefs/src/repository.rs +++ b/crates/composefs/src/repository.rs @@ -12,8 +12,8 @@ use anyhow::{bail, ensure, Context, Result}; use once_cell::sync::OnceCell; use rustix::{ fs::{ - fdatasync, flock, linkat, mkdirat, open, openat, readlinkat, symlinkat, AtFlags, Dir, - FileType, FlockOperation, Mode, OFlags, CWD, + fdatasync, flock, linkat, mkdirat, open, openat, readlinkat, AtFlags, Dir, FileType, + FlockOperation, Mode, OFlags, CWD, }, io::{Errno, Result as ErrnoResult}, }; @@ -26,7 +26,7 @@ use crate::{ }, mount::{composefs_fsmount, mount_at}, splitstream::{DigestMap, SplitStreamReader, SplitStreamWriter}, - util::{filter_errno, proc_self_fd, Sha256Digest}, + util::{filter_errno, proc_self_fd, replace_symlinkat, Sha256Digest}, }; /// Call openat() on the named subdirectory of "dirfd", possibly creating it first. @@ -309,7 +309,7 @@ impl Repository { let stream_path = format!("streams/{}", hex::encode(sha256)); let object_id = writer.done()?; let object_path = Self::format_object_path(&object_id); - self.ensure_symlink(&stream_path, &object_path)?; + self.symlink(&stream_path, &object_path)?; if let Some(name) = reference { let reference_path = format!("streams/refs/{name}"); @@ -358,7 +358,7 @@ impl Repository { let object_id = writer.done()?; let object_path = Self::format_object_path(&object_id); - self.ensure_symlink(&stream_path, &object_path)?; + self.symlink(&stream_path, &object_path)?; object_id } }; @@ -414,7 +414,7 @@ impl Repository { let object_path = Self::format_object_path(&object_id); let image_path = format!("images/{}", object_id.to_hex()); - self.ensure_symlink(&image_path, &object_path)?; + self.symlink(&image_path, &object_path)?; if let Some(reference) = name { let ref_path = format!("images/refs/{reference}"); @@ -499,14 +499,8 @@ impl Repository { relative.push(target_component); } - symlinkat(relative, &self.repository, name) - } - - pub fn ensure_symlink>(&self, name: P, target: &str) -> ErrnoResult<()> { - self.symlink(name, target).or_else(|e| match e { - Errno::EXIST => Ok(()), - _ => Err(e), - }) + // Atomically replace existing symlink + replace_symlinkat(&relative, &self.repository, name) } fn read_symlink_hashvalue(dirfd: &OwnedFd, name: &CStr) -> Result { diff --git a/crates/composefs/src/util.rs b/crates/composefs/src/util.rs index 9a3bba11..30c74f45 100644 --- a/crates/composefs/src/util.rs +++ b/crates/composefs/src/util.rs @@ -1,9 +1,17 @@ +use rand::{distr::Alphanumeric, Rng}; use std::{ io::{Error, ErrorKind, Read, Result}, - os::fd::{AsFd, AsRawFd}, + os::{ + fd::{AsFd, AsRawFd, OwnedFd}, + unix::ffi::OsStrExt, + }, + path::Path, }; -use rustix::io::{Errno, Result as ErrnoResult}; +use rustix::{ + fs::{readlinkat, renameat, symlinkat, unlinkat, AtFlags}, + io::{Errno, Result as ErrnoResult}, +}; use tokio::io::{AsyncRead, AsyncReadExt}; /// Formats a string like "/proc/self/fd/3" for the given fd. This can be used to work with kernel @@ -109,6 +117,55 @@ pub(crate) fn filter_errno( } } +fn generate_tmpname(prefix: &str) -> String { + let rand_string: String = rand::rng() + .sample_iter(&Alphanumeric) + .take(12) + .map(char::from) + .collect(); + format!("{}{}", prefix, rand_string) +} + +pub(crate) fn replace_symlinkat( + target: impl AsRef, + dirfd: &OwnedFd, + name: impl AsRef, +) -> ErrnoResult<()> { + let name = name.as_ref(); + let target = target.as_ref(); + + // Step 1: try to create the symlink + if filter_errno(symlinkat(target, dirfd, name), Errno::EXIST)?.is_some() { + return Ok(()); + }; + + // Step 2: the symlink already exists. Maybe it already has the correct target? + if let Some(current_target) = filter_errno(readlinkat(dirfd, name, []), Errno::NOENT)? { + if current_target.into_bytes() == target.as_os_str().as_bytes() { + return Ok(()); + } + } + + // Step 3: full atomic replace path + for _ in 0..16 { + let tmp_name = generate_tmpname(".symlink-"); + if filter_errno(symlinkat(target, dirfd, &tmp_name), Errno::EXIST)?.is_none() { + // This temporary filename already exists, try another + continue; + } + + match renameat(dirfd, &tmp_name, dirfd, name) { + Ok(_) => return Ok(()), + Err(e) => { + let _ = unlinkat(dirfd, tmp_name, AtFlags::empty()); + return Err(e); + } + } + } + + Err(Errno::EXIST) +} + #[cfg(test)] mod test { use similar_asserts::assert_eq; From 968b8b3d3d70ebe28c50721057154cbc423f6708 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Mon, 16 Jun 2025 18:21:31 +0200 Subject: [PATCH 3/3] repository: Add some error context This adds error context when opening images and streams. Without this it is kinda hard to figure out what ENOENT really means. Signed-off-by: Alexander Larsson --- crates/composefs/src/repository.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/composefs/src/repository.rs b/crates/composefs/src/repository.rs index b0169897..f11a4904 100644 --- a/crates/composefs/src/repository.rs +++ b/crates/composefs/src/repository.rs @@ -379,9 +379,11 @@ impl Repository { let filename = format!("streams/{name}"); let file = File::from(if let Some(verity_hash) = verity { - self.open_with_verity(&filename, verity_hash)? + self.open_with_verity(&filename, verity_hash) + .with_context(|| format!("Opening ref 'streams/{name}'"))? } else { - self.openat(&filename, OFlags::RDONLY)? + self.openat(&filename, OFlags::RDONLY) + .with_context(|| format!("Opening ref 'streams/{name}'"))? }); SplitStreamReader::new(file) @@ -434,7 +436,9 @@ impl Repository { /// Returns the fd of the image and whether or not verity should be /// enabled when mounting it. fn open_image(&self, name: &str) -> Result<(OwnedFd, bool)> { - let image = self.openat(&format!("images/{name}"), OFlags::RDONLY)?; + let image = self + .openat(&format!("images/{name}"), OFlags::RDONLY) + .with_context(|| format!("Opening ref 'images/{name}'"))?; if name.contains("/") { return Ok((image, true));