Skip to content

Commit

Permalink
Added filename in warn span
Browse files Browse the repository at this point in the history 
Signed-off-by: Ankit Saurabh <[email protected]>
  • Loading branch information
Ankit Saurabh committed Dec 3, 2023
1 parent 3e1d9ee commit 73cb83b
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 25 deletions.
6 changes: 5 additions & 1 deletion mountpoint-s3/src/fs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::ffi::{OsStr, OsString};
use std::str::FromStr;
use std::time::{Duration, UNIX_EPOCH};
use time::OffsetDateTime;
use tracing::{debug, error, trace};
use tracing::{debug, error, trace, Span};

use fuser::consts::FOPEN_DIRECT_IO;
use fuser::{FileAttr, KernelConfig};
Expand Down Expand Up @@ -666,6 +666,7 @@ where
None => return reply.error(err!(libc::EBADF, "invalid file handle")),
}
};
Span::current().record("name", handle.inode.name());
let file_etag: ETag;
let mut request = match &handle.typ {
FileHandleType::Write { .. } => return reply.error(err!(libc::EBADF, "file handle is not open for reads")),
Expand Down Expand Up @@ -770,6 +771,7 @@ where
None => return Err(err!(libc::EBADF, "invalid file handle")),
}
};
Span::current().record("name", handle.inode.name());

let len = {
let mut request = match &handle.typ {
Expand Down Expand Up @@ -971,6 +973,7 @@ where
None => return Err(err!(libc::EBADF, "invalid file handle")),
}
};
Span::current().record("name", file_handle.inode.name());
let mut request = match &file_handle.typ {
FileHandleType::Write(request) => request.lock().await,
FileHandleType::Read { .. } => return Ok(()),
Expand Down Expand Up @@ -1019,6 +1022,7 @@ where
.remove(&fh)
.ok_or_else(|| err!(libc::EBADF, "invalid file handle"))?
};
Span::current().record("name", file_handle.inode.name());

// Unwrap the atomic reference to have full ownership.
// The kernel should make a release call when there is no more references to the file handle,
Expand Down
3 changes: 2 additions & 1 deletion mountpoint-s3/src/fs/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,9 @@ impl ToErrno for InodeError {
fn to_errno(&self) -> libc::c_int {
match self {
InodeError::ClientError(_) => libc::EIO,
InodeError::FileDoesNotExist => libc::ENOENT,
InodeError::FileDoesNotExist(_) => libc::ENOENT,
InodeError::InodeDoesNotExist(_) => libc::ENOENT,
InodeError::DirectoryDoesNotExist(_) => libc::ENOENT,
InodeError::InvalidFileName(_) => libc::EINVAL,
InodeError::NotADirectory(_) => libc::ENOTDIR,
InodeError::IsDirectory(_) => libc::EISDIR,
Expand Down
22 changes: 11 additions & 11 deletions mountpoint-s3/src/fuse.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::ffi::OsStr;
use std::path::Path;
use std::time::SystemTime;
use time::OffsetDateTime;
use tracing::{instrument, Instrument};
use tracing::{field, instrument, Instrument};

use crate::fs::{
self, DirectoryEntry, DirectoryReplier, InodeNo, ReadReplier, S3Filesystem, S3FilesystemConfig, ToErrno,
Expand Down Expand Up @@ -92,28 +92,28 @@ where
}
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, name=field::Empty))]
fn getattr(&self, _req: &Request<'_>, ino: InodeNo, reply: ReplyAttr) {
match block_on(self.fs.getattr(ino).in_current_span()) {
Ok(attr) => reply.attr(&attr.ttl, &attr.attr),
Err(e) => fuse_error!("getattr", reply, e),
}
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino, nlookup))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino, nlookup, name=field::Empty))]
fn forget(&self, _req: &Request<'_>, ino: u64, nlookup: u64) {
block_on(self.fs.forget(ino, nlookup));
}

#[instrument(level="warn", skip_all, fields(req=req.unique(), ino=ino, pid=req.pid()))]
#[instrument(level="warn", skip_all, fields(req=req.unique(), ino=ino, pid=req.pid(), name=field::Empty))]
fn open(&self, req: &Request<'_>, ino: InodeNo, flags: i32, reply: ReplyOpen) {
match block_on(self.fs.open(ino, flags, req.pid()).in_current_span()) {
Ok(opened) => reply.opened(opened.fh, opened.flags),
Err(e) => fuse_error!("open", reply, e),
}
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh, offset=offset, size=size))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh, offset=offset, size=size, name=field::Empty))]
fn read(
&self,
_req: &Request<'_>,
Expand Down Expand Up @@ -164,7 +164,7 @@ where
metrics::histogram!("fuse.io_size", bytes_sent as f64, "type" => "read");
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=parent))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=parent, name=field::Empty))]
fn opendir(&self, _req: &Request<'_>, parent: InodeNo, flags: i32, reply: ReplyOpen) {
match block_on(self.fs.opendir(parent, flags).in_current_span()) {
Ok(opened) => reply.opened(opened.fh, opened.flags),
Expand Down Expand Up @@ -250,23 +250,23 @@ where
}
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh, datasync=datasync))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh, datasync=datasync, name=field::Empty))]
fn fsync(&self, _req: &Request<'_>, ino: u64, fh: u64, datasync: bool, reply: ReplyEmpty) {
match block_on(self.fs.fsync(ino, fh, datasync).in_current_span()) {
Ok(()) => reply.ok(),
Err(e) => fuse_error!("fsync", reply, e),
}
}

#[instrument(level="warn", skip_all, fields(req=req.unique(), ino=ino, fh=fh, pid=req.pid()))]
#[instrument(level="warn", skip_all, fields(req=req.unique(), ino=ino, fh=fh, pid=req.pid(), name=field::Empty))]
fn flush(&self, req: &Request<'_>, ino: u64, fh: u64, lock_owner: u64, reply: ReplyEmpty) {
match block_on(self.fs.flush(ino, fh, lock_owner, req.pid()).in_current_span()) {
Ok(()) => reply.ok(),
Err(e) => fuse_error!("flush", reply, e),
}
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh, name=field::Empty))]
fn release(
&self,
_req: &Request<'_>,
Expand Down Expand Up @@ -322,7 +322,7 @@ where
}
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh, offset=offset, length=data.len(), pid=_req.pid()))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, fh=fh, offset=offset, length=data.len(), pid=_req.pid(), name=field::Empty))]
fn write(
&self,
_req: &Request<'_>,
Expand Down Expand Up @@ -365,7 +365,7 @@ where
}
}

#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino))]
#[instrument(level="warn", skip_all, fields(req=_req.unique(), ino=ino, name=field::Empty))]
fn setattr(
&self,
_req: &Request<'_>,
Expand Down
26 changes: 16 additions & 10 deletions mountpoint-s3/src/inode.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use mountpoint_s3_client::ObjectClient;
use mountpoint_s3_crt::checksums::crc32c::{self, Crc32c};
use thiserror::Error;
use time::OffsetDateTime;
use tracing::{debug, error, trace, warn};
use tracing::{debug, error, trace, warn, Span};

use crate::fs::{CacheConfig, S3Personality};
use crate::prefix::Prefix;
Expand Down Expand Up @@ -139,6 +139,7 @@ impl Superblock {
error!("forget called on inode {ino} already removed from the superblock");
}
Some(inode) => {
Span::current().record("name", inode.name());
let new_lookup_count = inode.dec_lookup_count(n);
if new_lookup_count == 0 {
// Safe to remove, kernel no longer has a reference to it.
Expand Down Expand Up @@ -209,6 +210,7 @@ impl Superblock {
force_revalidate: bool,
) -> Result<LookedUp, InodeError> {
let inode = self.inner.get(ino)?;
Span::current().record("name", inode.name());

if !force_revalidate {
let sync = inode.get_inode_state()?;
Expand Down Expand Up @@ -243,6 +245,7 @@ impl Superblock {
mtime: Option<OffsetDateTime>,
) -> Result<LookedUp, InodeError> {
let inode = self.inner.get(ino)?;
Span::current().record("name", inode.name());
let mut sync = inode.get_mut_inode_state()?;

if sync.write_status == WriteStatus::Remote {
Expand Down Expand Up @@ -294,6 +297,7 @@ impl Superblock {
trace!(dir=?dir_ino, "readdir");

let dir = self.inner.get(dir_ino)?;
Span::current().record("name", dir.name());
if dir.kind() != InodeKind::Directory {
return Err(InodeError::NotADirectory(dir.err()));
}
Expand Down Expand Up @@ -326,7 +330,7 @@ impl Superblock {
.await;
match existing {
Ok(lookup) => return Err(InodeError::FileAlreadyExists(lookup.inode.err())),
Err(InodeError::FileDoesNotExist) => (),
Err(InodeError::FileDoesNotExist(_)) => (),
Err(e) => return Err(e),
}

Expand Down Expand Up @@ -801,7 +805,7 @@ impl SuperblockInner {
InodeKindData::Directory { children, .. } => children.get(name),
};
match (remote, inode) {
(None, None) => Err(InodeError::FileDoesNotExist),
(None, None) => Err(InodeError::FileDoesNotExist(name.into())),
(Some(remote), Some(existing_inode)) => {
let mut existing_state = existing_inode.get_mut_inode_state()?;
let existing_is_remote = existing_state.write_status == WriteStatus::Remote;
Expand Down Expand Up @@ -838,7 +842,7 @@ impl SuperblockInner {
InodeKindData::Directory { children, .. } => children.get(name).cloned(),
};
match (remote, inode) {
(None, None) => Err(InodeError::FileDoesNotExist),
(None, None) => Err(InodeError::FileDoesNotExist(name.into())),
(None, Some(existing_inode)) => {
let InodeKindData::Directory {
children,
Expand Down Expand Up @@ -868,7 +872,7 @@ impl SuperblockInner {
// being written. It must have previously existed but been removed on the remote
// side.
children.remove(name);
Err(InodeError::FileDoesNotExist)
Err(InodeError::FileDoesNotExist(name.into()))
}
}
(Some(remote), None) => {
Expand Down Expand Up @@ -1239,7 +1243,7 @@ impl Inode {
fn get_inode_state(&self) -> Result<RwLockReadGuard<InodeState>, InodeError> {
let inode_state = self.inner.sync.read().unwrap();
match &inode_state.kind_data {
InodeKindData::Directory { deleted, .. } if *deleted => Err(InodeError::InodeDoesNotExist(self.ino())),
InodeKindData::Directory { deleted, .. } if *deleted => Err(InodeError::DirectoryDoesNotExist(self.err())),
_ => Ok(inode_state),
}
}
Expand All @@ -1248,7 +1252,7 @@ impl Inode {
fn get_mut_inode_state(&self) -> Result<RwLockWriteGuard<InodeState>, InodeError> {
let inode_state = self.inner.sync.write().unwrap();
match &inode_state.kind_data {
InodeKindData::Directory { deleted, .. } if *deleted => Err(InodeError::InodeDoesNotExist(self.ino())),
InodeKindData::Directory { deleted, .. } if *deleted => Err(InodeError::DirectoryDoesNotExist(self.err())),
_ => Ok(inode_state),
}
}
Expand Down Expand Up @@ -1485,8 +1489,10 @@ impl InodeStat {
pub enum InodeError {
#[error("error from ObjectClient")]
ClientError(#[source] anyhow::Error),
#[error("file does not exist")]
FileDoesNotExist,
#[error("file does not exist {0:?}")]
FileDoesNotExist(OsString),
#[error("directory does not exist at inode {0}")]
DirectoryDoesNotExist(InodeErrorInfo),
#[error("inode {0} does not exist")]
InodeDoesNotExist(InodeNo),
#[error("invalid file name {0:?}")]
Expand Down Expand Up @@ -2473,7 +2479,7 @@ mod tests {
// All nested dirs disappear
let dirname = nested_dirs.first().unwrap();
let lookedup = superblock.lookup(&client, FUSE_ROOT_INODE, dirname.as_ref()).await;
assert!(matches!(lookedup, Err(InodeError::FileDoesNotExist)));
assert!(matches!(lookedup, Err(InodeError::FileDoesNotExist(_))));
}

#[tokio::test]
Expand Down
4 changes: 2 additions & 2 deletions mountpoint-s3/src/prefetch/part_stream.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ where
{
Ok(get_object_result) => get_object_result,
Err(e) => {
error!(error=?e, "GetObject request failed");
error!(key, error=?e, "GetObject request failed");
part_queue_producer.push(Err(PrefetchReadError::GetRequestFailed(e)));
return;
}
Expand Down Expand Up @@ -227,7 +227,7 @@ where
}
}
Some(Err(e)) => {
error!(error=?e, "GetObject body part failed");
error!(key, error=?e, "GetObject body part failed");
part_queue_producer.push(Err(PrefetchReadError::GetRequestFailed(e)));
break;
}
Expand Down

0 comments on commit 73cb83b

Please sign in to comment.