From 72bdcc43ff9c7c68591923b84ece14581fcc9854 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Mon, 16 Jun 2025 18:23:01 +0200 Subject: [PATCH 1/4] 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 f60bc8e2..c7ed6f29 100644 --- a/crates/composefs/src/repository.rs +++ b/crates/composefs/src/repository.rs @@ -25,7 +25,7 @@ use crate::{ }, mount::mount_composefs_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. @@ -485,15 +485,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 e30bba652553e8703fa2479b1c685a914aac011d Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Wed, 18 Jun 2025 17:21:36 +0200 Subject: [PATCH 2/4] 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 c7ed6f29..bb112a56 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}, }; @@ -25,7 +25,7 @@ use crate::{ }, mount::mount_composefs_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. @@ -261,7 +261,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}"); @@ -310,7 +310,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 } }; @@ -366,7 +366,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}"); @@ -432,14 +432,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 fff41b18f064dafb7484b12bb54c4756d0dfb906 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Mon, 16 Jun 2025 18:21:31 +0200 Subject: [PATCH 3/4] 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 bb112a56..53d89b47 100644 --- a/crates/composefs/src/repository.rs +++ b/crates/composefs/src/repository.rs @@ -331,9 +331,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) @@ -384,7 +386,9 @@ impl Repository { } pub fn open_image(&self, name: &str) -> Result { - 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("/") { // A name with no slashes in it is taken to be a sha256 fs-verity digest From adf4a545f42713d549c593c8ebf9d8003ae1ba49 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 24 Jun 2025 12:54:59 +0200 Subject: [PATCH 4/4] utils: Make filter_errno a method on the result instead of a function I.e. instead of `filter(open(), ENOENT)`, you do `open().filter(ENOENT)`. --- crates/composefs/src/repository.rs | 25 +++++++++++------------- crates/composefs/src/util.rs | 31 +++++++++++++++++++----------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/crates/composefs/src/repository.rs b/crates/composefs/src/repository.rs index 53d89b47..5d19f6b4 100644 --- a/crates/composefs/src/repository.rs +++ b/crates/composefs/src/repository.rs @@ -25,7 +25,7 @@ use crate::{ }, mount::mount_composefs_at, splitstream::{DigestMap, SplitStreamReader, SplitStreamWriter}, - util::{filter_errno, proc_self_fd, replace_symlinkat, Sha256Digest}, + util::{proc_self_fd, replace_symlinkat, ErrnoFilter, Sha256Digest}, }; /// Call openat() on the named subdirectory of "dirfd", possibly creating it first. @@ -483,24 +483,21 @@ impl Repository { fn gc_category(&self, category: &str) -> Result> { let mut objects = HashSet::new(); - let Some(category_fd) = filter_errno( - self.openat(category, OFlags::RDONLY | OFlags::DIRECTORY), - Errno::NOENT, - ) - .context("Opening {category} dir in repository")? + let Some(category_fd) = self + .openat(category, OFlags::RDONLY | OFlags::DIRECTORY) + .filter_errno(Errno::NOENT) + .context("Opening {category} dir in repository")? else { return Ok(objects); }; - if let Some(refs) = filter_errno( - openat( - &category_fd, - "refs", - OFlags::RDONLY | OFlags::DIRECTORY, - Mode::empty(), - ), - Errno::NOENT, + if let Some(refs) = openat( + &category_fd, + "refs", + OFlags::RDONLY | OFlags::DIRECTORY, + Mode::empty(), ) + .filter_errno(Errno::NOENT) .context("Opening {category}/refs dir in repository")? { Self::walk_symlinkdir(refs, &mut objects)?; diff --git a/crates/composefs/src/util.rs b/crates/composefs/src/util.rs index 30c74f45..75dbca5f 100644 --- a/crates/composefs/src/util.rs +++ b/crates/composefs/src/util.rs @@ -106,14 +106,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), +pub(crate) trait ErrnoFilter { + fn filter_errno(self, ignored: Errno) -> ErrnoResult>; +} + +impl ErrnoFilter for ErrnoResult { + fn filter_errno(self, ignored: Errno) -> ErrnoResult> { + match self { + Ok(result) => Ok(Some(result)), + Err(err) if err == ignored => Ok(None), + Err(err) => Err(err), + } } } @@ -135,12 +138,15 @@ pub(crate) fn replace_symlinkat( let target = target.as_ref(); // Step 1: try to create the symlink - if filter_errno(symlinkat(target, dirfd, name), Errno::EXIST)?.is_some() { + if symlinkat(target, dirfd, name) + .filter_errno(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 let Some(current_target) = readlinkat(dirfd, name, []).filter_errno(Errno::NOENT)? { if current_target.into_bytes() == target.as_os_str().as_bytes() { return Ok(()); } @@ -149,7 +155,10 @@ pub(crate) fn replace_symlinkat( // 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() { + if symlinkat(target, dirfd, &tmp_name) + .filter_errno(Errno::EXIST)? + .is_none() + { // This temporary filename already exists, try another continue; }