feat: validate account / snapshot paths for direct-io capability#10957
Conversation
kskalski
force-pushed
the
ks/check_direcT_io
branch
2 times, most recently
from
056a53c to
0be046e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10957 +/- ##
=========================================
- Coverage 83.0% 83.0% -0.1%
=========================================
Files 835 838 +3
Lines 317172 317333 +161
=========================================
+ Hits 263429 263548 +119
- Misses 53743 53785 +42 🚀 New features to boost your workflow:
|
| }; | ||
| // statx with STATX_DIOALIGN is the preferred check, but libc does not expose | ||
| // statx on musl (requires musl >= 1.2.3), so skip it there. | ||
| #[cfg(not(target_env = "musl"))] |
There was a problem hiding this comment.
this fails CI because libc gates usage of many features by some kind of unstable enablement env var - since we build without that flag, I disabled it... I guess it's possible to get this working correctly, but needs more research.
There was a problem hiding this comment.
confirmed: this could work if build on alpine / with musl used RUST_LIBC_UNSTABLE_MUSL_V1_2_3: true, I guess we don't want to force that just for the purpose of this call, which we have a fallback for
| } | ||
| if let Some(shrink_paths) = &config.accounts_db_config.shrink_paths { | ||
| for shrink_path in shrink_paths { | ||
| move_and_async_delete_path_contents(shrink_path); |
There was a problem hiding this comment.
I looked at the code in move_and_async_delete_path_contents and it also does create path at the end of the function... Seems like that create:
- doesn't belong to the function deleting the contents and should be moved out to the call site / other helper
- in fact it seems it better fits in the new
validate_account_paths- I will refactor this in separate PR if that sounds good
|
Awesome, thanks for putting up with my madness. |
brooksprumo
left a comment
brooksprumo
left a comment
There was a problem hiding this comment.
accounts-db/validator/ledger-tool changes look good to me
| #[cfg(target_os = "linux")] | ||
| pub fn check_direct_io_capability(path: impl AsRef<Path>) -> io::Result<Option<bool>> { | ||
| let Some(file) = find_any_file_under_path(path.as_ref())? else { | ||
| return Ok(None); |
There was a problem hiding this comment.
instead of this uncertain condition, couldn't we create a file and then stat it?
There was a problem hiding this comment.
This could be an additional way to check - I think some of the passed paths could in fact be read-only (e.g. someone runs ledger-tool on some read-only view to avoid data updates) or even if they are not ideally we should not mess with the dir, so it's preferrable to first try reading.
Maybe if the dir is empty (so exactly this branch) the write check is more plausible - in that case supposedly nobody cares if we create tmp file there.
Hm.. I will add this as separate PR, as it will make the check slightly more intrusive (otherwise I hope to BP this one).
| /// Returns whether `path` (a file or directory) resides on a filesystem that supports | ||
| /// direct I/O (`O_DIRECT`). | ||
| /// | ||
| /// Returns `Ok(Some(true))` if direct I/O is supported, `Ok(Some(false))` if it is not, |
There was a problem hiding this comment.
nit: this should probably return an ad hoc enum instead of requiring the caller
to do boolean logic between Option and bool 😂
kskalski
force-pushed
the
ks/check_direcT_io
branch
from
1e72257 to
7e082d1
Compare
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
) * feat(fs): add metadata module for queryng fs capabilities * feat: validate account / snapshot paths for direct-io capability (cherry picked from commit a4b6f4f)
…y (backport of #10957) (#11122) feat: validate account / snapshot paths for direct-io capability (#10957) * feat(fs): add metadata module for queryng fs capabilities * feat: validate account / snapshot paths for direct-io capability (cherry picked from commit a4b6f4f) Co-authored-by: Kamil Skalski <kamil.skalski@gmail.com>
5 participants
Problem
Enabling direct I/O for accounts-db creation from snapshot archive (or other snapshot ops in the future) maybe cause cryptic error surfaced to the user.
We could add a bit of code to detect if direct I/O is supported for paths we expect to use it on and signal more descriptive error.
Summary of Changes
metadatamodule inagave-fsthat detects direct I/O support by trying to find any file under specified path and check it using two approaches:statx(STATX_DIOALIGN)- not that not all kernels validator could run on support this syscall, additionally, not all filesystems that do support direct I/O actually implement this syscall flag correctly (e.g. tmpfs), so it is usually not enough to use outcome fromstatxand call it a dayThis PR enables switching direct-io on for snapshot operations by default in #10790