Replace full key with S3Location in file handles (#1539)

Internal change to directly propagate `S3Location` in file handles
rather than the derived `full_key` string. The value is used for logging
and error report, so we can postpone formatting the string until it is
needed.

### Does this change impact existing behavior?

Minor change in string formatting in logs.

### Does this change need a changelog entry? Does it require a version
change?

No.

---

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and I agree to the terms of
the [Developer Certificate of Origin
(DCO)](https://developercertificate.org/).

Signed-off-by: Alessandro Passaro <[email protected]>

Author: passaro

mountpoint-s3-fs/src/fs.rs

@@ -689,14 +689,14 @@ where
         };
 
         let complete_result = write_state
-            .complete_if_in_progress(self, file_handle.ino, &file_handle.location.full_key())
+            .complete_if_in_progress(self, file_handle.ino, &file_handle.location)
             .await;
         metrics::gauge!("fs.current_handles", "type" => "write").decrement(1.0);
 
         match complete_result {
             Ok(upload_completed_async) => {
                 if upload_completed_async {
-                    debug!(key = ?&file_handle.location.full_key(), "upload completed async after file was closed");
+                    debug!(key = %file_handle.location, "upload completed async after file was closed");
                 }
                 Ok(())
             }

mountpoint-s3-fs/src/fs/handles.rs

@@ -154,18 +154,10 @@ where
                 Err(e) => Err(e.into()),
             },
             UploadState::Completed => {
-                return Err(err!(
-                    libc::EIO,
-                    "upload already completed for key {:?}",
-                    handle.location.full_key()
-                ));
+                return Err(err!(libc::EIO, "upload already completed for key {}", handle.location));
             }
             UploadState::Failed(e) => {
-                return Err(err!(
-                    *e,
-                    "upload already aborted for key {:?}",
-                    handle.location.full_key()
-                ));
+                return Err(err!(*e, "upload already aborted for key {}", handle.location));
             }
         };
 
@@ -194,11 +186,7 @@ where
         match &self {
             UploadState::Completed => return Ok(()),
             UploadState::Failed(e) => {
-                return Err(err!(
-                    *e,
-                    "upload already aborted for key {:?}",
-                    handle.location.full_key()
-                ));
+                return Err(err!(*e, "upload already aborted for key {}", handle.location));
             }
             _ => {}
         };
@@ -210,7 +198,7 @@ where
                 written_bytes,
             } => {
                 let current_offset = request.current_offset();
-                match Self::commit_append(request, &handle.location.full_key()).await {
+                match Self::commit_append(request, &handle.location).await {
                     Ok(etag) => {
                         // Restart append request.
                         let initial_etag = etag.or(initial_etag);
@@ -234,7 +222,7 @@ where
                 }
             }
             UploadState::MPUInProgress { request, .. } => {
-                let result = Self::complete_upload(fs, handle.ino, handle.location.full_key().as_ref(), request).await;
+                let result = Self::complete_upload(fs, handle.ino, &handle.location, request).await;
                 if let Err(e) = &result {
                     *self = UploadState::Failed(e.to_errno());
                 }
@@ -260,12 +248,12 @@ where
             }
             UploadState::MPUInProgress { request, .. } => {
                 if request.size() == 0 {
-                    debug!(key=?handle.location.full_key(), "not completing upload because nothing was written yet");
+                    debug!(key=%handle.location, "not completing upload because nothing was written yet");
                     return Ok(());
                 }
                 if !are_from_same_process(open_pid, pid) {
                     debug!(
-                        key=?handle.location.full_key(),
+                        key=%handle.location,
                         pid, open_pid, "not completing upload because current PID differs from PID at open",
                     );
                     return Ok(());
@@ -284,9 +272,9 @@ where
         let result = match std::mem::replace(self, UploadState::Completed) {
             UploadState::AppendInProgress {
                 request, initial_etag, ..
-            } => Self::complete_append(fs, handle.ino, &handle.location.full_key(), request, initial_etag).await,
+            } => Self::complete_append(fs, handle.ino, &handle.location, request, initial_etag).await,
             UploadState::MPUInProgress { request, .. } => {
-                Self::complete_upload(fs, handle.ino, &handle.location.full_key(), request).await
+                Self::complete_upload(fs, handle.ino, &handle.location, request).await
             }
             UploadState::Failed(_) | UploadState::Completed => unreachable!("checked above"),
         };
@@ -304,7 +292,7 @@ where
         self,
         fs: &S3Filesystem<Client>,
         ino: InodeNo,
-        key: &str,
+        key: &S3Location,
     ) -> Result<bool, Error> {
         match self {
             UploadState::AppendInProgress {
@@ -324,28 +312,28 @@ where
     async fn complete_upload(
         fs: &S3Filesystem<Client>,
         ino: InodeNo,
-        key: &str,
+        key: &S3Location,
         upload: UploadRequest<Client>,
     ) -> Result<(), Error> {
         let size = upload.size();
         let (put_result, etag) = match upload.complete().await {
             Ok(result) => {
-                debug!(etag=?result.etag.as_str(), ?key, size, "put succeeded");
+                debug!(etag=?result.etag.as_str(), %key, size, "put succeeded");
                 (Ok(()), Some(result.etag))
             }
             Err(e) => (Err(err!(libc::EIO, source:e, "put failed")), None),
         };
         if let Err(err) = fs.metablock.finish_writing(ino, etag).await {
             // Log the issue but still return put_result.
-            error!(?err, ?key, "error updating the inode status");
+            error!(?err, %key, "error updating the inode status");
         }
         put_result
     }
 
     async fn complete_append(
         fs: &S3Filesystem<Client>,
         ino: InodeNo,
-        key: &str,
+        key: &S3Location,
         upload: AppendUploadRequest<Client>,
         initial_etag: Option<ETag>,
     ) -> Result<(), Error> {
@@ -361,14 +349,14 @@ where
         }
     }
 
-    async fn commit_append(upload: AppendUploadRequest<Client>, key: &str) -> Result<Option<ETag>, Error> {
+    async fn commit_append(upload: AppendUploadRequest<Client>, key: &S3Location) -> Result<Option<ETag>, Error> {
         match upload.complete().await {
             Ok(Some(result)) => {
-                debug!(key, "put succeeded");
+                debug!(%key, "put succeeded");
                 Ok(Some(result.etag))
             }
             Ok(None) => {
-                debug!(key, "no put required");
+                debug!(%key, "no put required");
                 Ok(None)
             }
             Err(e) => Err(err!(libc::EIO, source:e, "put failed")),

mountpoint-s3-fs/src/metablock/path.rs

@@ -183,6 +183,16 @@ impl S3Location {
     }
 }
 
+impl Display for S3Location {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "{}{} (bucket: {})",
+            self.path.prefix, self.partial_key, self.path.bucket
+        )
+    }
+}
+
 /// A valid name for an [Inode](super::Inode).
 #[derive(Debug, Clone, Copy)]
 pub struct ValidName<'a>(&'a str);