From c4651ca1cc288b833bf4781bddbca965c054ab92 Mon Sep 17 00:00:00 2001 From: Robert Collins Date: Thu, 21 Nov 2024 14:17:42 +0100 Subject: [PATCH 1/2] Shift parallel feature from a flag only to an optional runtime behaviour - expose via the CLI - add a new Builder and Remover struct to hold any other future things we might make configurable. --- src/_impl.rs | 199 ++++++++++++++++++++++---------------- src/_impl/io.rs | 1 + src/bin/remove-dir-all.rs | 21 +++- src/lib.rs | 72 +++++++++++++- 4 files changed, 204 insertions(+), 89 deletions(-) diff --git a/src/_impl.rs b/src/_impl.rs index 0a55754..c31a281 100644 --- a/src/_impl.rs +++ b/src/_impl.rs @@ -25,6 +25,13 @@ cfg_if::cfg_if! { } } +pub(crate) fn default_parallel_mode() -> ParallelMode { + #[cfg(feature = "parallel")] + return ParallelMode::Parallel; + #[cfg(not(feature = "parallel"))] + return ParallelMode::Serial; +} + impl super::RemoveDir for std::fs::File { fn remove_dir_contents(&mut self, debug_root: Option<&Path>) -> Result<()> { // thunk over to the free version adding in the os-specific IO trait impl @@ -62,17 +69,20 @@ pub(crate) fn _remove_dir_contents_path>(path: P) -> R /// exterior lifetime interface to dir removal fn _remove_dir_contents(d: &mut File, debug_root: &PathComponents<'_>) -> Result<()> { let owned_handle = I::duplicate_fd(d)?; - remove_dir_contents_recursive::(owned_handle, debug_root)?; + remove_dir_contents_recursive::(owned_handle, debug_root, default_parallel_mode())?; Ok(()) } /// deprecated interface -pub(crate) fn remove_dir_all_path>(path: P) -> Result<()> { +pub(crate) fn remove_dir_all_path>( + path: P, + parallel: ParallelMode, +) -> Result<()> { let p = path.as_ref(); // Opportunity 1 for races let d = I::open_dir(p)?; let debug_root = PathComponents::Path(if p.has_root() { p } else { Path::new(".") }); - remove_dir_contents_recursive::(d, &debug_root)?; + remove_dir_contents_recursive::(d, &debug_root, parallel)?; // Opportunity 2 for races std::fs::remove_dir(&path)?; #[cfg(feature = "log")] @@ -80,111 +90,128 @@ pub(crate) fn remove_dir_all_path>(path: P) -> Result< Ok(()) } -use crate::RemoveDir; +use crate::{ParallelMode, RemoveDir}; use self::path_components::PathComponents; // Core workhorse, heading towards this being able to be tasks. -#[allow(clippy::map_identity)] fn remove_dir_contents_recursive( mut d: File, debug_root: &PathComponents<'_>, + parallel: ParallelMode, ) -> Result { #[cfg(feature = "log")] log::trace!("scanning {}", &debug_root); - // We take a os level clone of the FD so that there are no lifetime - // concerns. It would *not* be ok to do readdir on one file twice - // concurrently because of shared kernel state. + // We take a os level clone of the FD so that there are no rust-level + // lifetime concerns. It would *not* be ok to do readdir on one file twice + // (even via the cloned FD) concurrently because of shared kernel state: the + // readdir state is stored per file, not per FD. let dirfd = I::duplicate_fd(&mut d)?; - cfg_if::cfg_if! { - if #[cfg(feature = "parallel")] { + match parallel { + ParallelMode::Serial => { + let mut iter = fs_at::read_dir(&mut d)?; + iter.try_for_each(|dir_entry| { + scan_and_remove_entry_recursively::(debug_root, &dirfd, dir_entry, parallel) + })?; + } + #[cfg(feature = "parallel")] + _ => { let iter = fs_at::read_dir(&mut d)?; let iter = iter.par_bridge(); - } else { - let mut iter = fs_at::read_dir(&mut d)?; + iter.try_for_each(|dir_entry| { + scan_and_remove_entry_recursively::(debug_root, &dirfd, dir_entry, parallel) + })?; } - } + }; - iter.try_for_each(|dir_entry| -> Result<()> { - let dir_entry = dir_entry?; - let name = dir_entry.name(); - if name == OsStr::new(".") || name == OsStr::new("..") { - return Ok(()); + #[cfg(feature = "log")] + log::trace!("scanned {}", &debug_root); + Ok(dirfd) +} + +#[allow(clippy::map_identity)] +fn scan_and_remove_entry_recursively( + debug_root: &PathComponents<'_>, + dirfd: &File, + dir_entry: Result, + parallel: ParallelMode, +) -> Result<()> { + let dir_entry = dir_entry?; + let name = dir_entry.name(); + if name == OsStr::new(".") || name == OsStr::new("..") { + return Ok(()); + } + let dir_path = Path::new(name); + let dir_debug_root = PathComponents::Component(debug_root, dir_path); + #[cfg(windows)] + { + // On windows: open the file and then decide what to do with it. + let mut opts = fs_at::OpenOptions::default(); + // Could possibly drop a syscall by dropping FILE_READ_ATTRIBUTES + // and trusting read_dir metadata more. OTOH that would introduce a + // race :/. + opts.desired_access(DELETE | FILE_LIST_DIRECTORY | FILE_READ_ATTRIBUTES); + let mut child_file = opts.open_path_at(dirfd, name)?; + let metadata = child_file.metadata()?; + let is_dir = metadata.is_dir(); + let is_symlink = metadata.is_symlink(); + if is_dir && !is_symlink { + remove_dir_contents_recursive::( + I::duplicate_fd(&mut child_file)?, + &dir_debug_root, + parallel, + )?; } - let dir_path = Path::new(name); - let dir_debug_root = PathComponents::Component(debug_root, dir_path); - #[cfg(windows)] - { - // On windows: open the file and then decide what to do with it. - let mut opts = fs_at::OpenOptions::default(); - // Could possibly drop a syscall by dropping FILE_READ_ATTRIBUTES - // and trusting read_dir metadata more. OTOH that would introduce a - // race :/. - opts.desired_access(DELETE | FILE_LIST_DIRECTORY | FILE_READ_ATTRIBUTES); - let mut child_file = opts.open_path_at(&dirfd, name)?; - let metadata = child_file.metadata()?; - let is_dir = metadata.is_dir(); - let is_symlink = metadata.is_symlink(); - if is_dir && !is_symlink { - remove_dir_contents_recursive::( - I::duplicate_fd(&mut child_file)?, - &dir_debug_root, - )?; + #[cfg(feature = "log")] + log::trace!("delete: {}", &dir_debug_root); + child_file.delete_by_handle().map_err(|(_f, e)| { + #[cfg(feature = "log")] + log::debug!("error removing {}", dir_debug_root); + e + })?; + } + #[cfg(not(windows))] + { + // Otherwise, open the path safely but normally, fstat to see if its + // a dir, then either unlink or recursively delete + let mut opts = fs_at::OpenOptions::default(); + opts.read(true) + .write(fs_at::OpenOptionsWriteMode::Write) + .follow(false); + let child_result = opts.open_dir_at(dirfd, name); + let is_dir = match child_result { + // We expect is_eloop to be the only error + Err(e) if !I::is_eloop(&e) => return Err(e), + Err(_) => false, + Ok(child_file) => { + let metadata = child_file.metadata()?; + let is_dir = metadata.is_dir(); + if is_dir { + remove_dir_contents_recursive::(child_file, &dir_debug_root, parallel)?; + #[cfg(feature = "log")] + log::trace!("rmdir: {}", &dir_debug_root); + let opts = fs_at::OpenOptions::default(); + opts.rmdir_at(dirfd, name).map_err(|e| { + #[cfg(feature = "log")] + log::debug!("error removing {}", dir_debug_root); + e + })?; + } + is_dir } + }; + if !is_dir { #[cfg(feature = "log")] - log::trace!("delete: {}", &dir_debug_root); - child_file.delete_by_handle().map_err(|(_f, e)| { + log::trace!("unlink: {}", &dir_debug_root); + opts.unlink_at(dirfd, name).map_err(|e| { #[cfg(feature = "log")] log::debug!("error removing {}", dir_debug_root); e })?; } - #[cfg(not(windows))] - { - // Otherwise, open the path safely but normally, fstat to see if its - // a dir, then either unlink or recursively delete - let mut opts = fs_at::OpenOptions::default(); - opts.read(true) - .write(fs_at::OpenOptionsWriteMode::Write) - .follow(false); - let child_result = opts.open_dir_at(&dirfd, name); - let is_dir = match child_result { - // We expect is_eloop to be the only error - Err(e) if !I::is_eloop(&e) => return Err(e), - Err(_) => false, - Ok(child_file) => { - let metadata = child_file.metadata()?; - let is_dir = metadata.is_dir(); - if is_dir { - remove_dir_contents_recursive::(child_file, &dir_debug_root)?; - #[cfg(feature = "log")] - log::trace!("rmdir: {}", &dir_debug_root); - let opts = fs_at::OpenOptions::default(); - opts.rmdir_at(&dirfd, name).map_err(|e| { - #[cfg(feature = "log")] - log::debug!("error removing {}", dir_debug_root); - e - })?; - } - is_dir - } - }; - if !is_dir { - #[cfg(feature = "log")] - log::trace!("unlink: {}", &dir_debug_root); - opts.unlink_at(&dirfd, name).map_err(|e| { - #[cfg(feature = "log")] - log::debug!("error removing {}", dir_debug_root); - e - })?; - } - } - #[cfg(feature = "log")] - log::trace!("removed {}", dir_debug_root); - - Ok(()) - })?; + } #[cfg(feature = "log")] - log::trace!("scanned {}", &debug_root); - Ok(dirfd) + log::trace!("removed {}", dir_debug_root); + + Ok(()) } diff --git a/src/_impl/io.rs b/src/_impl/io.rs index 73ccac4..a17f55c 100644 --- a/src/_impl/io.rs +++ b/src/_impl/io.rs @@ -13,6 +13,7 @@ pub(crate) trait Io { fn open_dir(p: &Path) -> io::Result; #[cfg(not(windows))] + #[allow(dead_code)] fn unique_identifier(d: &File) -> io::Result; #[cfg(not(windows))] diff --git a/src/bin/remove-dir-all.rs b/src/bin/remove-dir-all.rs index 3b21083..0cbfdf2 100644 --- a/src/bin/remove-dir-all.rs +++ b/src/bin/remove-dir-all.rs @@ -1,6 +1,15 @@ use std::{io::Result, path::PathBuf}; -use clap::Parser; +use clap::{Parser, ValueEnum}; + +/// What kind of parallelism to use +#[derive(Debug, Clone, Copy, ValueEnum)] +enum Parallelism { + /// No operations in parallel + Serial, + /// Parallelise readdir and unlink operations + Parallel, +} /// Simple CLI to use remove-dir-alls recursive deletion logic from the command /// line. @@ -10,14 +19,22 @@ struct Cli { /// Paths to delete #[arg(value_name = "FILE")] names: Vec, + /// Choose the parallelism strategy + #[arg(short = 'p', long = "parallelism", default_value = "parallel")] + parallelism: Parallelism, } fn main() -> Result<()> { env_logger::init(); let cli = Cli::parse(); + let remover = match cli.parallelism { + Parallelism::Serial => remove_dir_all::RemoverBuilder::new().serial().build(), + Parallelism::Parallel => remove_dir_all::RemoverBuilder::new().parallel().build(), + }; + for p in cli.names { - remove_dir_all::remove_dir_all(p)?; + remover.remove_dir_all(&p)?; } Ok(()) } diff --git a/src/lib.rs b/src/lib.rs index f4c8bf0..914e2dc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -196,7 +196,77 @@ pub fn remove_dir_contents>(path: P) -> Result<()> { /// /etc). Consider using [`RemoveDir::remove_dir_contents`] instead. pub fn remove_dir_all>(path: P) -> Result<()> { let path = path.as_ref().normalize()?; - _impl::remove_dir_all_path::<_impl::OsIo, _>(path) + _impl::remove_dir_all_path::<_impl::OsIo, _>(path, _impl::default_parallel_mode()) +} + +/// How to parallelise remove_dir_all(). +#[derive(Debug, Clone, Copy)] +enum ParallelMode { + /// No parallelism. + Serial, + /// Parallelise readdir and unlink operations - the default when the parallel feature is enabled. + #[cfg(feature = "parallel")] + Parallel, +} + +/// Builder for configuring the parallelism of remove_dir_all. +#[derive(Debug, Clone, Copy)] +#[non_exhaustive] +pub struct RemoverBuilder { + parallel: ParallelMode, +} + +impl RemoverBuilder { + /// Create a new RemoverBuilder. + pub fn new() -> Self { + Self { + #[cfg(feature = "parallel")] + parallel: ParallelMode::Parallel, + #[cfg(not(feature = "parallel"))] + parallel: ParallelMode::Serial, + } + } + + /// Serialise all IO operations. + pub fn serial(mut self) -> Self { + self.parallel = ParallelMode::Serial; + self + } + + /// Parallelise the removal of directories. + #[cfg(feature = "parallel")] + pub fn parallel(mut self) -> Self { + self.parallel = ParallelMode::Parallel; + self + } + + /// Build the Remover. + pub fn build(self) -> Remover { + Remover { + parallel: self.parallel, + } + } +} + +impl Default for RemoverBuilder { + fn default() -> Self { + Self::new() + } +} + +/// Remover holds configuration for different ways of removing directories. +#[derive(Debug, Clone, Copy)] +#[non_exhaustive] +pub struct Remover { + parallel: ParallelMode, +} + +impl Remover { + /// Remove the directory and all of its children. + pub fn remove_dir_all>(&self, path: P) -> Result<()> { + let path = path.as_ref().normalize()?; + _impl::remove_dir_all_path::<_impl::OsIo, _>(path, self.parallel) + } } #[allow(deprecated)] From 92a0f1a6f70e213ceced9ef2804422e9b644793d Mon Sep 17 00:00:00 2001 From: Robert Collins Date: Fri, 22 Nov 2024 12:30:28 +0100 Subject: [PATCH 2/2] Macos: disable parallel deletion by default Most MacOS filesystems will be APFS these days, which appears to have a global lock around readdir that leads to thrashing when performing readdir concurrently from different threads. https://gregoryszorc.com/blog/2018/10/29/global-kernel-locks-in-apfs/ --- src/_impl.rs | 4 +-- src/bin/remove-dir-all.rs | 9 +++--- src/lib.rs | 59 ++++++++++++++++++++------------------- 3 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/_impl.rs b/src/_impl.rs index c31a281..354c278 100644 --- a/src/_impl.rs +++ b/src/_impl.rs @@ -26,9 +26,9 @@ cfg_if::cfg_if! { } pub(crate) fn default_parallel_mode() -> ParallelMode { - #[cfg(feature = "parallel")] + #[cfg(all(feature = "parallel", not(target_os = "macos")))] return ParallelMode::Parallel; - #[cfg(not(feature = "parallel"))] + #[cfg(any(not(feature = "parallel"), target_os = "macos"))] return ParallelMode::Serial; } diff --git a/src/bin/remove-dir-all.rs b/src/bin/remove-dir-all.rs index 0cbfdf2..5db9ec2 100644 --- a/src/bin/remove-dir-all.rs +++ b/src/bin/remove-dir-all.rs @@ -20,8 +20,8 @@ struct Cli { #[arg(value_name = "FILE")] names: Vec, /// Choose the parallelism strategy - #[arg(short = 'p', long = "parallelism", default_value = "parallel")] - parallelism: Parallelism, + #[arg(short = 'p', long = "parallelism")] + parallelism: Option, } fn main() -> Result<()> { @@ -29,8 +29,9 @@ fn main() -> Result<()> { let cli = Cli::parse(); let remover = match cli.parallelism { - Parallelism::Serial => remove_dir_all::RemoverBuilder::new().serial().build(), - Parallelism::Parallel => remove_dir_all::RemoverBuilder::new().parallel().build(), + None => remove_dir_all::RemoverBuilder::new().build(), + Some(Parallelism::Serial) => remove_dir_all::RemoverBuilder::new().serial().build(), + Some(Parallelism::Parallel) => remove_dir_all::RemoverBuilder::new().parallel().build(), }; for p in cli.names { diff --git a/src/lib.rs b/src/lib.rs index 914e2dc..b8e04d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,7 +5,12 @@ //! following ways: //! - the `parallel` feature parallelises the deletion. This is useful when high //! syscall latency is occurring, such as on Windows (deletion IO accrues to -//! the process), or network file systems of any kind. This is off by default. +//! the process), or network file systems of any kind. This feature is off by +//! default. When enabled, it will disable itself on MacOS because of the bug +//! reported in this [blog +//! post](https://gregoryszorc.com/blog/2018/10/29/global-kernel-locks-in-apfs/). +//! Use [`RemoverBuilder`] to override this behaviour and force enable/disable +//! parallelism at runtime. //! - It tolerates files not being deleted atomically (this is a Windows //! specific behaviour). //! - It resets the readonly flag on Windows as needed. @@ -17,20 +22,22 @@ //! affect the filesystem outside of the directory tree being deleted. //! //! The extension trait [`RemoveDir`] can be used to invoke `remove_dir_all` on -//! an open [`File`](std::fs::File), where it will error if the file is not a directory, -//! and otherwise delete the contents. This allows callers to be more confident that -//! what is deleted is what was requested even in the presence of malicious -//! actors changing the filesystem concurrently. +//! an open [`File`](std::fs::File), where it will error if the file is not a +//! directory, and otherwise delete the contents. This allows callers to be more +//! confident that what is deleted is what was requested even in the presence of +//! malicious actors changing the filesystem concurrently. //! -//! The functions [`remove_dir_all`], [`remove_dir_contents`], and [`ensure_empty_dir`] -//! are intrinsically sensitive to file system races, as the path to the -//! directory to delete can be substituted by an attacker inserting a symlink -//! along that path. Relative paths with one path component are the least -//! fragile, but using [`RemoveDir::remove_dir_contents`] is recommended. +//! The functions [`remove_dir_all`], [`remove_dir_contents`], and +//! [`ensure_empty_dir`] are intrinsically sensitive to file system races, as +//! the path to the directory to delete can be substituted by an attacker +//! inserting a symlink along that path. Relative paths with one path component +//! are the least fragile, but using [`RemoveDir::remove_dir_contents`] is +//! recommended. //! //! ## Features //! -//! - parallel: When enabled, deletion of directories is parallised. (#parallel)[more details] +//! - parallel: When enabled, deletion of directories is parallised. +//! (#parallel)[more details] //! - log: Include some log messages about the deletion taking place. //! //! About the implementation. The implementation prioritises security, then @@ -65,12 +72,12 @@ //! trust-but-verify of the node type metadata returned from directory scanning: //! only names that appear to be directories get their contents scanned. The //! consequence is that if an attacker replaces a non-directory with a -//! directory, or vice versa, an error will occur - but the `remove_dir_all` will -//! not escape from the directory tree. On Windows file deletion requires +//! directory, or vice versa, an error will occur - but the `remove_dir_all` +//! will not escape from the directory tree. On Windows file deletion requires //! obtaining a handle to the file, but again the kind metadata from the //! directory scan is used to avoid re-querying the metadata. Symlinks are -//! detected by a failure to open a path with `O_NOFOLLOW`, they are unlinked with -//! no further processing. +//! detected by a failure to open a path with `O_NOFOLLOW`, they are unlinked +//! with no further processing. //! //! ## Serial deletion //! @@ -102,15 +109,14 @@ //! remove_dir_all = {version = "0.8"} //! ``` //! ## Future Plans -//! Open directory handles are kept in -//! a lg-spaced cache after the first 10 levels: -//! level10/skipped1/level12/skipped2/skipped3/skipped4/level16. If EMFILE is -//! encountered, no more handles are cached, and directories are opened by -//! re-traversing from the closest previously opened handle. Deletion should -//! succeed even only 4 file descriptors are available: one to hold the root, -//! two to iterate individual directories, and one to open-and-delete individual -//! files, though that will be quadratic in the depth of the tree, successfully -//! deleting leaves only on each iteration. +//! Open directory handles are kept in a lg-spaced cache after the first 10 +//! levels: level10/skipped1/level12/skipped2/skipped3/skipped4/level16. If +//! EMFILE is encountered, no more handles are cached, and directories are +//! opened by re-traversing from the closest previously opened handle. Deletion +//! should succeed even only 4 file descriptors are available: one to hold the +//! root, two to iterate individual directories, and one to open-and-delete +//! individual files, though that will be quadratic in the depth of the tree, +//! successfully deleting leaves only on each iteration. //! //! IO Prioritisation: //! 1) directory scanning when few paths are queued for deletion (to avoid @@ -220,10 +226,7 @@ impl RemoverBuilder { /// Create a new RemoverBuilder. pub fn new() -> Self { Self { - #[cfg(feature = "parallel")] - parallel: ParallelMode::Parallel, - #[cfg(not(feature = "parallel"))] - parallel: ParallelMode::Serial, + parallel: _impl::default_parallel_mode(), } }