DLPX-85760 zoa panic: Failed to get zfs/.../data/... (openzfs#829)

If there is an unclean shutdown (e.g. kernel panic or power loss) while
a txg is in progress, the agent may have created some objects for the
in-progress txg, which are no longer needed.  These objects are
destroyed when the agent next starts, so that they are not leaked.  The
code that implements this, `PoolState::cleanup_data_objects()`, assumes
that the last object written is `ObjectVersion(None)`, i.e. the reclaim
code has not rewritten the last object.

Unfortunately, this assumption is not true, which leads to the deletion
of the last object, which is still in use.  The bug can be triggered on
any agent restart, even if the system did not reboot.

This PR addresses the issue by taking into account the ObjectVersion
when determining the last-valid object’s key.

The problem was introduced by openzfs#668.  The fix is simple because the
version suffix is `-#`, rather than `/#` which was used earlier in the
development of openzfs#668.

cmd/zfs_object_agent/zettaobject/src/base_types.rs

@@ -1,3 +1,4 @@
+use std::fmt;
 use std::fmt::*;
 use std::num::NonZeroU16;
 
@@ -107,6 +108,19 @@ impl VersionedObjectId {
     }
 }
 
+impl Display for VersionedObjectId {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // Note, the ObjectId and Version are delimited by a `-` rather than a `/`, because MinIO
+        // does not support objects whose names are a prefix of other objects, and the
+        // ObjectVersion(None) and ObjectVersion(Some(1)) of a given ObjectId will both exist at
+        // the same time.
+        match self.version.0 {
+            Some(version) => write!(f, "{}-{version}", self.object),
+            None => write!(f, "{}", self.object),
+        }
+    }
+}
+
 /// This is a helper struct to allow us to deserialize a VersionedObjectId from either a legacy
 /// ObjectId, or the new ObjectId + ObjectVersion, which is needed when reading the old on-disk
 /// format.  It also allows us to serialize a VersionedObjectId (with ObjectVersion(None)) the

cmd/zfs_object_agent/zettaobject/src/data_object.rs

@@ -74,26 +74,13 @@ impl Key {
 
 impl Display for Key {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        // Note, the ObjectId and Version are delimited by a `-` rather than a `/`, because MinIO
-        // does not support objects whose names are a prefix of other objects, and the
-        // ObjectVersion(None) and ObjectVersion(Some(1)) of a given ObjectId will both exist at
-        // the same time.
-        match self.versioned_object.version.0 {
-            Some(version) => write!(
-                f,
-                "zfs/{}/data/{:03}/{}-{version}",
-                self.guid,
-                self.versioned_object.object.prefix(),
-                self.versioned_object.object,
-            ),
-            None => write!(
-                f,
-                "zfs/{}/data/{:03}/{}",
-                self.guid,
-                self.versioned_object.object.prefix(),
-                self.versioned_object.object,
-            ),
-        }
+        write!(
+            f,
+            "zfs/{}/data/{:03}/{}",
+            self.guid,
+            self.versioned_object.object.prefix(),
+            self.versioned_object,
+        )
     }
 }
 

cmd/zfs_object_agent/zettaobject/src/pool.rs

@@ -836,7 +836,7 @@ impl PoolState {
         let shared_state = self.shared_state.clone();
         let oa = &shared_state.object_access;
         let begin = Instant::now();
-        let last_object = self.object_block_map.last_object().object;
+        let last_object = self.object_block_map.last_object();
         let mut count: u32 = 0;
 
         oa.delete_objects(
@@ -1825,7 +1825,7 @@ async fn recover_list(
     assert!(shared_state.object_access.supports_list_after());
 
     let begin = Instant::now();
-    let last_object = state.object_block_map.last_object().object;
+    let last_object = state.object_block_map.last_object();
     let list_stream = FuturesUnordered::new();
     for prefix in DataObject::prefixes(shared_state.guid) {
         let shared_state = shared_state.clone();