Improve revision reaping (#725)

firewood/src/manager.rs

@@ -143,14 +143,14 @@ impl RevisionManager {
             return Err(RevisionManagerError::NotLatest);
         }
 
-        let mut committed = proposal.as_committed();
+        let committed = proposal.as_committed();
 
         // 2. Persist delete list for this committed revision to disk for recovery
 
         // 3 Take the deleted entries from the oldest revision and mark them as free for this revision
         // If you crash after freeing some of these, then the free list will point to nodes that are not actually free.
         // TODO: Handle the case where we get something off the free list that is not free
-        while self.historical.len() > self.max_revisions {
+        while self.historical.len() >= self.max_revisions {
             let oldest = self.historical.pop_front().expect("must be present");
             if let Some(oldest_hash) = oldest.kind.root_hash() {
                 self.by_hash.remove(&oldest_hash);
@@ -162,9 +162,8 @@ impl RevisionManager {
             // This guarantee is there because we have a `&mut self` reference to the manager, so
             // the compiler guarantees we are the only one using this manager.
             match Arc::try_unwrap(oldest) {
-                Ok(oldest) => committed.reap_deleted(&oldest)?,
+                Ok(oldest) => oldest.reap_deleted()?,
                 Err(original) => {
-                    // TODO: try reaping the next revision
                     warn!("Oldest revision could not be reaped; still referenced");
                     self.historical.push_front(original);
                 }

storage/src/nodestore.rs

@@ -53,7 +53,7 @@ use std::fmt::Debug;
 /// ```
 use std::io::{Error, ErrorKind, Write};
 use std::iter::once;
-use std::mem::offset_of;
+use std::mem::{offset_of, take};
 use std::num::NonZeroU64;
 use std::ops::Deref;
 use std::sync::Arc;
@@ -1092,12 +1092,12 @@ where
 
 impl<S: WritableStorage> NodeStore<Committed, S> {
     /// adjust the freelist of this proposal to reflect the freed nodes in the oldest proposal
-    pub fn reap_deleted(&mut self, oldest: &NodeStore<Committed, S>) -> Result<(), Error> {
+    pub fn reap_deleted(mut self) -> Result<(), Error> {
         self.storage
-            .invalidate_cached_nodes(oldest.kind.deleted.iter());
-        trace!("There are {} nodes to reap", oldest.kind.deleted.len());
-        for addr in oldest.kind.deleted.iter() {
-            self.delete_node(*addr)?;
+            .invalidate_cached_nodes(self.kind.deleted.iter());
+        trace!("There are {} nodes to reap", self.kind.deleted.len());
+        for addr in take(&mut self.kind.deleted) {
+            self.delete_node(addr)?;
         }
         Ok(())
     }