-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement statfs with synthetic values #1118
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Christian Hagemeier <chagem@amazon.com>
5708c16
to
89df80a
Compare
Signed-off-by: Christian Hagemeier <chagem@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Awesome to see this working.
There are a few comments inline.
Can you add some tests for this, just to verify these values make it out of the file system? I'd recommend creating a new file in mountpoint-s3/tests/fuse_tests/
named statfs_test.rs
, and write some tests that test end-to-end. statvfs
isn't actually in the Rust standard library, but we have some example of using a crate named nix
for querying this information.
mountpoint-s3/mountpoint-s3/src/data_cache/disk_data_cache.rs
Lines 308 to 321 in 9206ed4
CacheLimit::AvailableSpace { min_ratio } => { | |
let stats = match nix::sys::statvfs::statvfs(&self.cache_directory) { | |
Ok(stats) if stats.blocks() == 0 => { | |
warn!("unable to determine available space (0 blocks reported)"); | |
return false; | |
} | |
Ok(stats) => stats, | |
Err(error) => { | |
warn!(?error, "unable to determine available space"); | |
return false; | |
} | |
}; | |
(stats.blocks_free() as f64) < min_ratio * (stats.blocks() as f64) | |
} |
mountpoint-s3/src/fs.rs
Outdated
const FREE_BLOCKS: u64 = u64::MAX / 2; | ||
const FREE_INODES: u64 = u64::MAX / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where any boundaries may lie, but I think we can take this down by 1024 at least. (In this case, I think we are fine to adjust these values in the future.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it, assuming taking down means / 1024
instead of / 2
.
Signed-off-by: Christian Hagemeier <chagem@amazon.com>
#[derive(Debug)] | ||
/// Reply to a 'statfs' call | ||
pub struct StatFs { | ||
/// Total number of blocks | ||
pub total_blocks: u64, | ||
/// Number of free blocks | ||
pub free_blocks: u64, | ||
/// Number of free blocks available to unprivileged user | ||
pub available_blocks: u64, | ||
/// Number of inodes in file system | ||
pub total_inodes: u64, | ||
/// Available inodes | ||
pub free_inodes: u64, | ||
/// Optimal transfer block size | ||
pub block_size: u32, | ||
/// Maximum name length | ||
pub maximum_name_length: u32, | ||
/// Fragement size | ||
pub fragment_size: u32, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: didn't catch this last time, but we always put Rustdoc comments above macros (so its clear what macros are applied to the struct)
#[derive(Debug)] | |
/// Reply to a 'statfs' call | |
pub struct StatFs { | |
/// Total number of blocks | |
pub total_blocks: u64, | |
/// Number of free blocks | |
pub free_blocks: u64, | |
/// Number of free blocks available to unprivileged user | |
pub available_blocks: u64, | |
/// Number of inodes in file system | |
pub total_inodes: u64, | |
/// Available inodes | |
pub free_inodes: u64, | |
/// Optimal transfer block size | |
pub block_size: u32, | |
/// Maximum name length | |
pub maximum_name_length: u32, | |
/// Fragement size | |
pub fragment_size: u32, | |
} | |
/// Reply to a 'statfs' call | |
#[derive(Debug)] | |
pub struct StatFs { | |
/// Total number of blocks | |
pub total_blocks: u64, | |
/// Number of free blocks | |
pub free_blocks: u64, | |
/// Number of free blocks available to unprivileged user | |
pub available_blocks: u64, | |
/// Number of inodes in file system | |
pub total_inodes: u64, | |
/// Available inodes | |
pub free_inodes: u64, | |
/// Optimal transfer block size | |
pub block_size: u32, | |
/// Maximum name length | |
pub maximum_name_length: u32, | |
/// Fragement size | |
pub fragment_size: u32, | |
} |
non-blocking
assert_ne!(stats.blocks(), 0); | ||
} | ||
|
||
/// Tests that default values from FUSER are reported for mpst fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is mpst
, is it typo for most
?
// These five aren't default values but set by us, so maybe drop | ||
assert_eq!(stats.blocks(), u64::MAX / 1024); | ||
assert_eq!(stats.blocks_free(), u64::MAX / 1024); | ||
assert_eq!(stats.blocks_available(), u64::MAX / 1024); | ||
assert_eq!(stats.files(), u64::MAX / 1024); | ||
assert_eq!(stats.files_available(), u64::MAX / 1024); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd drop these, but actually maybe we just combine with the test above. No need to separate them.
let test_session = creator_fn(prefix, Default::default()); | ||
let mount_dir = test_session.mount_path(); | ||
let stats = nix::sys::statvfs::statvfs(mount_dir.into()).unwrap(); | ||
//assert_eq!(stats.name_max(), 255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop commented out code
#[cfg(feature = "s3_tests")] | ||
#[test_case(""; "no prefix")] | ||
#[test_case("statfs_report_nonzero_test"; "prefix")] | ||
fn statfs_report_fuser_defaults_s3(prefix: &str) { | ||
statfs_test_available_nonzero(fuse::s3_session::new, prefix); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test case is wrong
#[cfg(feature = "s3_tests")] | |
#[test_case(""; "no prefix")] | |
#[test_case("statfs_report_nonzero_test"; "prefix")] | |
fn statfs_report_fuser_defaults_s3(prefix: &str) { | |
statfs_test_available_nonzero(fuse::s3_session::new, prefix); | |
} | |
#[cfg(feature = "s3_tests")] | |
#[test_case(""; "no prefix")] | |
#[test_case("statfs_report_fuser_defaults"; "prefix")] | |
fn statfs_report_fuser_defaults_s3(prefix: &str) { | |
statfs_report_fuser_defaults(fuse::s3_session::new, prefix); | |
} |
Description of change
This PR adds support for calling
statfs
on virtual file system created using mountpoint.Some applications depend on the filesystem reporting non-zero available space; currently mountpoint reports 0 as number of available blocks, which can cause these applications to not work as expected.
This PR (building on #871) implements statfs with synthetic values (4611686018427387904 free blocks).
For example, the DF output now is:
Thus, checks for available space should no longer fail.
Relevant issues: #710.
This change impacts existing behaviour, as Mountpoint will report non-zero value for total blocks, free blocks, free inodes and maximum file name length.
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).