API to acquire available locks #3871
Conversation
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
2 times, most recently
from
1aba76f to
a3240b1
Compare
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
3 times, most recently
from
11c0059 to
111b2ac
Compare
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
111b2ac to
652d5e5
Compare
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
652d5e5 to
59a6f57
Compare
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
59a6f57 to
af59887
Compare
WalkthroughAdds non-blocking "available" lock acquisition paths and guards (read_all_available / write_all_available, AllLockReadAvailableGuard / AllLockWriteAvailableGuard); exposes optional Engine methods available_pools() / available_pools_mut(); SimEngine and StratEngine implement/use them; timers/event handlers early-return when none available. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant E as Engine
participant L as AllOrSomeLock
participant G as AvailableGuard
C->>E: available_pools()
E->>L: read_all_available()
alt some available
L-->>E: Some(AllLockReadAvailableGuard)
E-->>C: Some(G.into_dyn())
note right of G `#a2d2ff`: non-blocking — iterate/inspect
else none available
L-->>E: None
E-->>C: None
end
sequenceDiagram
autonumber
participant T as Timer (StratEngine)
participant S as StratEngine
participant L as AllOrSomeLock
participant G as WriteAvailableGuard
T->>S: fs/pool event tick
S->>L: write_all_available()
alt Some available
L-->>S: Some(AllLockWriteAvailableGuard)
S->>G: convert/iterate -> SomeLockWriteGuard[]
S-->>T: process available pools
else None
L-->>S: None
S-->>T: return early (no work)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/engine/structures/lock.rs (3)
360-386: Doc correctness: “readable” vs “writable” and API intent
- Line 374: The comment for get_available_write() says “readable”; it should be “writable”.
- The functions’ behavior looks correct: you atomically determine availability while holding the mutexes and add per-element locks.
Apply this diff:
- /// Get a list of all readable pools in the current lock. + /// Get a list of all writable pools in the current lock. fn get_available_write<T>(&mut self, inner: &mut Table<U, T>) -> Table<U, *mut T> {
607-614: Doc correctness: “read” vs “write”The write_all_available() method’s doc comment says “Issue a read...”; it should say “Issue a write...”.
Apply this diff:
- /// Issue a read on all available elements. + /// Issue a write on all available elements. pub async fn write_all_available(&self) -> Option<AllLockWriteAvailableGuard<U, T>> {
1121-1127: Doc correctness: wrong guard provenanceThe doc says “Guard returned by AllRead future.” This type is returned by AllReadAvailable.
Apply this diff:
-/// Guard returned by AllRead future. +/// Guard returned by AllReadAvailable future. pub struct AllLockReadAvailableGuard<U: AsUuid, T: ?Sized>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(3 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:37:22.447Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.447Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/sim_engine/engine.rssrc/engine/engine.rssrc/engine/strat_engine/engine.rs
🧬 Code Graph Analysis (4)
src/engine/sim_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(745-747)pools_mut(210-212)pools_mut(749-751)available_pools_mut(214-218)available_pools_mut(753-755)
src/engine/engine.rs (2)
src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(745-747)pools_mut(210-212)pools_mut(749-751)available_pools_mut(214-218)available_pools_mut(753-755)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (12)
default(52-57)is_empty(174-176)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
🔇 Additional comments (14)
src/engine/structures/lock.rs (2)
577-584: LGTM: read_all_available APINon-blocking discovery returning Option is clean, and the guard semantics match the rest of the design.
1093-1120: LGTM: AllReadAvailable future should not waitNon-blocking polling that returns None if no entries are available is consistent with the API. Holding both mutexes while materializing the available set prevents races.
src/engine/engine.rs (3)
22-25: LGTM: re-exports for available guard typesPublicly exposing AllLockReadAvailableGuard and AllLockWriteAvailableGuard through structures looks correct.
512-515: API addition: available_poolsThe Option-guard API surface is clear and consistent with non-blocking semantics. No concerns.
519-522: API addition: available_pools_mutMatches the read side and integrates cleanly with the rest of the trait.
src/engine/structures/mod.rs (1)
10-13: LGTM: export available guard typesRe-export and ordering are fine; no functional changes beyond public surface exposure.
src/engine/strat_engine/engine.rs (5)
37-40: LGTM: imports include available guardsImports are complete and consistent with the new APIs.
206-209: LGTM: inherent available_poolsDirectly forwards to AllOrSomeLock::read_all_available(); matches AnyPool usage.
214-219: LGTM: inherent available_pools_mutDirectly forwards to AllOrSomeLock::write_all_available().
745-747: LGTM: Engine::available_pools implementationPattern matches existing pools() implementation; into_dyn conversion ensures dyn Pool guard.
753-755: LGTM: Engine::available_pools_mut implementationPattern matches existing pools_mut() implementation; into_dyn conversion ensures dyn Pool guard.
src/engine/sim_engine/engine.rs (3)
23-26: LGTM: imports include available guardsConsistent with the new surface.
305-308: LGTM: Engine::available_pools (sim) implementationNon-blocking mapping with into_dyn; mirrors read_all() path.
313-315: LGTM: Engine::available_pools_mut (sim) implementationNon-blocking mapping with into_dyn; mirrors write_all() path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/engine/strat_engine/engine.rs (3)
345-348: Mirror the timer improvement for filesystem events; add a log for visibilitySame rationale as pool_evented_timer: non-blocking behavior is appropriate for the timer path. Consider a debug log when the write-all lock is unavailable.
Apply this diff:
- match self.pools.write_all_available().await { - Some(g) => g.into(), - None => return HashMap::default(), - }; + if let Some(g) = self.pools.write_all_available().await { + g.into() + } else { + debug!("Skipping fs_evented_timer iteration: pools write lock unavailable"); + return HashMap::default(); + };
751-753: Avoid potential ambiguity by calling the inherent method via UFCSInside the Engine impl, calling self.available_pools() can be momentarily confusing since the trait method shares the same name. Using UFCS clarifies that the inherent method returning AnyPool is being called.
Apply this diff:
- self.available_pools().await.map(|l| l.into_dyn()) + StratEngine::available_pools(self).await.map(|l| l.into_dyn())
759-761: Same UFCS clarification for the mutable available accessorConsistent with the read variant, prefer UFCS to clearly target the inherent method.
Apply this diff:
- self.available_pools_mut().await.map(|l| l.into_dyn()) + StratEngine::available_pools_mut(self).await.map(|l| l.into_dyn())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/engine/strat_engine/engine.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:37:22.447Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.447Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/strat_engine/engine.rs
🧬 Code Graph Analysis (1)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: stratis-cli-checks
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
🔇 Additional comments (4)
src/engine/strat_engine/engine.rs (4)
37-40: Imports: availability-aware guards are correctly exposed for useThe addition of AllLockReadAvailableGuard and AllLockWriteAvailableGuard matches the new non-blocking API usage below and aligns with the public re-exports described.
206-208: New read-availability accessor is consistent and minimalavailable_pools() correctly mirrors pools() while respecting the non-blocking semantics via read_all_available().
214-218: New write-availability accessor is consistent and minimalavailable_pools_mut() correctly mirrors pools_mut() while respecting the non-blocking semantics via write_all_available().
311-314: Add lightweight debug log & confirm conversion existsSwitching to
write_all_available()with an early return avoids timer stalls. Consider emitting a low-level debug log to help diagnose persistent contention, and ensure that theInto<Vec<SomeLockWriteGuard<…>>>impl exists for the returned guard.File:
src/engine/strat_engine/engine.rs(lines 311–314)Apply this diff:
- match self.pools.write_all_available().await { - Some(g) => g.into(), - None => return HashMap::default(), - }; + if let Some(g) = self.pools.write_all_available().await { + g.into() + } else { + debug!("Skipping pool_evented_timer iteration: pools write lock unavailable"); + return HashMap::default(); + };Please verify:
write_all_available()returns anAllLockWriteAvailableGuard<…>- There’s an
impl From<AllLockWriteAvailableGuard<…>> for Vec<SomeLockWriteGuard<…>>(or equivalentInto<…>)- All necessary methods (
write_all_available, any conversion helpers) are correctly defined and in scope
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
2de15f5 to
bd968a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/engine/structures/lock.rs (2)
1489-1508: Critical: Into<Vec> has invalid invariants and will double-release locksTwo issues:
- Asserts assume no per-element locks are held, but this guard already holds per-element write locks. Also, verifying read_locked/write_locked emptiness is too strong; only global state should be asserted.
- Without disabling the guard’s Drop after conversion, the available guard will attempt to release locks that are now owned by the per-element guards, causing panics (remove_write_lock() on a UUID already removed) or corrupt state. Unlike reads, writes are tracked as a set, so re-adding does not refcount.
Apply this diff:
-impl<U, T> Into<Vec<SomeLockWriteGuard<U, T>>> for AllLockWriteAvailableGuard<U, T> +impl<U, T> Into<Vec<SomeLockWriteGuard<U, T>>> for AllLockWriteAvailableGuard<U, T> where U: AsUuid, { - fn into(self) -> Vec<SomeLockWriteGuard<U, T>> { + fn into(mut self) -> Vec<SomeLockWriteGuard<U, T>> { let mut lock_record = self.0.lock().expect("Mutex only locked internally"); - assert!(lock_record.read_locked.is_empty()); - assert!(lock_record.write_locked.is_empty()); - assert_eq!(lock_record.all_read_locked, 0); + // No global all-read or all-write locks should be set at this point. + assert_eq!(lock_record.all_read_locked, 0); + assert!(!lock_record.all_write_locked); + + // Transfer responsibility for unlocking to the per-element guards. + // For writes, the lock_record uses a set (no refcounting); therefore, + // the available guard must not also try to remove these on Drop. + self.2 = false; self.1 .iter() .map(|(n, u, t)| { ( *u, SomeLockWriteGuard(Arc::clone(&self.0), *u, n.clone(), *t, true), ) }) .map(|(u, guard)| { lock_record.add_write_lock(u, None); guard }) .collect::<Vec<_>>() } }This aligns invariants to global state and prevents double-removal by disabling the available guard’s Drop after handing off to per-element guards.
1618-1632: Critical: Dropping AllLockWriteAvailableGuard removes the wrong lock typeCurrently calls remove_write_all_lock(), but this guard represents per-element write locks acquired via add_write_lock(). This will panic/corrupt state and defeat the available-lock semantics.
Apply this diff:
impl<U, T> Drop for AllLockWriteAvailableGuard<U, T> where U: AsUuid, T: ?Sized, { fn drop(&mut self) { - trace!("Dropping all write lock"); + trace!("Dropping write locks for available subset"); if self.2 { let mut lock_record = self.0.lock().expect("Mutex only locked internally"); - lock_record.remove_write_all_lock(); + // Remove the per-element write locks acquired in get_available_write() + for u in self.iter().map(|(_, u, _)| *u) { + lock_record.remove_write_lock(&u); + } lock_record.wake(); } - trace!("All write lock dropped"); + trace!("Write locks for available subset dropped"); } }
🧹 Nitpick comments (5)
src/engine/structures/lock.rs (4)
360-372: Available-read acquisition logic looks sound; minor doc nitThe non-blocking discovery and per-element read-lock acquisition match the intended semantics (no waiting, no interaction with the wait queue, and respecting conflicts_with_woken/already_acquired). Nit: "pools" in the docstring is a bit misleading in this generic structure.
Apply this diff to make the comment more general:
- /// Get a list of all readable pools in the current lock. + /// Get a list of all readable entries in the current lock.
374-386: Docstring typo: should say "writable" (not "readable")This function collects writable entries; the docstring currently says "readable".
- /// Get a list of all readable pools in the current lock. + /// Get a list of all writable entries in the current lock.
607-614: Docstring says "read" but this is write_all_available()Small correction for clarity.
- /// Issue a read on all available elements. + /// Issue a write on all available elements.
1229-1245: Adjust log messages to reflect "available subset" semanticsThis guard releases per-element locks on a subset, not a global "all read" lock. Logging should reflect that to aid diagnostics.
- trace!("Dropping all read lock"); + trace!("Dropping read locks for available subset"); if self.2 { let mut lock_record = self.0.lock().expect("Mutex only locked internally"); for u in self.iter().map(|(_, u, _)| *u) { lock_record.remove_read_lock(u); } lock_record.wake(); } - trace!("All read lock dropped"); + trace!("Read locks for available subset dropped");src/engine/strat_engine/engine.rs (1)
751-761: Disambiguate inherent vs trait method calls for clarityThese wrapper methods inside the Engine impl call inherent methods with the same names. It works today, but is easy to misread as self-recursion. Prefer explicit inherent calls.
Apply this diff:
- async fn available_pools(&self) -> Option<AllLockReadAvailableGuard<PoolUuid, dyn Pool>> { - self.available_pools().await.map(|l| l.into_dyn()) + async fn available_pools(&self) -> Option<AllLockReadAvailableGuard<PoolUuid, dyn Pool>> { + StratEngine::available_pools(self).await.map(|l| l.into_dyn()) } @@ - async fn pools_mut(&self) -> AllLockWriteGuard<PoolUuid, dyn Pool> { - self.pools_mut().await.into_dyn() + async fn pools_mut(&self) -> AllLockWriteGuard<PoolUuid, dyn Pool> { + StratEngine::pools_mut(self).await.into_dyn() } @@ - async fn available_pools_mut(&self) -> Option<AllLockWriteAvailableGuard<PoolUuid, dyn Pool>> { - self.available_pools_mut().await.map(|l| l.into_dyn()) + async fn available_pools_mut(&self) -> Option<AllLockWriteAvailableGuard<PoolUuid, dyn Pool>> { + StratEngine::available_pools_mut(self).await.map(|l| l.into_dyn()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/engine/sim_engine/engine.rs
- src/engine/structures/mod.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/strat_engine/engine.rs
🧬 Code Graph Analysis (3)
src/engine/engine.rs (2)
src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(751-753)pools_mut(210-212)pools_mut(755-757)available_pools_mut(214-218)available_pools_mut(759-761)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (12)
default(52-57)is_empty(174-176)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: stratis-cli-checks
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile check-typos, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (11)
src/engine/structures/lock.rs (4)
577-584: LGTM: Non-blocking read_all_available()The API is consistent with the rest of the lock surface and properly wraps the AllReadAvailable future.
1093-1120: LGTM: AllReadAvailable future behaves correctly
- Never waits, as intended.
- Correctly respects conflicts_with_woken and already_acquired indirectly via get_available_read().
- Returns None when no entries are available.
1121-1169: LGTM: AllLockReadAvailableGuard API surface mirrors read-all guardThe guard type, Into<Vec<SomeLockReadGuard<…>>>, into_dyn(), iterator types, and Send/Sync impls are consistent with existing patterns.
1450-1476: LGTM: AllWriteAvailable future is non-blocking and correct
- Never waits; uses get_available_write() with proper conflict checks.
- Returns None when nothing is available.
src/engine/engine.rs (3)
22-25: LGTM: Publicly importing available guard typesBrings the new availability-aware guard types into the Engine module scope as expected.
512-515: LGTM: Non-blocking available_pools APIThe trait addition is clear and matches the new lock APIs. Returning Option to indicate “no available pools right now” is appropriate.
519-522: LGTM: Non-blocking available_pools_mut APISymmetric with the read variant; aligns with usage in SimEngine/StratEngine.
src/engine/strat_engine/engine.rs (4)
37-40: LGTM: Importing available guard types for StratEngineMakes the availability-aware guard types available for both public API and Engine impl wrappers.
206-219: LGTM: Public available_pools / available_pools_mut on StratEngineThese inherent methods cleanly expose the non-blocking variants and match the underlying lock API.
311-314: Timer path: Correct switch to availability-aware write guardsThis avoids blocking in the timer thread when nothing is immediately writable. Note: correctness depends on fixing the available-write guard Into/Drop semantics in lock.rs to avoid double-removal/panics.
If you apply the lock.rs fixes, this path should be safe. Without them, it'll panic under contention. No further changes required here.
345-348: Timer path: fs_evented uses availability-aware write guards appropriatelySame note as for pool_evented_timer regarding reliance on correct Into/Drop behavior of available-write guards.
No action here after lock.rs fix.
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
3 times, most recently
from
a430229 to
42f8b7d
Compare
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
42f8b7d to
90192ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/engine/structures/lock.rs (1)
1484-1507: Write-available conversion/drop balance is correctAs with read, into() adds one write lock per entry and Drop removes one, leaving each SomeLockWriteGuard with a single write lock. This fixes the earlier bug of removing a global write-all lock. Nice.
Also applies to: 1616-1632
🧹 Nitpick comments (11)
src/engine/structures/lock.rs (9)
374-386: Doc fix: “readable” -> “writable”The docstring says “readable” but this helper returns writable entries. Recommend correcting to avoid confusion.
- /// Get a list of all readable pools in the current lock. + /// Get a list of all writable pools in the current lock.
607-614: Doc fix: write_all_available() describes a readThe docstring says “Issue a read…” but this issues a write across available elements. Please correct.
- /// Issue a read on all available elements. + /// Issue a write on all available elements.
1093-1120: AllReadAvailable: “never wait” path is sound; tighten docsThe future does a single pass under the mutexes and returns immediately. Good. The guard doc, however, mentions the wrong future.
Suggested doc tweak:
-/// Guard returned by AllRead future. +/// Guard returned by AllReadAvailable future.If helpful, I can add unit tests that:
- verify None when all entries conflict or when conflicts exist in woken,
- verify subset selection with mixed conflicts,
- verify wake() is triggered on drop and counters are balanced after Into<Vec<_>>.
Do you want me to draft these?Also applies to: 1121-1127
1128-1150: Conversion/drop balance is correct; add a clarifying commentConverting an AllLockReadAvailableGuard into Vec increments per-entry read counts once, and the guard’s Drop decrements once — net result: each SomeLockReadGuard holds one read lock. Consider an inline comment to document this intentional “add then drop” pattern to prevent future regressions.
Example comment to append above the map that increments:
+ // Note: get_available_read() already added one read lock per entry. + // We add one more here so that after this guard drops (and removes one), + // each returned SomeLockReadGuard retains exactly one read lock.Also applies to: 1228-1245
1450-1476: AllWriteAvailable: implementation matches design; fix guard docThe write-available future mirrors the read path and avoids waiting; looks good. The guard doc mentions the wrong future.
-/// Guard returned by AllWrite future. +/// Guard returned by AllWriteAvailable future.Also applies to: 1477-1483
1527-1540: Align Send/Sync bounds style with other guardsOther guards use T: ?Sized + Send/Sync; here it’s T: Send and T: Sync. Functionally fine, but for consistency and clarity (T is declared ?Sized at the type), mirror the existing pattern.
-unsafe impl<U, T> Send for AllLockWriteAvailableGuard<U, T> +unsafe impl<U, T> Send for AllLockWriteAvailableGuard<U, T> where U: AsUuid + Send, - T: Send, + T: ?Sized + Send, { } -unsafe impl<U, T> Sync for AllLockWriteAvailableGuard<U, T> +unsafe impl<U, T> Sync for AllLockWriteAvailableGuard<U, T> where U: AsUuid + Sync, - T: Sync, + T: ?Sized + Sync, { }
1233-1244: Trace messages: clarify “available subset” vs “all”The drop traces still say “Dropping all … lock” for the available-guard drops. Suggest refining the messages to avoid confusion during debugging.
- trace!("Dropping all read lock"); + trace!("Dropping read locks for available subset"); ... - trace!("All read lock dropped"); + trace!("Read locks for available subset dropped");- trace!("Dropping all write lock"); + trace!("Dropping write locks for available subset"); ... - trace!("All write lock dropped"); + trace!("Write locks for available subset dropped");Also applies to: 1621-1631
374-386: Optional: assert “no waiter manipulation” for available futures (debug-only)Available paths are documented as “should never wait.” A lightweight debug assertion could validate that we didn’t enqueue a waiter while building the available set.
Example (behind debug assertions) right after computing the available table:
let available = lock_record.get_available_write(unsafe { inner.get().as_mut() }.expect("Not null")); + debug_assert!( + !lock_record.waiting.iter().any(|w| matches!(w.ty, WaitType::AllWrite | WaitType::SomeWrite(_))), + "Available write should not enqueue waiters" + );Same pattern could be used for read. Totally optional.
Also applies to: 607-614
360-372: Micro: avoid repeating the WaitType construction in filtersYou construct WaitType twice per iteration (already_acquired and conflicts_with_woken). Cache it to shave a tiny bit of work and improve readability.
- for (n, u, p) in inner.iter() { - if !self.already_acquired(&WaitType::SomeRead(*u)) - && !self.conflicts_with_woken(&WaitType::SomeRead(*u)) + for (n, u, p) in inner.iter() { + let wt = WaitType::SomeRead(*u); + if !self.already_acquired(&wt) && !self.conflicts_with_woken(&wt) { self.add_read_lock(*u, None); table.insert(n.clone(), *u, p as *const T); } }- for (n, u, p) in inner.iter_mut() { - if !self.already_acquired(&WaitType::SomeWrite(*u)) - && !self.conflicts_with_woken(&WaitType::SomeWrite(*u)) + for (n, u, p) in inner.iter_mut() { + let wt = WaitType::SomeWrite(*u); + if !self.already_acquired(&wt) && !self.conflicts_with_woken(&wt) { self.add_write_lock(*u, None); table.insert(n.clone(), *u, p as *mut T); } }Also applies to: 374-386
src/engine/strat_engine/engine.rs (2)
311-314: Timer now uses availability-aware writes; consider a debug trace on skipEarly-returning an empty map when no pools are available is fine. A low-level debug log can aid observability during event droughts or heavy contention.
- match self.pools.write_all_available().await { - Some(g) => g.into(), - None => return HashMap::default(), - }; + match self.pools.write_all_available().await { + Some(g) => g.into(), + None => { + debug!("pool_evented_timer: no pools available for write at this tick"); + return HashMap::default(); + } + };
345-348: Same observability nit for fs_evented_timerMirror the optional debug trace here as well.
- match self.pools.write_all_available().await { - Some(g) => g.into(), - None => return HashMap::default(), - }; + match self.pools.write_all_available().await { + Some(g) => g.into(), + None => { + debug!("fs_evented_timer: no pools available for write at this tick"); + return HashMap::default(); + } + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/engine/engine.rs
- src/engine/structures/mod.rs
- src/engine/sim_engine/engine.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/strat_engine/engine.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile fmt-ci, 1.89.0, rustfmt)
- GitHub Check: python-based-tests
- GitHub Check: stratis-cli-checks
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile docs-ci, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.89.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (6)
src/engine/structures/lock.rs (2)
360-372: Non-blocking availability discovery looks correctThe available-read scan respects both already-acquired and woken-conflict checks and acquires per-element read locks without touching the waiter queue. Matches the “should never wait” contract.
577-584: API addition: read_all_available() aligns with semanticsThis exposes the availability surface as an Option without queuing; the “no wait” behavior is clear and appropriate for callers that should not block.
src/engine/strat_engine/engine.rs (4)
206-209: Add available_pools(): straightforward wrapperGood addition; mirrors read_all() with non-blocking semantics and returns Option as expected.
214-218: Add available_pools_mut(): straightforward wrapperMatches write_all_available() behavior and keeps the Option surface to signal “none available.”
751-754: Engine::available_pools dyn wrapper is correctMapping to dyn via into_dyn() yields the expected trait-object guard. LGTM.
759-762: Engine::available_pools_mut dyn wrapper is correctSymmetric to the read side; no issues spotted.
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
90192ee to
824de39
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/engine/structures/lock.rs (3)
607-613: Fix incorrect documentation comment.This is a duplicate of a previous review comment, but the documentation still says "read" when it should say "write".
Apply this diff:
- /// Issue a read on all available elements. + /// Issue a write on all available elements. pub async fn write_all_available(&self) -> Option<AllLockWriteAvailableGuard<U, T>> {
1121-1126: Fix incorrect documentation comment.This is identified as a duplicate of a previous review comment.
Apply this diff:
-/// Guard returned by AllRead future. +/// Guard returned by AllReadAvailable future. pub struct AllLockReadAvailableGuard<U: AsUuid, T: ?Sized>(
1477-1482: Fix incorrect documentation comment.This is identified as a duplicate of a previous review comment.
Apply this diff:
-/// Guard returned by AllWrite future. +/// Guard returned by AllWriteAvailable future. pub struct AllLockWriteAvailableGuard<U: AsUuid, T: ?Sized>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/engine/structures/mod.rs
- src/engine/strat_engine/engine.rs
- src/engine/engine.rs
- src/engine/sim_engine/engine.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (11)
default(52-57)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
🔇 Additional comments (5)
src/engine/structures/lock.rs (5)
360-372: LGTM! Well-structured available read discovery method.The logic correctly filters pools to only include those that are not already acquired or conflicted with woken requests, and properly acquires per-element read locks for the available subset.
1228-1244: LGTM! Correct Drop implementation for available read guards.The Drop implementation correctly removes per-element read locks (not a global read-all lock) and properly wakes waiting tasks. This addresses the bug from previous reviews.
1616-1632: LGTM! Correct Drop implementation for available write guards.The Drop implementation correctly removes per-element write locks (not a global write-all lock) and properly wakes waiting tasks. This also addresses the bug from previous reviews.
1128-1150: Verify the Into implementation invariants.The assertion
assert!(!lock_record.all_write_locked);is correct since available guards shouldn't conflict with global write locks. However, the absence of checks forread_lockedandwrite_lockedmaps suggests this was addressed from previous reviews.
1484-1507: LGTM! Correct Into implementation for write available guards.The invariants only check global locks (
all_write_lockedandall_read_locked) which is appropriate since per-element locks can coexist with available subsets.
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
824de39 to
a29f6b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/engine/structures/lock.rs (4)
374-386: Fix incorrect doc comment for get_available_writeThis helper enumerates writable entries and acquires per-element write locks; the doc says “readable”.
Apply:
- /// Get a list of all readable pools in the current lock. + /// Get a list of all writable pools in the current lock. fn get_available_write<T>(&mut self, inner: &mut Table<U, T>) -> Table<U, *mut T> {
607-614: Fix incorrect doc comment for write_all_availableDoc says “read on all available elements”; this function acquires write locks.
Apply:
- /// Issue a read on all available elements. + /// Issue a write on all available elements. pub async fn write_all_available(&self) -> Option<AllLockWriteAvailableGuard<U, T>> {
1093-1119: Document AllReadAvailable future semanticsClarify non-blocking behavior and return value.
Apply:
-/// Future returned by AllOrSomeLock::read_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::read_all_available(). +/// +/// Never waits: polls lock state and immediately returns +/// - Some(AllLockReadAvailableGuard) if at least one entry can be read now, acquiring +/// per-element read locks for those entries; or +/// - None if no entries are currently readable without waiting. struct AllReadAvailable<U: AsUuid, T>(AllOrSomeLock<U, T>);
1449-1475: Document AllWriteAvailable future semanticsMirror the read variant’s explanation.
Apply:
-/// Future returned by AllOrSomeLock::write_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::write_all_available(). +/// +/// Never waits: polls lock state and immediately returns +/// - Some(AllLockWriteAvailableGuard) if at least one entry can be written now, acquiring +/// per-element write locks for those entries; or +/// - None if no entries are currently writable without waiting. struct AllWriteAvailable<U: AsUuid, T>(AllOrSomeLock<U, T>);
🧹 Nitpick comments (6)
src/engine/structures/lock.rs (6)
577-584: Tighten trace logs to reflect Option resultLogs currently claim locks were acquired even when returning None.
Apply:
pub async fn read_all_available(&self) -> Option<AllLockReadAvailableGuard<U, T>> { trace!("Acquiring read lock on all available pools"); - let guard = AllReadAvailable(self.clone()).await; - trace!("All read lock on all available pools acquired"); + let guard = AllReadAvailable(self.clone()).await; + if guard.is_some() { + trace!("Acquired read locks on available pools"); + } else { + trace!("No pools currently available for read"); + } guard } @@ pub async fn write_all_available(&self) -> Option<AllLockWriteAvailableGuard<U, T>> { trace!("Acquiring write lock on all available pools"); - let guard = AllWriteAvailable(self.clone()).await; - trace!("All write lock on all available pools acquired"); + let guard = AllWriteAvailable(self.clone()).await; + if guard.is_some() { + trace!("Acquired write locks on available pools"); + } else { + trace!("No pools currently available for write"); + } guard }Also applies to: 607-614
1121-1127: Fix guard doc to reference AllReadAvailableThe guard is produced by the available variant, not AllRead.
Apply:
-/// Guard returned by AllRead future. +/// Guard returned by the AllReadAvailable future. pub struct AllLockReadAvailableGuard<U: AsUuid, T: ?Sized>(
1229-1243: Make drop logs specific to available-subset guardsAvoid “all read/write lock” phrasing which implies global locks.
Apply:
- trace!("Dropping all read lock"); + trace!("Dropping read locks for available subset"); @@ - trace!("All read lock dropped"); + trace!("Read locks for available subset dropped");- trace!("Dropping all write lock"); + trace!("Dropping write locks for available subset"); @@ - trace!("All write lock dropped"); + trace!("Write locks for available subset dropped");Also applies to: 1616-1632
1128-1150: Optional: avoid double-counting when converting available read guard into per-element guardsCurrent implementation increments each per-element read lock again and relies on this guard’s Drop to decrement once. It’s correct but harder to reason about. Transfer ownership instead.
Apply:
-impl<U, T> Into<Vec<SomeLockReadGuard<U, T>>> for AllLockReadAvailableGuard<U, T> +impl<U, T> Into<Vec<SomeLockReadGuard<U, T>>> for AllLockReadAvailableGuard<U, T> where U: AsUuid, { - fn into(self) -> Vec<SomeLockReadGuard<U, T>> { - let mut lock_record = self.0.lock().expect("Mutex only acquired internally"); - assert!(!lock_record.all_write_locked); - - self.1 - .iter() - .map(|(n, u, t)| { - ( - *u, - SomeLockReadGuard(Arc::clone(&self.0), *u, n.clone(), *t as *const _, true), - ) - }) - .map(|(u, guard)| { - lock_record.add_read_lock(u, None); - guard - }) - .collect::<Vec<_>>() - } + fn into(mut self) -> Vec<SomeLockReadGuard<U, T>> { + // Transfer existing per-element read locks to individual guards. + self.2 = false; // prevent this guard's Drop from removing the locks + self.1 + .iter() + .map(|(n, u, t)| { + SomeLockReadGuard(Arc::clone(&self.0), *u, n.clone(), *t as *const _, true) + }) + .collect() + } }
1477-1482: Fix guard doc to reference AllWriteAvailableAligns with the producing future.
Apply:
-/// Guard returned by AllWrite future. +/// Guard returned by the AllWriteAvailable future. pub struct AllLockWriteAvailableGuard<U: AsUuid, T: ?Sized>(
1484-1507: Optional: avoid double-counting when converting available write guard into per-element guardsSame pattern as read: currently increments each write lock again and relies on Drop to subtract once.
Apply:
impl<U, T> Into<Vec<SomeLockWriteGuard<U, T>>> for AllLockWriteAvailableGuard<U, T> where U: AsUuid, { - fn into(self) -> Vec<SomeLockWriteGuard<U, T>> { - let mut lock_record = self.0.lock().expect("Mutex only locked internally"); - assert!(!lock_record.all_write_locked); - assert_eq!(lock_record.all_read_locked, 0); - - self.1 - .iter() - .map(|(n, u, t)| { - ( - *u, - SomeLockWriteGuard(Arc::clone(&self.0), *u, n.clone(), *t, true), - ) - }) - .map(|(u, guard)| { - lock_record.add_write_lock(u, None); - guard - }) - .collect::<Vec<_>>() - } + fn into(mut self) -> Vec<SomeLockWriteGuard<U, T>> { + // Transfer existing per-element write locks to individual guards. + self.2 = false; // prevent this guard's Drop from removing the locks + self.1 + .iter() + .map(|(n, u, t)| SomeLockWriteGuard(Arc::clone(&self.0), *u, n.clone(), *t, true)) + .collect() + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/engine/engine.rs
- src/engine/structures/mod.rs
- src/engine/strat_engine/engine.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3898
File: src/engine/engine.rs:533-541
Timestamp: 2025-08-28T19:38:49.745Z
Learning: In the lock implementation in src/engine/structures/lock.rs, the upgrade_pool operation gets priority over other queued lock operations because it passes AtomicBool::new(true) to add_waiter(), which causes the waiter to be added to the front of the queue via push_front() rather than push_back() like other operations.
📚 Learning: 2025-08-28T19:38:49.745Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3898
File: src/engine/engine.rs:533-541
Timestamp: 2025-08-28T19:38:49.745Z
Learning: In the lock implementation in src/engine/structures/lock.rs, the upgrade_pool operation gets priority over other queued lock operations because it passes AtomicBool::new(true) to add_waiter(), which causes the waiter to be added to the front of the queue via push_front() rather than push_back() like other operations.
Applied to files:
src/engine/sim_engine/engine.rs
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/sim_engine/engine.rssrc/engine/structures/lock.rs
🧬 Code graph analysis (2)
src/engine/sim_engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(751-753)pools_mut(210-212)pools_mut(755-757)available_pools_mut(214-218)available_pools_mut(759-761)src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (11)
default(52-57)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (5)
src/engine/sim_engine/engine.rs (3)
23-26: Imports extended correctly for available guards — LGTMThe new available guard types are imported alongside existing ones. No concerns.
305-307: available_pools wiring looks correct — LGTMDelegates to read_all_available() and converts to dyn via into_dyn(). Matches Engine trait.
313-315: available_pools_mut wiring looks correct — LGTMDelegates to write_all_available() and converts to dyn via into_dyn(). Matches Engine trait.
src/engine/structures/lock.rs (2)
360-372: get_available_read helper — LGTMReads are filtered against current acquisitions and woken conflicts; per-element locks are recorded. Looks correct.
922-922: Doc typo fix (read_all) — LGTMReference now correctly points to read_all().
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
3 times, most recently
from
68b1ff0 to
5ec3f32
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/engine/sim_engine/engine.rs (1)
313-315: available_pools_mut implementation — LGTM; same verification appliesMirrors the read variant.
src/engine/structures/lock.rs (3)
607-613: Fix copy/paste in doc: this issues a write, not a readDoc should say “write”; also mirror the read-available wording.
- /// Issue a read on all available elements. + /// Issue a write on all available elements. + /// + /// Returns `None` if no elements are currently available; never waits.
1093-1119: Document the AllReadAvailable future behaviorExpand beyond “Should never wait” to describe readiness and return value.
-/// Future returned by AllOrSomeLock::read_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::read_all_available(). +/// +/// This future never waits: it polls synchronously and returns immediately. +/// Returns `None` when no elements are currently available for reading, +/// otherwise returns a guard containing the available subset. struct AllReadAvailable<U: AsUuid, T>(AllOrSomeLock<U, T>);
1454-1475: Document the AllWriteAvailable future behaviorMatch the read-available docs.
-/// Future returned by AllOrSomeLock::write_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::write_all_available(). +/// +/// This future never waits: it polls synchronously and returns immediately. +/// Returns `None` when no elements are currently available for writing, +/// otherwise returns a guard containing the available subset. struct AllWriteAvailable<U: AsUuid, T>(AllOrSomeLock<U, T>);
🧹 Nitpick comments (14)
src/engine/engine.rs (2)
512-515: Clarify Option semantics in docsSpell out when None vs Some is returned to avoid ambiguity for callers.
- /// Get all pools belonging to this engine that are currently available and will not block on - /// locking. + /// Try to acquire non-blocking read access to currently available pools. + /// Returns Some(...) if at least one pool could be locked immediately; None otherwise.
519-522: Clarify Option semantics in docs (mutable variant)Mirror the read variant’s clarification.
- /// Get mutable references to all pools belonging to this engine that are currently available - /// and will not block on locking. + /// Try to acquire non-blocking write access to currently available pools. + /// Returns Some(...) if at least one pool could be locked immediately; None otherwise.src/engine/strat_engine/engine.rs (4)
311-314: Timer: add trace on skipped iteration when no locks availableHelps diagnose timer starvation without spamming logs.
- match self.pools.write_all_available().await { - Some(g) => g.into(), - None => return HashMap::default(), - }; + match self.pools.write_all_available().await { + Some(g) => g.into(), + None => { + trace!("No pools available for write lock; skipping pool_evented_timer cycle"); + return HashMap::default(); + } + };
345-348: Timer: add trace on skipped iteration when no locks available (fs variant)Same rationale as pool timer.
- match self.pools.write_all_available().await { - Some(g) => g.into(), - None => return HashMap::default(), - }; + match self.pools.write_all_available().await { + Some(g) => g.into(), + None => { + trace!("No pools available for write lock; skipping fs_evented_timer cycle"); + return HashMap::default(); + } + };
751-753: Disambiguate inherent vs trait method to avoid perceived recursionFully qualify to make it obvious this calls the inherent helper, not the trait method.
- self.available_pools().await.map(|l| l.into_dyn()) + StratEngine::available_pools(self).await.map(|l| l.into_dyn())
759-761: Disambiguate inherent vs trait method (mutable variant)Same readability improvement as above.
- self.available_pools_mut().await.map(|l| l.into_dyn()) + StratEngine::available_pools_mut(self).await.map(|l| l.into_dyn())src/engine/structures/lock.rs (8)
360-372: Doc: clarify that locks are acquiredThe helper acquires per-element read locks; make that explicit in the doc.
- /// Get a list of all readable pools in the current lock. + /// Get a list of all pools currently available for reading and acquire read locks for them.
374-386: Doc: “writable” spelling and semanticsPrefer “writable” and note that write locks are acquired.
- /// Get a list of all writeable pools in the current lock. + /// Get a list of all pools currently available for writing and acquire write locks for them.
577-583: API doc: mention return None and never-wait semanticsMake the behavior explicit.
- /// Issue a read on all available elements. + /// Issue a read on all available elements. + /// + /// Returns `None` if no elements are currently available; never waits.
1121-1127: Correct guard doc: it’s for AllReadAvailableUpdate the doc to reference the right future and RAII behavior.
-/// Guard returned by AllRead future. +/// Guard returned by the `AllReadAvailable` future (RAII; releases per-element +/// read locks for the available subset on drop).
1128-1150: Simplify lock transfer; avoid extra add/remove cycleMirror the write-available path: set
self.2 = falseand do not increment per-element read counts again.-impl<U, T> Into<Vec<SomeLockReadGuard<U, T>>> for AllLockReadAvailableGuard<U, T> +impl<U, T> Into<Vec<SomeLockReadGuard<U, T>>> for AllLockReadAvailableGuard<U, T> where U: AsUuid, { - fn into(self) -> Vec<SomeLockReadGuard<U, T>> { - let mut lock_record = self.0.lock().expect("Mutex only acquired internally"); - assert!(!lock_record.all_write_locked); - - self.1 - .iter() - .map(|(n, u, t)| { - ( - *u, - SomeLockReadGuard(Arc::clone(&self.0), *u, n.clone(), *t as *const _, true), - ) - }) - .map(|(u, guard)| { - lock_record.add_read_lock(u, None); - guard - }) - .collect::<Vec<_>>() - } + fn into(mut self) -> Vec<SomeLockReadGuard<U, T>> { + let lock_record = self.0.lock().expect("Mutex only acquired internally"); + assert!(!lock_record.all_write_locked); + drop(lock_record); + + // Transfer ownership of the existing per-element read locks to the new guards. + self.2 = false; + self.1 + .iter() + .map(|(n, u, t)| SomeLockReadGuard(Arc::clone(&self.0), *u, n.clone(), *t, true)) + .collect::<Vec<_>>() + } }
1228-1244: Trace messages: clarify “available subset”These drops release per-element read locks, not a global “all read” lock.
- trace!("Dropping all read lock"); + trace!("Dropping read locks for available subset"); ... - trace!("All read lock dropped"); + trace!("Read locks for available subset dropped");
1477-1483: Correct guard doc: it’s for AllWriteAvailableUpdate to the correct origin and RAII note.
-/// Guard returned by AllWrite future. +/// Guard returned by the `AllWriteAvailable` future (RAII; releases per-element +/// write locks for the available subset on drop).
1609-1625: Trace messages: clarify “available subset”Align with write-available semantics.
- trace!("Dropping all write lock"); + trace!("Dropping write locks for available subset"); ... - trace!("All write lock dropped"); + trace!("Write locks for available subset dropped");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/sim_engine/engine.rssrc/engine/strat_engine/engine.rssrc/engine/structures/lock.rs
📚 Learning: 2025-08-28T19:38:49.745Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3898
File: src/engine/engine.rs:533-541
Timestamp: 2025-08-28T19:38:49.745Z
Learning: In the lock implementation in src/engine/structures/lock.rs, the upgrade_pool operation gets priority over other queued lock operations because it passes AtomicBool::new(true) to add_waiter(), which causes the waiter to be added to the front of the queue via push_front() rather than push_back() like other operations.
Applied to files:
src/engine/strat_engine/engine.rs
🧬 Code graph analysis (4)
src/engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(751-753)pools_mut(210-212)pools_mut(755-757)available_pools_mut(214-218)available_pools_mut(759-761)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/sim_engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(751-753)pools_mut(210-212)pools_mut(755-757)available_pools_mut(214-218)available_pools_mut(759-761)src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (11)
default(52-57)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: stratis-cli-checks
- GitHub Check: stratis-min-cli-checks
- GitHub Check: checks (make -f Makefile fmt-ci, 1.89.0, rustfmt)
- GitHub Check: python-based-tests
- GitHub Check: checks (make -f Makefile check-typos, 1.89.0, cargo)
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (8)
src/engine/structures/mod.rs (1)
10-12: Public re-exports for Available guards — LGTMRe-exports look correct and match downstream usage in engine/sim_engine/strat_engine.
src/engine/engine.rs (1)
22-25: Importing Available guard types — LGTMImports align with the new non-blocking API.
src/engine/sim_engine/engine.rs (2)
23-26: Importing Available guard types — LGTMConsistent with the trait changes.
305-307: available_pools implementation — LGTM; into_dyn on available guards verifiedinto_dyn impls exist for AllLockReadAvailableGuard (src/engine/structures/lock.rs:1157) and AllLockWriteAvailableGuard (src/engine/structures/lock.rs:1507); callers match the blocking variant.
src/engine/strat_engine/engine.rs (3)
37-40: Importing Available guard types — LGTMMatches usage throughout this file.
206-208: Non-blocking read accessor (inherent) — LGTMSimple forwarding to read_all_available().
214-218: Non-blocking write accessor (inherent) — LGTMSimple forwarding to write_all_available().
src/engine/structures/lock.rs (1)
1109-1111: No change required — Table::is_empty() exists.Defined at src/engine/structures/table.rs:174 (returns self.items.is_empty()); leave the calls in src/engine/structures/lock.rs (1109–1111 and 1465–1466) unchanged.
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
5ec3f32 to
9bbb291
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/engine/structures/lock.rs (1)
607-613: Doc mismatch: says “read” but this is the write path.Fix the method doc to avoid confusion. (This mirrors earlier feedback.)
- /// Issue a read on all available elements. + /// Issue a write on all available elements.
🧹 Nitpick comments (7)
src/engine/structures/lock.rs (7)
374-386: Spelling: “writable” (not “writeable”).Update the doc comment for clarity.
- /// Get a list of all writeable pools in the current lock. + /// Get a list of all writable pools in the current lock.
1093-1119: Document the “never wait” behavior and return semantics.Expand the future’s docs to state it polls immediately, never registers a waker, and returns None when no pools are available.
-/// Future returned by AllOrSomeLock::read_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::read_all_available(). +/// +/// This future never waits: it polls the lock state once and immediately +/// returns `Poll::Ready`. It does not register a waker. +/// Returns `Some(AllLockReadAvailableGuard)` if at least one pool is +/// currently readable without blocking; otherwise returns `None`.
1121-1127: Incorrect guard doc header.This guard is returned by AllReadAvailable, not AllRead.
-/// Guard returned by AllRead future. +/// Guard returned by the `AllReadAvailable` future.
1228-1244: Trace messages are misleading for “available” guard.These are per-element read locks, not an “all read lock”.
- trace!("Dropping all read lock"); + trace!("Dropping read locks for available subset"); ... - trace!("All read lock dropped"); + trace!("Read locks for available subset dropped");
1449-1475: Add analogous docs for write-available future.Mirror the read variant’s details.
-/// Future returned by AllOrSomeLock::write_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::write_all_available(). +/// +/// This future never waits: it polls the lock state once and immediately +/// returns `Poll::Ready`. It does not register a waker. +/// Returns `Some(AllLockWriteAvailableGuard)` if at least one pool is +/// currently writable without blocking; otherwise returns `None`.
1477-1483: Incorrect guard doc header.This is returned by AllWriteAvailable, not AllWrite.
-/// Guard returned by AllWrite future. +/// Guard returned by the `AllWriteAvailable` future.
1609-1625: Trace messages are misleading for “available” write guard.Use wording consistent with per-element lock release.
- trace!("Dropping all write lock"); + trace!("Dropping write locks for available subset"); ... - trace!("All write lock dropped"); + trace!("Write locks for available subset dropped");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/structures/mod.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3898
File: src/engine/engine.rs:533-541
Timestamp: 2025-08-28T19:38:49.745Z
Learning: In the lock implementation in src/engine/structures/lock.rs, the upgrade_pool operation gets priority over other queued lock operations because it passes AtomicBool::new(true) to add_waiter(), which causes the waiter to be added to the front of the queue via push_front() rather than push_back() like other operations.
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/engine.rssrc/engine/strat_engine/engine.rssrc/engine/structures/lock.rssrc/engine/sim_engine/engine.rs
📚 Learning: 2025-08-28T19:38:49.745Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3898
File: src/engine/engine.rs:533-541
Timestamp: 2025-08-28T19:38:49.745Z
Learning: In the lock implementation in src/engine/structures/lock.rs, the upgrade_pool operation gets priority over other queued lock operations because it passes AtomicBool::new(true) to add_waiter(), which causes the waiter to be added to the front of the queue via push_front() rather than push_back() like other operations.
Applied to files:
src/engine/strat_engine/engine.rssrc/engine/sim_engine/engine.rs
🧬 Code graph analysis (4)
src/engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(759-761)pools_mut(210-212)pools_mut(763-765)available_pools_mut(214-218)available_pools_mut(767-769)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (4)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)default(47-53)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (11)
default(52-57)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
src/engine/sim_engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(759-761)pools_mut(210-212)pools_mut(763-765)available_pools_mut(214-218)available_pools_mut(767-769)src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: python-based-tests
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile stratisd-tools, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.89.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (19)
src/engine/structures/lock.rs (6)
360-372: Algorithm is correct; naming is clear.Non-blocking readable-subset discovery with conflict checks is sound.
577-583: LGTM: read_all_available() semantics.Immediate, non-waiting behavior with Option guard is appropriate.
1128-1149: LGTM: conversion preserves per-element semantics.Asserts avoid global-write conflicts; per-element read locks are transferred correctly.
1152-1168: LGTM: into_dyn() handoff.Disabling the original guard before recreating with dyn pointers is correct.
1184-1198: LGTM: iterator yields safe shared refs.Unsafe blocks are minimal and justified.
1488-1500: LGTM: write-available conversion.Global-lock assertions are correct; ownership transfer to per-element guards is sound.
src/engine/engine.rs (3)
22-25: Public API surface: imports look right.New guard types are correctly re-exported for trait signatures.
512-515: Non-blocking accessor is well-scoped.Returning Option cleanly distinguishes “no available pools” without blocking callers.
519-522: Mutable non-blocking accessor mirrors read variant.Consistent semantics; good.
src/engine/sim_engine/engine.rs (3)
23-26: Imports and type usage consistent with new API.
305-307: LGTM: forwards to read_all_available().into_dyn() handoff is correct.
313-315: LGTM: forwards to write_all_available().into_dyn() handoff is correct.
src/engine/strat_engine/engine.rs (7)
37-40: Imports align with available-guard usage.
206-208: Expose read-available for AnyPool.Matches Engine trait and keeps blocking behavior out of fast paths.
214-218: Expose write-available for AnyPool.Symmetric with read side; good.
311-314: Timer path: avoid blocking when busy.Early return on None is a sensible fallback; conversion to per-element guards is correct.
345-348: Filesystem timer mirrors pool timer.Consistent, non-blocking behavior.
759-761: Engine impl: dyn conversion via inherent method.Name resolution calls the inherent available_pools(); no recursion. All good.
767-769: Engine impl: mutable available pools dyn conversion.Symmetric and correct.
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
9bbb291 to
7b803c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/engine/structures/lock.rs (8)
374-386: Nit: spelling in docUse “writable” instead of “writeable”.
- /// Get a list of all writeable pools in the current lock. + /// Get a list of all writable pools in the current lock. fn get_available_write<T>(&mut self, inner: &mut Table<U, T>) -> Table<U, *mut T> {
1093-1096: Improve AllReadAvailable docsClarify semantics: never waits; returns None when none available; Some(guard) otherwise.
-/// Future returned by AllOrSomeLock::read_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::read_all_available(). +/// +/// This future never waits: it immediately returns +/// - `None` if no pools are currently readable, or +/// - `Some(AllLockReadAvailableGuard<...>)` containing the readable subset.
1121-1127: Doc fix: wrong guard mentionedThis is the guard for the available variant.
-/// Guard returned by AllRead future. +/// Guard returned by `AllReadAvailable` future. pub struct AllLockReadAvailableGuard<U: AsUuid, T: ?Sized>(
1128-1149: Simplify Into for AllLockReadAvailableGuard; avoid extra add_read_lockSet self.2 = false and don’t increment read counts again; mirrors write-available variant and reduces counter churn.
-impl<U, T> Into<Vec<SomeLockReadGuard<U, T>>> for AllLockReadAvailableGuard<U, T> +impl<U, T> Into<Vec<SomeLockReadGuard<U, T>>> for AllLockReadAvailableGuard<U, T> where U: AsUuid, { - fn into(self) -> Vec<SomeLockReadGuard<U, T>> { - let mut lock_record = self.0.lock().expect("Mutex only acquired internally"); + fn into(mut self) -> Vec<SomeLockReadGuard<U, T>> { + let lock_record = self.0.lock().expect("Mutex only acquired internally"); assert!(!lock_record.all_write_locked); - self.1 - .iter() - .map(|(n, u, t)| { - ( - *u, - SomeLockReadGuard(Arc::clone(&self.0), *u, n.clone(), *t as *const _, true), - ) - }) - .map(|(u, guard)| { - lock_record.add_read_lock(u, None); - guard - }) - .collect::<Vec<_>>() + // Transfer ownership of per-element read locks to the returned guards. + self.2 = false; + self.1 + .iter() + .map(|(n, u, t)| { + SomeLockReadGuard(Arc::clone(&self.0), *u, n.clone(), *t as *const _, true) + }) + .collect() } }
1228-1244: Nit: trace message is misleadingThis drops per-element read locks for an available subset, not a global “all read” lock.
- trace!("Dropping all read lock"); + trace!("Dropping read locks for available subset"); ... - trace!("All read lock dropped"); + trace!("Read locks for available subset dropped");
1449-1452: Improve AllWriteAvailable docsMatch the read variant’s clarification.
-/// Future returned by AllOrSomeLock::write_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::write_all_available(). +/// +/// This future never waits: it immediately returns +/// - `None` if no pools are currently writable, or +/// - `Some(AllLockWriteAvailableGuard<...>)` containing the writable subset.
1477-1483: Doc fix: wrong guard mentionedAvailable variant, not “AllWrite”.
-/// Guard returned by AllWrite future. +/// Guard returned by `AllWriteAvailable` future. pub struct AllLockWriteAvailableGuard<U: AsUuid, T: ?Sized>(
1609-1625: Nit: trace message is misleadingThis drops per-element write locks, not a global “all write” lock.
- trace!("Dropping all write lock"); + trace!("Dropping write locks for available subset"); ... - trace!("All write lock dropped"); + trace!("Write locks for available subset dropped");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/engine.rssrc/engine/sim_engine/engine.rssrc/engine/strat_engine/engine.rssrc/engine/structures/lock.rs
🧬 Code graph analysis (4)
src/engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(760-762)pools_mut(210-212)pools_mut(764-766)available_pools_mut(214-218)available_pools_mut(768-770)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/sim_engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(760-762)pools_mut(210-212)pools_mut(764-766)available_pools_mut(214-218)available_pools_mut(768-770)src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (11)
default(52-57)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: python-based-tests
- GitHub Check: checks (make -f Makefile fmt-ci, 1.90.0, rustfmt)
- GitHub Check: stratis-min-cli-checks
- GitHub Check: stratis-cli-checks
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: checks (make -f Makefile build, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.90.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.90.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.90.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.90.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.90.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.90.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.90.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
🔇 Additional comments (16)
src/engine/structures/lock.rs (4)
360-372: LGTM on get_available_read()Non-blocking scan + per‑element read lock acquisition looks correct.
577-584: LGTM: read_all_available() APINon‑blocking behavior with Option guard is appropriate.
1484-1501: LGTM: write-available Into transfers ownership cleanlyDisabling drop and passing per-element write locks to SomeLockWriteGuard is correct.
607-614: Doc fix: write_all_available() says "read"Change the doc to say “write”.
- /// Issue a read on all available elements. + /// Issue a write on all available elements. pub async fn write_all_available(&self) -> Option<AllLockWriteAvailableGuard<U, T>> {src/engine/structures/mod.rs (1)
10-12: LGTM: re-exports updatedNew available guard types are correctly exposed.
src/engine/engine.rs (3)
22-25: LGTM: imports include available guard typesConsistent with new trait methods.
512-515: LGTM: available_pools APIOptional, non-blocking accessor fits semantics.
519-522: LGTM: available_pools_mut APIOptional, non-blocking accessor fits semantics.
src/engine/sim_engine/engine.rs (3)
23-25: LGTM: imports include available guard types
305-307: LGTM: available_pools forwards correctly and into_dyn()No recursion risk here; uses underlying lock directly.
313-315: LGTM: available_pools_mut forwards correctly and into_dyn()src/engine/strat_engine/engine.rs (5)
37-40: LGTM: imports include available guard types
206-209: LGTM: available_pools (inherent) delegates to lock
214-219: LGTM: available_pools_mut (inherent) delegates to lock
311-314: LGTM: pool_evented_timer uses non-blocking write guardEarly return on None avoids blocking the timer path.
345-348: LGTM: fs_evented_timer uses non-blocking write guardEarly return on None is appropriate.
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
7b803c0 to
8564b5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/engine/strat_engine/engine.rs (1)
756-758: Prefer UFCS to disambiguate inherent vs trait methodsAvoid potential confusion by calling inherent methods explicitly.
Apply:
- async fn pools(&self) -> AllLockReadGuard<PoolUuid, dyn Pool> { - self.pools().await.into_dyn() - } + async fn pools(&self) -> AllLockReadGuard<PoolUuid, dyn Pool> { + StratEngine::pools(self).await.into_dyn() + } - async fn available_pools(&self) -> Option<AllLockReadAvailableGuard<PoolUuid, dyn Pool>> { - self.available_pools().await.map(|l| l.into_dyn()) - } + async fn available_pools(&self) -> Option<AllLockReadAvailableGuard<PoolUuid, dyn Pool>> { + StratEngine::available_pools(self).await.map(|l| l.into_dyn()) + } - async fn pools_mut(&self) -> AllLockWriteGuard<PoolUuid, dyn Pool> { - self.pools_mut().await.into_dyn() - } + async fn pools_mut(&self) -> AllLockWriteGuard<PoolUuid, dyn Pool> { + StratEngine::pools_mut(self).await.into_dyn() + } - async fn available_pools_mut(&self) -> Option<AllLockWriteAvailableGuard<PoolUuid, dyn Pool>> { - self.available_pools_mut().await.map(|l| l.into_dyn()) - } + async fn available_pools_mut(&self) -> Option<AllLockWriteAvailableGuard<PoolUuid, dyn Pool>> { + StratEngine::available_pools_mut(self).await.map(|l| l.into_dyn()) + }Also applies to: 760-762, 764-766, 768-770
🧹 Nitpick comments (1)
src/engine/structures/lock.rs (1)
607-614: Fix doc inaccuracies and clarify “available” futures/guards
- Correct “read”→“write” in write_all_available docs.
- Name guards after the correct futures (…Available).
- Briefly explain the “never wait” semantics.
Apply:
@@ - /// Issue a read on all available elements. + /// Issue a write on all available elements. pub async fn write_all_available(&self) -> Option<AllLockWriteAvailableGuard<U, T>> { @@ -/// Future returned by AllOrSomeLock::read_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::read_all_available(). +/// +/// Polls immediately and never waits. Returns: +/// - Some(AllLockReadAvailableGuard) if at least one entry is readable now +/// - None if no entries are currently readable. struct AllReadAvailable<U: AsUuid, T>(AllOrSomeLock<U, T>); @@ -/// Guard returned by AllRead future. +/// Guard returned by the `read_all_available` future. pub struct AllLockReadAvailableGuard<U: AsUuid, T: ?Sized>( @@ -/// Future returned by AllOrSomeLock::write_all_available(). -/// -/// Should never wait. +/// Future returned by AllOrSomeLock::write_all_available(). +/// +/// Polls immediately and never waits. Returns: +/// - Some(AllLockWriteAvailableGuard) if at least one entry is writable now +/// - None if no entries are currently writable. struct AllWriteAvailable<U: AsUuid, T>(AllOrSomeLock<U, T>); @@ -/// Guard returned by AllWrite future. +/// Guard returned by the `write_all_available` future. pub struct AllLockWriteAvailableGuard<U: AsUuid, T: ?Sized>(Also applies to: 1093-1119, 1121-1127, 1449-1455, 1477-1483
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/engine.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/engine/sim_engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(760-762)pools_mut(210-212)pools_mut(764-766)available_pools_mut(214-218)available_pools_mut(768-770)src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (11)
default(52-57)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build-min, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.90.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.90.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.90.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.90.0, clippy)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.90.0, cargo)
🔇 Additional comments (9)
src/engine/structures/mod.rs (1)
10-12: Re-exports look goodNew available-guard types are correctly surfaced alongside existing guards.
src/engine/sim_engine/engine.rs (2)
23-26: Imports updated correctlyAvailable-guard types are imported from structures; matches new API.
305-307: Non-blocking available accessors LGTMForwarding to read_all_available/write_all_available and converting to dyn is correct.
Also applies to: 313-315
src/engine/strat_engine/engine.rs (3)
37-40: Imports updated correctlyAvailable-guard types added; consistent with module changes.
206-209: Inherent available methods LGTM*They correctly forward to read_all_available/write_all_available.
Also applies to: 214-219
311-314: Timer path uses available write guards correctlyEarly-return on None prevents blocking and maintains per-element write locks via into(); good.
Also applies to: 345-348
src/engine/structures/lock.rs (3)
360-372: Available lock acquisition logic LGTMNon-conflicting scan + per-element lock accounting is sound for both read and write paths; futures never wait as intended.
Also applies to: 374-386, 577-584
1228-1245: Drop semantics for available guards are correctPer-element lock removal on drop matches acquisition; wake() usage is appropriate.
Also applies to: 1609-1625
1484-1500: Conversion to per-element write guards LGTMSuppressing drop and not double-counting locks is correct.
jbaublitz
force-pushed
the
issue-stratisd-3837
branch
from
8564b5e to
270c5d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/engine/structures/lock.rs (2)
374-386: Fix documentation comment.Apply this diff to correct the spelling:
- /// Get a list of all writeable pools in the current lock. + /// Get a list of all writable pools in the current lock. fn get_available_write<T>(&mut self, inner: &mut Table<U, T>) -> Table<U, *mut T> {
607-613: Fix incorrect documentation.The doc comment incorrectly states "read" when it should say "write".
Apply this diff:
- /// Issue a read on all available elements. + /// Issue a write on all available elements. pub async fn write_all_available(&self) -> Option<AllLockWriteAvailableGuard<U, T>> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/engine/engine.rs(2 hunks)src/engine/sim_engine/engine.rs(2 hunks)src/engine/strat_engine/engine.rs(5 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/structures/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/sim_engine/engine.rssrc/engine/engine.rssrc/engine/strat_engine/engine.rssrc/engine/structures/lock.rs
📚 Learning: 2025-09-05T14:32:42.528Z
Learnt from: jbaublitz
Repo: stratis-storage/stratisd PR: 3898
File: src/engine/sim_engine/pool.rs:934-948
Timestamp: 2025-09-05T14:32:42.528Z
Learning: In src/engine/sim_engine/pool.rs, the start_encrypt_pool method intentionally creates a SizedKeyMemory with 1 byte allocated but 0 length reported - this is to avoid NULL pointer issues while simulating an empty key slice in the sim engine.
Applied to files:
src/engine/sim_engine/engine.rssrc/engine/engine.rs
🧬 Code graph analysis (4)
src/engine/sim_engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(760-762)pools_mut(210-212)pools_mut(764-766)available_pools_mut(214-218)available_pools_mut(768-770)src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)
src/engine/engine.rs (2)
src/engine/strat_engine/engine.rs (6)
available_pools(206-208)available_pools(760-762)pools_mut(210-212)pools_mut(764-766)available_pools_mut(214-218)available_pools_mut(768-770)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (3)
available_pools(514-514)pools_mut(517-517)available_pools_mut(521-521)src/engine/sim_engine/engine.rs (3)
available_pools(305-307)pools_mut(309-311)available_pools_mut(313-315)
src/engine/structures/lock.rs (1)
src/engine/structures/table.rs (11)
default(52-57)next(71-75)next(94-98)next(117-121)get_by_uuid(212-216)get_by_name(205-209)iter(182-186)len(178-180)get_mut_by_uuid(227-231)get_mut_by_name(219-224)iter_mut(188-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.91.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.91.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
- GitHub Check: python-based-tests
- GitHub Check: stratis-min-cli-checks
- GitHub Check: shell-checks
- GitHub Check: stratis-cli-checks
- GitHub Check: checks (make -f Makefile fmt-ci, 1.91.0, rustfmt)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.91.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.91.0, clippy)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.91.0, cargo)
🔇 Additional comments (17)
src/engine/structures/mod.rs (1)
10-12: LGTM!The re-exports for the new availability guard types are correctly placed and follow the existing pattern.
src/engine/sim_engine/engine.rs (2)
305-307: LGTM!The
available_poolsimplementation correctly forwards toread_all_availableand converts to the dynamic trait object, following the same pattern as the existingpoolsmethod.
313-315: LGTM!The
available_pools_mutimplementation correctly forwards towrite_all_availableand converts to the dynamic trait object, consistent with the existingpools_mutmethod.src/engine/engine.rs (2)
512-514: LGTM!The trait method signature and documentation clearly communicate the non-blocking availability-based semantics.
519-521: LGTM!The trait method signature and documentation are clear and consistent with the read variant.
src/engine/strat_engine/engine.rs (6)
206-208: LGTM!The inherent method correctly delegates to the underlying lock's
read_all_availablemethod.
214-218: LGTM!The inherent method correctly delegates to the underlying lock's
write_all_availablemethod.
311-314: LGTM!The timer-based pool event handler correctly uses
write_all_availableto avoid blocking and early-returns an empty map when no pools are available. This prevents unnecessary waiting in the timer thread.
345-348: LGTM!The timer-based filesystem event handler correctly uses
write_all_availablewith the same early-return pattern, maintaining consistency with the pool event handler.
760-762: LGTM!The trait implementation correctly calls the inherent method (Rust prioritizes inherent methods over trait methods for concrete types) and converts from
AnyPooltodyn Pool. This is the intended bridging pattern.
768-770: LGTM!The trait implementation follows the same correct pattern as
available_pools, bridging from the concreteAnyPooltype to the trait object.src/engine/structures/lock.rs (6)
360-372: LGTM!The helper correctly identifies available pools for reading by checking that they don't conflict with existing locks or woken waiters, and pre-acquires read locks for each available pool.
577-583: LGTM!The method correctly provides non-blocking access to all currently available pools for reading.
1093-1119: LGTM!The future correctly implements synchronous polling (never waits) and returns
Nonewhen no pools are available orSome(guard)when at least one pool is available for reading.
1121-1244: LGTM!The guard implementation correctly:
- Provides iteration and lookup methods
- Converts to dynamic trait objects via
into_dyn- Releases per-element read locks in Drop (lines 1237-1239)
1449-1475: LGTM!The write variant follows the same synchronous polling pattern as the read variant and correctly acquires per-element write locks.
1477-1625: LGTM!The write guard implementation correctly:
- Acquires per-element write locks via
get_available_write- Provides mutation methods and iteration
- Properly converts to
Vec<SomeLockWriteGuard>without duplicating locks (setsself.2 = false)- Releases per-element write locks in Drop (lines 1618-1620)
1 participant
Summary by CodeRabbit
New Features
Performance
Documentation