Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

180 changes: 156 additions & 24 deletions storage/overlaybd/src/lsmt/file/helper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::io::{self, ErrorKind};
use std::mem::size_of;
use std::os::unix::fs::{FileExt, OpenOptionsExt};
use std::path::{Path, PathBuf};
use std::sync::{Arc, OnceLock, Weak};
use std::sync::{Arc, Mutex as StdMutex, OnceLock, Weak};
use std::time::{SystemTime, UNIX_EPOCH};
use tokio::sync::{Mutex, OwnedMutexGuard};
use uuid::Uuid;
Expand Down Expand Up @@ -472,50 +472,157 @@ pub(super) fn decode_premerged_index_artifact(
)))
}

/// Growth in artifact bytes that amortizes one full-dir prune scan.
const PREMERGED_INDEX_PRUNE_SCAN_FRACTION: u64 = 8;

/// Per-cache-dir growth accounting and scan serialization.
pub(super) struct PremergedIndexPruneState {
account: StdMutex<PremergedIndexScanAccount>,
}

struct PremergedIndexScanAccount {
bytes_since_scan: u64,
scan_in_progress: bool,
}

pub(super) async fn premerged_index_prune_state(cache_dir: &Path) -> Arc<PremergedIndexPruneState> {
static STATES: OnceLock<Mutex<HashMap<PathBuf, Arc<PremergedIndexPruneState>>>> =
OnceLock::new();
Comment on lines +489 to +490

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
This process-global registry retains a strong Arc for every distinct raw PathBuf indefinitely. A long-running daemon that receives many cache-directory paths can therefore grow this map without bound; equivalent relative/absolute or symlinked paths also get separate states and can scan the same physical directory concurrently. Canonicalize the key and use weak references or an eviction/removal policy if the set of cache directories is not strictly fixed.

STATES
.get_or_init(|| Mutex::new(HashMap::new()))
.lock()
.await
.entry(cache_dir.to_path_buf())
.or_insert_with(|| {
Arc::new(PremergedIndexPruneState {
account: StdMutex::new(PremergedIndexScanAccount {
bytes_since_scan: 0,
scan_in_progress: false,
Comment on lines +499 to +500

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
The accounting starts at zero and only grows when this process successfully writes an artifact; it never inspects an existing directory. Because the cache directory persists across restarts and max_dir_bytes can change, a pre-populated or already-oversized cache will not be pruned until another max_dir_bytes / 8 bytes are written. With low write activity, this can leave the cache over its configured limit indefinitely, whereas the previous implementation checked after every successful write. Consider initializing the state with an initial scan/election (or otherwise ensuring the first use prunes an existing over-limit directory).

}),
})
})
.clone()
Comment on lines +495 to +504

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
This process-wide map retains a strong Arc and copied PathBuf for every cache directory ever passed to the public cache-opening API. Callers using transient/per-image directories will grow STATES for the lifetime of the process, even after all writes and scans finish. Store Weak<PremergedIndexPruneState> values and remove/recreate stale entries, as the digest-lock registry already does, to bound registry lifetime.

}

impl PremergedIndexPruneState {
/// Charge `written_bytes` and elect the single caller that runs the next scan.
pub(super) fn elect_scan(&self, written_bytes: u64, max_dir_bytes: u64) -> bool {
let threshold = (max_dir_bytes / PREMERGED_INDEX_PRUNE_SCAN_FRACTION).max(1);
let mut account = self.account.lock().unwrap_or_else(|err| err.into_inner());
account.bytes_since_scan = account.bytes_since_scan.saturating_add(written_bytes);
if account.bytes_since_scan >= threshold && !account.scan_in_progress {
account.bytes_since_scan = 0;
account.scan_in_progress = true;
return true;
}
Comment on lines +512 to +517

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
A newly created process state starts at zero, so this is no longer guaranteed to inspect an already oversized on-disk cache on the first write. After a restart, cache-limit reduction, or earlier failed cleanup, pruning is deferred until new artifacts total max_dir_bytes / 8 (at least 8 MiB with the production minimum), even though the directory may already exceed max_dir_bytes. There is no startup/maintenance prune elsewhere; initialize a new directory state so its first successful write elects a scan, or otherwise perform an initial size check.

false
}

/// End the scan; returns true when growth charged while it ran crosses
/// the trigger and a follow-up scan should run.
pub(super) fn scan_finished(&self, max_dir_bytes: u64) -> bool {
let threshold = (max_dir_bytes / PREMERGED_INDEX_PRUNE_SCAN_FRACTION).max(1);
let mut account = self.account.lock().unwrap_or_else(|err| err.into_inner());
account.scan_in_progress = false;
if account.bytes_since_scan >= threshold {
account.bytes_since_scan = 0;
account.scan_in_progress = true;
return true;
}
false
}

/// Release a failed scan; concurrent charges are kept so the next write
/// re-elects.
pub(super) fn scan_aborted(&self) {
let mut account = self.account.lock().unwrap_or_else(|err| err.into_inner());
account.scan_in_progress = false;
}
Comment on lines +537 to +540

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
The charge that elected the failed scan was already reset to zero in elect_scan, but this abort path restores only the gate. Therefore, when a scan fails without at least another threshold of concurrent writes, the next ordinary write does not re-elect as documented; cleanup can be postponed by another full threshold after a transient read_dir, metadata, or join failure. Preserve/restore the triggering threshold on abort (while retaining concurrent charges) so the next write retries.

Comment on lines +537 to +540

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
On an aborted scan, the charge that elected the scan was already reset in elect_scan, and this method only clears the gate. If the scan fails before any concurrent writes add credit, the next write must accumulate a full new threshold before retrying; repeated transient read/join failures can therefore keep an oversized cache unpruned. Preserve a retry-pending/threshold charge when aborting (or explicitly schedule a bounded retry) instead of discarding the election.

}

/// One blocking task for the whole scan: async `tokio::fs` would cost a
/// blocking-pool round-trip per directory entry.
async fn prune_premerged_index_dir(dir: &Path, max_dir_bytes: u64) -> Result<()> {
let dir = dir.to_path_buf();
tokio::task::spawn_blocking(move || prune_premerged_index_dir_blocking(&dir, max_dir_bytes))
.await
.context("join premerged index cache prune task")?
}

struct PremergedArtifactEntry {
path: PathBuf,
len: u64,
modified: SystemTime,
}

/// Delete oldest premerged index artifacts until `dir` fits `max_dir_bytes`.
fn prune_premerged_index_dir_blocking(dir: &Path, max_dir_bytes: u64) -> Result<()> {
let mut entries = Vec::new();
let mut total = 0u64;
let mut reader = tokio::fs::read_dir(dir).await?;

while let Some(entry) = reader.next_entry().await? {
for entry in std::fs::read_dir(dir)
.with_context(|| format!("read premerged index cache dir {}", dir.display()))?
{
let entry = entry?;
let path = entry.path();
if path.extension().and_then(|v| v.to_str()) != Some(PREMERGED_INDEX_EXT) {
continue;
}
let metadata = entry.metadata().await?;
let metadata = match entry.metadata() {
Ok(metadata) => metadata,
// Entries deleted concurrently with the scan only shrink it.
Err(err) if err.kind() == ErrorKind::NotFound => continue,
Err(err) => {
return Err(err).with_context(|| format!("stat {}", path.display()));
}
};
if !metadata.is_file() {
continue;
}
let len = metadata.len();
let modified = metadata.modified().unwrap_or(UNIX_EPOCH);
total = total.saturating_add(len);
entries.push((path, len, modified));
entries.push(PremergedArtifactEntry {
path,
len,
modified,
});
}

if total <= max_dir_bytes {
return Ok(());
}

entries.sort_by_key(|(_, _, modified): &(PathBuf, u64, SystemTime)| *modified);
for (path, len, _) in entries {
entries.sort_by_key(|entry| entry.modified);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · medium
An elected writer still awaits a full directory scan that collects every artifact and sorts all entries by modification time. With a large cache directory this is O(n) metadata plus O(n) memory and O(n log n) CPU, and the blocking-pool worker remains occupied for the entire operation, causing latency spikes and potentially delaying unrelated blocking tasks. Consider a bounded/incremental eviction strategy or an independently scheduled maintenance task if large directories are expected.

for entry in entries {
if total <= max_dir_bytes {
break;
}
match tokio::fs::remove_file(&path).await {
Ok(()) => total = total.saturating_sub(len),
match std::fs::remove_file(&entry.path) {
Ok(()) => total = total.saturating_sub(entry.len),
// Already removed by someone else since the scan: those bytes
// left the dir too, so count them as freed.
Err(err) if err.kind() == ErrorKind::NotFound => {
total = total.saturating_sub(entry.len);
}
Err(err) => {
tracing::warn!(?err, path = %path.display(), "remove premerged index artifact failed")
tracing::warn!(
?err,
path = %entry.path.display(),
"remove premerged index artifact failed"
)
}
}
}
Ok(())
}

/// Write the artifact atomically (tmp file + rename) and return its size in
/// bytes so callers can account cache growth.
async fn write_premerged_index_artifact(
cache_dir: &Path,
key: &PremergedIndexCacheKey,
index: &ReadOnlyIndex,
) -> Result<()> {
) -> Result<u64> {
let dir = cache_dir.join(PREMERGED_INDEX_DIR);
tokio::fs::create_dir_all(&dir)
.await
Expand Down Expand Up @@ -550,7 +657,7 @@ async fn write_premerged_index_artifact(
});
}

Ok(())
Ok(artifact.len() as u64)
}

pub(super) async fn try_read_premerged_index_artifact(
Expand Down Expand Up @@ -582,6 +689,34 @@ pub(super) async fn try_read_premerged_index_artifact(
}
}

/// Charge `written` artifact bytes for `cache_dir` and run prune scans
/// while the growth trigger keeps electing.
async fn prune_premerged_index_cache(cache_dir: &Path, written: u64, max_dir_bytes: u64) {
let state = premerged_index_prune_state(cache_dir).await;
if !state.elect_scan(written, max_dir_bytes) {
return;
}
let dir = cache_dir.join(PREMERGED_INDEX_DIR);
loop {
match prune_premerged_index_dir(&dir, max_dir_bytes).await {
Ok(()) => {
if !state.scan_finished(max_dir_bytes) {
return;
}
}
Err(err) => {
tracing::warn!(
?err,
path = %dir.display(),
"prune premerged index cache dir failed"
);
state.scan_aborted();
return;
}
}
}
}

pub(super) fn spawn_premerged_index_artifact_write(
cache_dir: PathBuf,
key: PremergedIndexCacheKey,
Expand All @@ -599,18 +734,15 @@ pub(super) fn spawn_premerged_index_artifact_write(
"write premerged index artifact failed"
);
}
let key_digest = key.digest_hex.clone();
// Release the merged index and the digest lock before the prune tail:
// the index can be hundreds of MB and the prune scan may pin it for
// the whole scan, while the held lock would block the next writer for
// the same digest.
drop(merged);
drop(guard);
release_premerged_index_lock(&key_digest, &lock).await;
if write_result.is_ok() {
let dir = cache_dir.join(PREMERGED_INDEX_DIR);
if let Err(err) = prune_premerged_index_dir(&dir, max_dir_bytes).await {
tracing::warn!(
?err,
path = %dir.display(),
"prune premerged index cache dir failed"
);
}
release_premerged_index_lock(&key.digest_hex, &lock).await;
if let Ok(written) = write_result {
prune_premerged_index_cache(&cache_dir, written, max_dir_bytes).await;
}
});
}
Expand Down
54 changes: 54 additions & 0 deletions storage/overlaybd/src/lsmt/file/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2536,6 +2536,60 @@ async fn test_premerged_index_lock_map_drops_idle_entries() {
release_premerged_index_lock(&stale_key, &replacement).await;
}

#[tokio::test]
async fn test_premerged_index_prune_state_is_per_dir_and_elects_one_scan() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test · low
The added test covers basic per-directory identity and single election, but not the filesystem race where an enumerated artifact disappears before remove_file, nor the interleaving between elect_scan and scan_finished that can trigger a back-to-back scan. Tests for these cases would help protect the new accounting and concurrency invariants.

let temp_dir = TempDir::new().unwrap();
let cache_a = temp_dir.path().join("cache-a");
let cache_b = temp_dir.path().join("cache-b");

let a = premerged_index_prune_state(&cache_a).await;
assert!(Arc::ptr_eq(
&a,
&premerged_index_prune_state(&cache_a).await
));
Comment on lines +2545 to +2549

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test · medium
This test only invokes elect_scan sequentially, so it does not exercise the concurrency invariant implied by the shared per-directory state: simultaneous writers could both race around scan election or charge accounting while this test still passes. Please add a concurrent test (for example, synchronized Tokio tasks issuing charges against the same Arc) and assert that exactly one caller is elected and pending growth is preserved.

Suggestion:

Suggested change
let a = premerged_index_prune_state(&cache_a).await;
assert!(Arc::ptr_eq(
&a,
&premerged_index_prune_state(&cache_a).await
));
let a = premerged_index_prune_state(&cache_a).await;
assert!(Arc::ptr_eq(
&a,
&premerged_index_prune_state(&cache_a).await
));
// Exercise election and accounting from concurrent callers here.

Comment on lines +2545 to +2549

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test · low
This only checks sequential lookups. It does not exercise concurrent first-time initialization of the global state map, so a check-then-insert implementation could still create multiple Arcs for the same directory and lose accounting while these assertions pass. Add concurrent lookups (for example, several tasks synchronized by a barrier) and assert all returned Arcs are pointer-identical.

let b = premerged_index_prune_state(&cache_b).await;
assert!(!Arc::ptr_eq(&a, &b));

// Budget 64 triggers one scan per 8 new bytes (`max_dir_bytes` / 8).
let budget = 64;
assert!(!a.elect_scan(7, budget));
Comment on lines +2553 to +2555

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · low
The assertions encode private policy details (max_dir_bytes / 8, counter reset timing, and the exact follow-up-scan state machine) rather than an externally observable pruning contract. This makes the test unnecessarily brittle: a valid change to the amortization threshold or accounting strategy would require rewriting it even if cache pruning behavior remains correct. Consider keeping this as a focused state-machine unit test only if these details are an intentional invariant; otherwise prefer a behavior-oriented test that writes artifacts and verifies the directory is bounded.

Suggestion:

Suggested change
// Budget 64 triggers one scan per 8 new bytes (`max_dir_bytes` / 8).
let budget = 64;
assert!(!a.elect_scan(7, budget));
// Verify pruning behavior through artifact writes and the resulting directory size.

// Growth charged to A leaves B below B's own trigger.
assert!(!b.elect_scan(7, budget));
// The winner's charge resets: the next scan needs a fresh trigger of
// post-election growth.
assert!(a.elect_scan(1, budget));

// Crossings while a scan runs lose the election but keep their charge:
// when they cross the trigger, scan_finished chains one follow-up scan
// instead of waiting for new writes.
assert!(!a.elect_scan(8, budget));
assert!(a.scan_finished(budget));
assert!(!a.elect_scan(1, budget));
// Below the trigger: the chain ends and the gate is released.
assert!(!a.scan_finished(budget));
assert!(a.elect_scan(8, budget));
// No growth during the scan: no follow-up.
assert!(!a.scan_finished(budget));

// A failed scan releases the gate but keeps concurrent charges.
assert!(a.elect_scan(8, budget));
assert!(!a.elect_scan(7, budget));
a.scan_aborted();
assert!(a.elect_scan(1, budget));
assert!(!a.scan_finished(budget));

// A sub-fraction budget floors the trigger at one byte instead of zero
// (which would elect a scan per write).
assert!(a.elect_scan(1, 0));
assert!(!a.scan_finished(0));

// Saturating accounting: a u64::MAX charge on a near-trigger counter
// must cross the trigger, not wrap 7 + MAX back below it.
assert!(!a.elect_scan(7, budget));
assert!(a.elect_scan(u64::MAX, budget));
assert!(!a.scan_finished(budget));
}

fn premerged_artifact_count(cache_dir: &std::path::Path) -> usize {
let dir = cache_dir.join(PREMERGED_INDEX_DIR);
match std::fs::read_dir(dir) {
Expand Down
1 change: 1 addition & 0 deletions storage/ublk-daemon/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ tokio = { version = "1.44.2", features = ["full"] }
tracing = "0.1.41"
tracing-log = "0.2.0"
reqwest = { version = "0.13", default-features = false, features = ["rustls", "json"] }
tikv-jemallocator = { version = "0.6", features = ["background_threads"] }

[dev-dependencies]
tempfile = "3"
4 changes: 4 additions & 0 deletions storage/ublk-daemon/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ use uvm_ublk_daemon::{server::UblkDaemonServer, ResizeToolSpec};

mod metrics_server;

// Mirrors src/bin/server.rs.
#[global_allocator]
static ALLOC: tikv_jemallocator::Jemalloc = tikv_jemallocator::Jemalloc;

#[derive(Debug, Parser)]
#[command(
name = "uvm-ublk-daemon",
Expand Down
Loading