composefs-backend/gc: Refactor and add logs

Add debug logs for whatever is being deleted
Remove the `delete` param from `delete_deployment` function
Use `atomic_replace_with` instead of writing to a buffer then writing

Signed-off-by: Pragyan Poudyal <[email protected]>

Author: Johan-Liebert1

crates/lib/src/bootc_composefs/boot.rs

@@ -696,28 +696,27 @@ fn write_grub_uki_menuentry(
     //
     // TODO: We might find a staged deployment here
     if is_upgrade {
-        let mut buffer = vec![];
-
-        // Shouldn't really fail so no context here
-        buffer.write_all(efi_uuid_source.as_bytes())?;
-        buffer.write_all(
-            MenuEntry::new(&boot_label, &id.to_hex())
-                .to_string()
-                .as_bytes(),
-        )?;
-
         let mut str_buf = String::new();
         let boot_dir =
             Dir::open_ambient_dir(boot_dir, ambient_authority()).context("Opening boot dir")?;
         let entries = get_sorted_grub_uki_boot_entries(&boot_dir, &mut str_buf)?;
 
-        // Write out only the currently booted entry, which should be the very first one
-        // Even if we have booted into the second menuentry "boot entry", the default will be the
-        // first one
-        buffer.write_all(entries[0].to_string().as_bytes())?;
-
         grub_dir
-            .atomic_write(user_cfg_name, buffer)
+            .atomic_replace_with(user_cfg_name, |f| -> std::io::Result<_> {
+                f.write_all(efi_uuid_source.as_bytes())?;
+                f.write_all(
+                    MenuEntry::new(&boot_label, &id.to_hex())
+                        .to_string()
+                        .as_bytes(),
+                )?;
+
+                // Write out only the currently booted entry, which should be the very first one
+                // Even if we have booted into the second menuentry "boot entry", the default will be the
+                // first one
+                f.write_all(entries[0].to_string().as_bytes())?;
+
+                Ok(())
+            })
             .with_context(|| format!("Writing to {user_cfg_name}"))?;
 
         rustix::fs::fsync(grub_dir.reopen_as_ownedfd()?).context("fsync")?;
@@ -737,18 +736,17 @@ fn write_grub_uki_menuentry(
     )?;
 
     // Write to grub2/user.cfg
-    let mut buffer = vec![];
-
-    // Shouldn't really fail so no context here
-    buffer.write_all(efi_uuid_source.as_bytes())?;
-    buffer.write_all(
-        MenuEntry::new(&boot_label, &id.to_hex())
-            .to_string()
-            .as_bytes(),
-    )?;
-
     grub_dir
-        .atomic_write(user_cfg_name, buffer)
+        .atomic_replace_with(user_cfg_name, |f| -> std::io::Result<_> {
+            f.write_all(efi_uuid_source.as_bytes())?;
+            f.write_all(
+                MenuEntry::new(&boot_label, &id.to_hex())
+                    .to_string()
+                    .as_bytes(),
+            )?;
+
+            Ok(())
+        })
         .with_context(|| format!("Writing to {user_cfg_name}"))?;
 
     rustix::fs::fsync(grub_dir.reopen_as_ownedfd()?).context("fsync")?;

crates/lib/src/bootc_composefs/delete.rs

@@ -28,11 +28,6 @@ use crate::{
     status::Slot,
 };
 
-pub(crate) struct ObjectRefs {
-    pub(crate) other_depl: HashSet<Sha512HashValue>,
-    pub(crate) depl_to_del: HashSet<Sha512HashValue>,
-}
-
 #[fn_error_context::context("Deleting Type1 Entry {}", depl.deployment.verity)]
 fn delete_type1_entry(depl: &DeploymentEntry, boot_dir: &Dir, deleting_staged: bool) -> Result<()> {
     let entries_dir_path = if deleting_staged {
@@ -78,6 +73,7 @@ fn delete_type1_entry(depl: &DeploymentEntry, boot_dir: &Dir, deleting_staged: b
                 }
 
                 // Boot dir in case of EFI will be the ESP
+                tracing::debug!("Deleting EFI .conf file: {}", file_name);
                 entry.remove_file().context("Removing .conf file")?;
                 delete_uki(&depl.deployment.verity, boot_dir)?;
 
@@ -93,6 +89,7 @@ fn delete_type1_entry(depl: &DeploymentEntry, boot_dir: &Dir, deleting_staged: b
                     continue;
                 }
 
+                tracing::debug!("Deleting non-EFI .conf file: {}", file_name);
                 entry.remove_file().context("Removing .conf file")?;
 
                 if should_del_kernel {
@@ -107,6 +104,10 @@ fn delete_type1_entry(depl: &DeploymentEntry, boot_dir: &Dir, deleting_staged: b
     }
 
     if deleting_staged {
+        tracing::debug!(
+            "Deleting staged entries directory: {}",
+            TYPE1_ENT_PATH_STAGED
+        );
         boot_dir
             .remove_dir_all(TYPE1_ENT_PATH_STAGED)
             .context("Removing staged entries dir")?;
@@ -122,11 +123,13 @@ fn delete_kernel_initrd(bls_config: &BLSConfigType, boot_dir: &Dir) -> Result<()
     };
 
     // "linux" and "initrd" are relative to the boot_dir in our config files
+    tracing::debug!("Deleting kernel: {:?}", linux);
     boot_dir
         .remove_file(linux)
         .with_context(|| format!("Removing {linux:?}"))?;
 
     for ird in initrd {
+        tracing::debug!("Deleting initrd: {:?}", ird);
         boot_dir
             .remove_file(ird)
             .with_context(|| format!("Removing {ird:?}"))?;
@@ -144,6 +147,7 @@ fn delete_kernel_initrd(bls_config: &BLSConfigType, boot_dir: &Dir) -> Result<()
     if kernel_parent_dir.entries().iter().len() == 0 {
         // We don't have anything other than kernel and initrd in this directory for now
         // So this directory should *always* be empty, for now at least
+        tracing::debug!("Deleting empty kernel directory: {:?}", dir);
         kernel_parent_dir.remove_open_dir()?;
     };
 
@@ -162,9 +166,11 @@ fn delete_uki(uki_id: &str, esp_mnt: &Dir) -> Result<()> {
 
         // The actual UKI PE binary
         if entry_name == format!("{}{}", uki_id, EFI_EXT) {
+            tracing::debug!("Deleting UKI: {}", entry_name);
             entry.remove_file().context("Deleting UKI")?;
         } else if entry_name == format!("{}{}", uki_id, EFI_ADDON_DIR_EXT) {
             // Addons dir
+            tracing::debug!("Deleting UKI addons directory: {}", entry_name);
             ukis.remove_dir_all(entry_name)
                 .context("Deleting UKI addons dir")?;
         }
@@ -178,6 +184,7 @@ fn remove_grub_menucfg_entry(id: &str, boot_dir: &Dir, deleting_staged: bool) ->
     let grub_dir = boot_dir.open_dir("grub2").context("Opening grub2")?;
 
     if deleting_staged {
+        tracing::debug!("Deleting staged grub menuentry file: {}", USER_CFG_STAGED);
         return grub_dir
             .remove_file(USER_CFG_STAGED)
             .context("Deleting staged Menuentry");
@@ -186,20 +193,20 @@ fn remove_grub_menucfg_entry(id: &str, boot_dir: &Dir, deleting_staged: bool) ->
     let mut string = String::new();
     let menuentries = get_sorted_grub_uki_boot_entries(boot_dir, &mut string)?;
 
-    let mut buffer = vec![];
-
-    buffer.write_all(get_efi_uuid_source().as_bytes())?;
+    grub_dir
+        .atomic_replace_with(USER_CFG_STAGED, move |f| -> std::io::Result<_> {
+            f.write_all(get_efi_uuid_source().as_bytes())?;
 
-    for entry in menuentries {
-        if entry.body.chainloader.contains(id) {
-            continue;
-        }
+            for entry in menuentries {
+                if entry.body.chainloader.contains(id) {
+                    continue;
+                }
 
-        buffer.write_all(entry.to_string().as_bytes())?;
-    }
+                f.write_all(entry.to_string().as_bytes())?;
+            }
 
-    grub_dir
-        .atomic_write(USER_CFG_STAGED, buffer)
+            Ok(())
+        })
         .with_context(|| format!("Writing to {USER_CFG_STAGED}"))?;
 
     rustix::fs::fsync(grub_dir.reopen_as_ownedfd().context("Reopening")?).context("fsync")?;
@@ -245,7 +252,8 @@ fn delete_depl_boot_entries(deployment: &DeploymentEntry, deleting_staged: bool)
     }
 }
 
-pub(crate) fn get_image_objects(sysroot: &Dir, deployment_id: Option<&str>) -> Result<ObjectRefs> {
+#[fn_error_context::context("Getting image objects")]
+pub(crate) fn get_image_objects(sysroot: &Dir) -> Result<HashSet<Sha512HashValue>> {
     let repo = open_composefs_repo(&sysroot)?;
 
     let images_dir = sysroot
@@ -256,10 +264,7 @@ pub(crate) fn get_image_objects(sysroot: &Dir, deployment_id: Option<&str>) -> R
         .entries_utf8()
         .context("Reading entries in images dir")?;
 
-    let mut object_refs = ObjectRefs {
-        other_depl: HashSet::new(),
-        depl_to_del: HashSet::new(),
-    };
+    let mut object_refs = HashSet::new();
 
     for image in image_entries {
         let image = image?;
@@ -270,52 +275,56 @@ pub(crate) fn get_image_objects(sysroot: &Dir, deployment_id: Option<&str>) -> R
             .objects_for_image(&img_name)
             .with_context(|| format!("Getting objects for image {img_name}"))?;
 
-        if let Some(deployment_id) = deployment_id {
-            if deployment_id == img_name {
-                object_refs.depl_to_del.extend(objects);
-            }
-        } else {
-            object_refs.other_depl.extend(objects);
-        }
+        object_refs.extend(objects);
     }
 
     Ok(object_refs)
 }
 
+#[fn_error_context::context("Deleting image for deployment {}", deployment_id)]
 pub(crate) fn delete_image(sysroot: &Dir, deployment_id: &str) -> Result<()> {
     let img_path = Path::new("composefs").join("images").join(deployment_id);
 
+    tracing::debug!("Deleting EROFS image: {:?}", img_path);
     sysroot
         .remove_file(&img_path)
         .context("Deleting EROFS image")
 }
 
+#[fn_error_context::context("Deleting state directory for deployment {}", deployment_id)]
 pub(crate) fn delete_state_dir(sysroot: &Dir, deployment_id: &str) -> Result<()> {
     let state_dir = Path::new(STATE_DIR_RELATIVE).join(deployment_id);
 
+    tracing::debug!("Deleting state directory: {:?}", state_dir);
     sysroot
         .remove_dir_all(&state_dir)
         .with_context(|| format!("Removing dir {state_dir:?}"))
 }
 
+#[fn_error_context::context("Deleting staged deployment")]
 pub(crate) fn delete_staged(staged: &Option<BootEntry>) -> Result<()> {
     if staged.is_none() {
         tracing::debug!("No staged deployment");
         return Ok(());
     };
 
     let file = Path::new(COMPOSEFS_TRANSIENT_STATE_DIR).join(COMPOSEFS_STAGED_DEPLOYMENT_FNAME);
-    tracing::debug!("Deleting staged file {file:?}");
+    tracing::debug!("Deleting staged deployment file: {file:?}");
     std::fs::remove_file(file).context("Removing staged file")?;
 
     Ok(())
 }
 
-pub(crate) async fn delete_composefs_deployment(deployment_id: &str, delete: bool) -> Result<()> {
+#[fn_error_context::context("Deleting composefs deployment {}", deployment_id)]
+pub(crate) async fn delete_composefs_deployment(deployment_id: &str) -> Result<()> {
     let host = composefs_deployment_status().await?;
 
     let booted = host.require_composefs_booted()?;
 
+    if deployment_id == &booted.verity {
+        anyhow::bail!("Cannot delete currently booted deployment");
+    }
+
     let all_depls = host.all_composefs_deployments()?;
 
     let depl_to_del = all_depls
@@ -338,15 +347,6 @@ pub(crate) async fn delete_composefs_deployment(deployment_id: &str, delete: boo
         composefs_rollback().await?;
     }
 
-    // For debugging, but maybe useful elsewhere?
-    if !delete {
-        return Ok(());
-    }
-
-    if deployment_id == &booted.verity {
-        anyhow::bail!("Cannot delete currently booted deployment");
-    }
-
     let kind = if depl_to_del.pinned {
         "pinned "
     } else if deleting_staged {

crates/lib/src/bootc_composefs/gc.rs

@@ -24,6 +24,7 @@ use crate::{
     spec::Bootloader,
 };
 
+#[fn_error_context::context("Listing EROFS images")]
 fn list_erofs_images(sysroot: &Dir) -> Result<Vec<String>> {
     let images_dir = sysroot
         .open_dir("composefs/images")
@@ -44,6 +45,7 @@ fn list_erofs_images(sysroot: &Dir) -> Result<Vec<String>> {
 ///
 /// # Returns
 /// The fsverity of EROFS images corresponding to boot entries
+#[fn_error_context::context("Listing bootloader entries")]
 fn list_bootloader_entries() -> Result<Vec<String>> {
     let bootloader = get_bootloader()?;
 
@@ -92,6 +94,7 @@ fn list_bootloader_entries() -> Result<Vec<String>> {
     Ok(entries)
 }
 
+#[fn_error_context::context("Listing state directories")]
 fn list_state_dirs(sysroot: &Dir) -> Result<Vec<String>> {
     let state = sysroot
         .open_dir(STATE_DIR_RELATIVE)
@@ -116,9 +119,13 @@ fn list_state_dirs(sysroot: &Dir) -> Result<Vec<String>> {
 /// present EROFS images
 ///
 /// We do not delete streams though
+#[fn_error_context::context("Garbage collecting objects")]
+// TODO(Johan-Liebert1): This will be moved to composefs-rs
 pub(crate) fn gc_objects(sysroot: &Dir) -> Result<()> {
+    tracing::debug!("Running garbage collection on unreferenced objects");
+
     // Get all the objects referenced by all available images
-    let obj_refs = get_image_objects(sysroot, None)?;
+    let obj_refs = get_image_objects(sysroot)?;
 
     // List all objects in the objects directory
     let objects_dir = sysroot
@@ -141,8 +148,8 @@ pub(crate) fn gc_objects(sysroot: &Dir) -> Result<()> {
             let id = Sha512HashValue::from_object_dir_and_basename(dir_name, filename.as_bytes())?;
 
             // If this object is not referenced by any image, delete it
-            if !obj_refs.other_depl.contains(&id) {
-                tracing::trace!("Removed unreferenced object: {filename}");
+            if !obj_refs.contains(&id) {
+                tracing::trace!("Deleting unreferenced object: {filename}");
 
                 entry
                     .remove_file()
@@ -164,6 +171,7 @@ pub(crate) fn gc_objects(sysroot: &Dir) -> Result<()> {
 ///
 /// Similarly if EROFS image B1 doesn't exist, but state dir does, then delete the state dir and
 /// perform GC
+#[fn_error_context::context("Running composefs garbage collection")]
 pub(crate) async fn composefs_gc() -> Result<()> {
     let host = composefs_deployment_status().await?;
     let booted_cfs = host.require_composefs_booted()?;
@@ -207,8 +215,6 @@ pub(crate) async fn composefs_gc() -> Result<()> {
         .collect::<Vec<_>>();
 
     for verity in &state_img_diff {
-        tracing::debug!("Cleaning up orphaned state directory: {verity}");
-
         delete_staged(staged)?;
         delete_state_dir(&sysroot, verity)?;
     }

crates/lib/src/bootc_composefs/rollback.rs

@@ -1,4 +1,4 @@
-use std::fmt::Write;
+use std::io::Write;
 
 use anyhow::{anyhow, Context, Result};
 use cap_std_ext::cap_std::ambient_authority;
@@ -92,16 +92,18 @@ fn rollback_grub_uki_entries(boot_dir: &Dir) -> Result<()> {
     let (first, second) = menuentries.split_at_mut(1);
     std::mem::swap(&mut first[0], &mut second[0]);
 
-    let mut buffer = get_efi_uuid_source();
-
-    for entry in menuentries {
-        write!(buffer, "{entry}")?;
-    }
-
     let entries_dir = boot_dir.open_dir("grub2").context("Opening grub dir")?;
 
     entries_dir
-        .atomic_write(USER_CFG_STAGED, buffer)
+        .atomic_replace_with(USER_CFG_STAGED, |f| -> std::io::Result<_> {
+            f.write_all(get_efi_uuid_source().as_bytes())?;
+
+            for entry in menuentries {
+                f.write_all(entry.to_string().as_bytes())?;
+            }
+
+            Ok(())
+        })
         .with_context(|| format!("Writing to {USER_CFG_STAGED}"))?;
 
     rename_exchange_user_cfg(&entries_dir)