Skip to content

Commit ca71532

Browse files
passaropassaro
authored
Use explicit result type in readdir reply callback (#1700)
The readdir reply callback in the `metablock` module was misleading and its documentation was wrong: `TryAddDirEntry` would return `false` on success (contrary to the rustdoc and common sense). Note though that all existing invocations were using it correctly. This change introduces an explicit return type to avoid any confusion, at the cost of some increase in verbosity. ### Does this change impact existing behavior? No, code tidy only. ### 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 <alexpax@amazon.co.uk>
1 parent 1850e98 commit ca71532

File tree

5 files changed

+60
-41
lines changed

5 files changed

+60
-41
lines changed

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,8 @@ use crate::async_util::Runtime;
1818
use crate::logging;
1919
use crate::mem_limiter::MemoryLimiter;
2020
use crate::memory::PagedPool;
21-
use crate::metablock::Metablock;
21+
use crate::metablock::{AddDirEntry, AddDirEntryResult, InodeInformation, Metablock};
2222
pub use crate::metablock::{InodeError, InodeKind, InodeNo};
23-
use crate::metablock::{InodeInformation, TryAddDirEntry};
2423
use crate::prefetch::{Prefetcher, PrefetcherBuilder};
2524
use crate::sync::atomic::{AtomicU64, Ordering};
2625
use crate::sync::{Arc, AsyncMutex, AsyncRwLock};
@@ -579,15 +578,19 @@ where
579578
is_readdirplus: bool,
580579
mut reply: R,
581580
) -> Result<(), Error> {
582-
let adder: TryAddDirEntry = Box::new(move |information, name, offset, generation| {
583-
reply.add(DirectoryEntry {
581+
let adder: AddDirEntry = Box::new(move |information, name, offset, generation| {
582+
if reply.add(DirectoryEntry {
584583
ino: information.ino(),
585584
offset,
586585
name,
587586
attr: self.make_attr(&information),
588587
generation,
589588
ttl: information.validity(),
590-
})
589+
}) {
590+
AddDirEntryResult::ReplyBufferFull
591+
} else {
592+
AddDirEntryResult::EntryAdded
593+
}
591594
});
592595

593596
self.metablock

‎mountpoint-s3-fs/src/manifest/metablock.rs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ use time::OffsetDateTime;
88
use super::core::{Manifest, ManifestDirIter, ManifestError};
99

1010
use crate::metablock::{
11-
InodeError, InodeErrorInfo, InodeInformation, InodeKind, InodeNo, InodeStat, Lookup, Metablock, NEVER_EXPIRE_TTL,
12-
ROOT_INODE_NO, TryAddDirEntry, ValidName, WriteMode,
11+
AddDirEntry, AddDirEntryResult, InodeError, InodeErrorInfo, InodeInformation, InodeKind, InodeNo, InodeStat,
12+
Lookup, Metablock, NEVER_EXPIRE_TTL, ROOT_INODE_NO, ValidName, WriteMode,
1313
};
1414
use crate::s3::S3Path;
1515
use crate::sync::atomic::{AtomicU64, Ordering};
@@ -122,7 +122,7 @@ impl Metablock for ManifestMetablock {
122122
//
123123
// [ManifestMetablock] never forgets inodes, so this argument is unused.
124124
_is_readdirplus: bool,
125-
mut replier: TryAddDirEntry<'a>,
125+
mut add: AddDirEntry<'a>,
126126
) -> Result<(), InodeError> {
127127
let Some(readdir_handle) = self
128128
.readdir_handles
@@ -136,25 +136,27 @@ impl Metablock for ManifestMetablock {
136136

137137
// serve '.' and '..' entries
138138
if offset < 1 {
139-
if replier(
139+
if add(
140140
InodeInformation::new(parent, self.stat_for_directory(), InodeKind::Directory, true),
141141
".".into(),
142142
offset + 1,
143143
0,
144-
) {
144+
) == AddDirEntryResult::ReplyBufferFull
145+
{
145146
return Ok(());
146147
}
147148
offset += 1;
148149
}
149150

150151
if offset < 2 {
151152
let grandparent_ino = self.get_parent_id(parent)?;
152-
if replier(
153+
if add(
153154
InodeInformation::new(grandparent_ino, self.stat_for_directory(), InodeKind::Directory, true),
154155
"..".into(),
155156
offset + 1,
156157
0,
157-
) {
158+
) == AddDirEntryResult::ReplyBufferFull
159+
{
158160
return Ok(());
159161
}
160162
offset += 1;
@@ -168,7 +170,7 @@ impl Metablock for ManifestMetablock {
168170
let (inode_info, name) = manifest_entry
169171
.clone()
170172
.into_inode_information(&self.channels, self.mount_time)?;
171-
if replier(inode_info, OsString::from(name), offset + 1, 0) {
173+
if add(inode_info, OsString::from(name), offset + 1, 0) == AddDirEntryResult::ReplyBufferFull {
172174
readdir_handle.readd(manifest_entry);
173175
break;
174176
}

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

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,14 @@ pub trait Metablock: Send + Sync {
7777

7878
/// Reads entries from the readdir stream, for the directory `parent`, referred to by `fh` starting at offset `offset`.
7979
///
80-
/// Entries shall be passed onto the `replier` as described in its documentation.
80+
/// Entries shall be passed to `add` as described in its documentation.
8181
async fn readdir<'a>(
8282
&self,
8383
parent: InodeNo,
8484
fh: u64,
8585
offset: i64,
8686
is_readdirplus: bool,
87-
mut replier: TryAddDirEntry<'a>,
87+
mut add: AddDirEntry<'a>,
8888
) -> Result<(), InodeError>;
8989

9090
/// Closes the readdir handle.
@@ -108,7 +108,7 @@ pub trait Metablock: Send + Sync {
108108
async fn unlink(&self, parent_ino: InodeNo, name: &OsStr) -> Result<(), InodeError>;
109109
}
110110

111-
/// A callback function used to pass information to the filesystem.
111+
/// Callback to the file system which adds directory entries to the reply buffer.
112112
///
113113
/// # Parameters (in order)
114114
///
@@ -119,13 +119,23 @@ pub trait Metablock: Send + Sync {
119119
///
120120
/// # Returns
121121
///
122-
/// Returns `true` if the entry was successfully used
122+
/// - [AddDirEntryResult::EntryAdded] if the entry was added, or
123+
/// - [AddDirEntryResult::ReplyBufferFull] if the reply buffer was full.
123124
///
124125
///
125126
/// [^1]: The generation number is used to ensure uniqueness of inode/generation pairs.
126127
/// If the file system were exported over NFS, these pairs would need to be unique.
127128
/// For more information, see the [libfuse documentation](https://github.com/libfuse/libfuse/blob/fc1c8da0cf8a18d222cb1feed0057ba44ea4d18f/include/fuse_lowlevel.h#L70).
128-
pub type TryAddDirEntry<'r> = Box<dyn FnMut(InodeInformation, OsString, i64, u64) -> bool + Send + Sync + 'r>;
129+
pub type AddDirEntry<'r> = Box<dyn FnMut(InodeInformation, OsString, i64, u64) -> AddDirEntryResult + Send + Sync + 'r>;
130+
131+
/// Result of a call to `AddDirEntry`.
132+
#[derive(Debug, PartialEq, Eq)]
133+
pub enum AddDirEntryResult {
134+
/// The entry was added successfully.
135+
EntryAdded,
136+
/// The entry was not added because the reply buffer was full.
137+
ReplyBufferFull,
138+
}
129139

130140
#[derive(Debug, Default)]
131141
pub struct WriteMode {

‎mountpoint-s3-fs/src/superblock.rs‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,10 @@ use tracing::{debug, error, trace, warn};
4141

4242
use crate::fs::{CacheConfig, FUSE_ROOT_INODE};
4343
use crate::logging;
44-
use crate::metablock::Metablock;
45-
use crate::metablock::{InodeError, InodeKind, InodeNo, InodeStat, WriteMode};
46-
use crate::metablock::{InodeInformation, Lookup};
47-
use crate::metablock::{S3Location, TryAddDirEntry};
48-
use crate::metablock::{ValidKey, ValidName};
44+
use crate::metablock::{
45+
AddDirEntry, AddDirEntryResult, InodeError, InodeInformation, InodeKind, InodeNo, InodeStat, Lookup, Metablock,
46+
S3Location, ValidKey, ValidName, WriteMode,
47+
};
4948
use crate::s3::{S3Path, S3Personality};
5049
use crate::sync::atomic::{AtomicU64, Ordering};
5150
use crate::sync::{Arc, RwLock};
@@ -850,7 +849,7 @@ impl<OC: ObjectClient + Send + Sync + Clone> Metablock for Superblock<OC> {
850849
fh: u64,
851850
offset: i64,
852851
is_readdirplus: bool,
853-
mut try_add: TryAddDirEntry<'a>,
852+
mut add: AddDirEntry<'a>,
854853
) -> Result<(), InodeError> {
855854
let dir_handle = {
856855
let dir_handles = self.inner.dir_handles.read().unwrap();
@@ -893,12 +892,13 @@ impl<OC: ObjectClient + Send + Sync + Clone> Metablock for Superblock<OC> {
893892
if (last_offset..last_offset + entries.len()).contains(&offset) {
894893
trace!(offset, "repeating readdir response");
895894
for entry in entries[offset - last_offset..].iter() {
896-
if try_add(
895+
if add(
897896
entry.lookup.clone().into(),
898897
entry.name.clone(),
899898
entry.offset,
900899
entry.generation,
901-
) {
900+
) == AddDirEntryResult::ReplyBufferFull
901+
{
902902
break;
903903
}
904904
// We are returning this result a second time, so the contract is that we
@@ -921,32 +921,29 @@ impl<OC: ObjectClient + Send + Sync + Clone> Metablock for Superblock<OC> {
921921
/// Wrap a replier to duplicate the entries and store them in `dir_handle.last_response` so
922922
/// we can re-use them if the directory handle rewinds
923923
struct Reply<'a> {
924-
try_add: TryAddDirEntry<'a>,
924+
add: AddDirEntry<'a>,
925925
entries: Vec<DirectoryEntryReaddir>,
926926
}
927927

928928
impl Reply<'_> {
929929
async fn finish(self, offset: i64, dir_handle: &DirHandle) {
930930
*dir_handle.last_response.lock().await = Some((offset, self.entries));
931931
}
932-
fn add(&mut self, entry: DirectoryEntryReaddir) -> bool {
933-
let result = (self.try_add)(
932+
fn add(&mut self, entry: DirectoryEntryReaddir) -> AddDirEntryResult {
933+
let result = (self.add)(
934934
entry.lookup.clone().into(),
935935
entry.name.clone(),
936936
entry.offset,
937937
entry.generation,
938938
);
939-
if !result {
939+
if result == AddDirEntryResult::EntryAdded {
940940
self.entries.push(entry);
941941
}
942942
result
943943
}
944944
}
945945

946-
let mut reply = Reply {
947-
try_add,
948-
entries: vec![],
949-
};
946+
let mut reply = Reply { add, entries: vec![] };
950947

951948
if dir_handle.offset() < 1 {
952949
let lookup = self.getattr_with_inode(parent, false).await?;
@@ -956,7 +953,7 @@ impl<OC: ObjectClient + Send + Sync + Clone> Metablock for Superblock<OC> {
956953
generation: 0,
957954
lookup,
958955
};
959-
if reply.add(entry) {
956+
if reply.add(entry) == AddDirEntryResult::ReplyBufferFull {
960957
reply.finish(offset, &dir_handle).await;
961958
return Ok(());
962959
}
@@ -970,7 +967,7 @@ impl<OC: ObjectClient + Send + Sync + Clone> Metablock for Superblock<OC> {
970967
generation: 0,
971968
lookup,
972969
};
973-
if reply.add(entry) {
970+
if reply.add(entry) == AddDirEntryResult::ReplyBufferFull {
974971
reply.finish(offset, &dir_handle).await;
975972
return Ok(());
976973
}
@@ -993,7 +990,7 @@ impl<OC: ObjectClient + Send + Sync + Clone> Metablock for Superblock<OC> {
993990
lookup: next.clone(),
994991
};
995992

996-
if reply.add(entry) {
993+
if reply.add(entry) == AddDirEntryResult::ReplyBufferFull {
997994
readdir_handle.readd(next);
998995
reply.finish(offset, &dir_handle).await;
999996
return Ok(());
@@ -1797,6 +1794,7 @@ mod tests {
17971794
use time::{Duration, OffsetDateTime};
17981795

17991796
use crate::fs::{FUSE_ROOT_INODE, TimeToLive, ToErrno};
1797+
use crate::metablock::AddDirEntryResult;
18001798
use crate::s3::{Bucket, Prefix};
18011799

18021800
use super::*;
@@ -2196,7 +2194,7 @@ mod tests {
21962194
if name != OsStr::new(".") && name != OsStr::new("..") {
21972195
entries.push((entry, name.to_owned()));
21982196
}
2199-
false // Continue collecting entries
2197+
AddDirEntryResult::EntryAdded // Continue collecting entries
22002198
}),
22012199
)
22022200
.await
@@ -2382,7 +2380,7 @@ mod tests {
23822380
0,
23832381
true,
23842382
Box::new(|_entry, _name, _offset, _generation| {
2385-
false // Continue collecting entries
2383+
AddDirEntryResult::EntryAdded // Continue collecting entries
23862384
}),
23872385
)
23882386
.await

‎mountpoint-s3-fs/src/superblock/readdir.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -645,7 +645,7 @@ impl DirHandle {
645645
#[cfg(test)]
646646
mod tests {
647647
use crate::fs::FUSE_ROOT_INODE;
648-
use crate::metablock::{InodeKind, Metablock};
648+
use crate::metablock::{AddDirEntryResult, InodeKind, Metablock};
649649
use crate::s3::{Bucket, S3Path};
650650
use crate::superblock::Superblock;
651651
use crate::sync::Arc;
@@ -690,7 +690,13 @@ mod tests {
690690
.expect("Finish writing failed");
691691

692692
superblock
693-
.readdir(FUSE_ROOT_INODE, handle_id, 0, false, Box::new(|_, _, _, _| false))
693+
.readdir(
694+
FUSE_ROOT_INODE,
695+
handle_id,
696+
0,
697+
false,
698+
Box::new(|_, _, _, _| AddDirEntryResult::EntryAdded),
699+
)
694700
.await
695701
.expect("Readdir failed");
696702
}

0 commit comments

Comments
 (0)