Support dm without udev#1036
Conversation
|
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. |
|
Hey @mulkieran I need your help to review this PR. Really appreciate it. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR modifies device-mapper's udev synchronization handling to gracefully degrade when udev is disabled or unavailable. ChangesUdev disable and graceful fallback handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@Apokleos Thanks for the PR! Please fix the clippy error and force push to resume testing. |
Apokleos
force-pushed
the
support-dm-without-udev
branch
from
2052ecc to
cd043a1
Compare
Hey @mulkieran Thanks very much. I have updated the code, but I cannot run the reset CI, which I just can see Could you please help approve to run the CI ? appreciate it. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/dm.rs (1)
1181-1217: 💤 Low valueTest reads as a good end-to-end sanity check, one small consistency nit.
The lifecycle test correctly exercises
create → table_load → resume (suspend without DM_SUSPEND) → device_info → removewith the no-udev option set, and the post-resume assertion!info.flags().contains(DmFlags::DM_SUSPEND)is a solid invariant.One small inconsistency worth fixing while you're here: the final
device_removecall rebuilds the options viano_udev_dm_options()instead of reusing the localoptsyou already created on Line 1188. Usingoptskeeps the test uniform with steps 3 and would also catch any accidental divergence ifno_udev_dm_options()is later changed.♻️ Reuse the existing options binding
- // Step 5: Remove device with no-udev options - dm.device_remove(&id, no_udev_dm_options()).unwrap(); + // Step 5: Remove device with no-udev options + dm.device_remove(&id, opts).unwrap();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/dm.rs` around lines 1181 - 1217, The test sudo_test_no_udev_device_lifecycle creates a local binding opts = no_udev_dm_options() but then calls dm.device_remove(&id, no_udev_dm_options()) instead of reusing opts; change the final dm.device_remove call to use the existing opts variable (dm.device_remove(&id, opts).unwrap()) so the test consistently reuses the same options and will detect any future divergence in no_udev_dm_options().src/core/dm_udev_sync.rs (2)
298-298: ⚡ Quick winDense one-liner — break out the predicate for readability.
matches!(ioctl as u32, A | B | C if *SYSV_SEM_SUPPORTED && (hdr.flags & DmFlags::DM_SUSPEND.bits()) == 0)is syntactically fine but it mixes "which ioctl?" with "is the environment usable?" and "is this actually a resume, not a suspend?" on one ~150-char line. Splitting it makes the intent (and the SUSPEND-vs-resume distinction) obvious and easier to test/maintain.♻️ Suggested split
- // First check if this ioctl command requires udev synchronization. - // Only REMOVE, RENAME, and SUSPEND (non-suspended) operations need it. - let requires_sync = matches!(ioctl as u32, dmi::DM_DEV_REMOVE_CMD | dmi::DM_DEV_RENAME_CMD | dmi::DM_DEV_SUSPEND_CMD if *SYSV_SEM_SUPPORTED && (hdr.flags & DmFlags::DM_SUSPEND.bits()) == 0); + // Only REMOVE, RENAME, and SUSPEND-as-resume operations generate + // udev events that we need to synchronize on, and we can only do + // that synchronization when the kernel supports SysV semaphores. + let is_udev_cookie_cmd = matches!( + ioctl as u32, + dmi::DM_DEV_REMOVE_CMD | dmi::DM_DEV_RENAME_CMD | dmi::DM_DEV_SUSPEND_CMD + ); + let is_actual_suspend = (hdr.flags & DmFlags::DM_SUSPEND.bits()) != 0; + let requires_sync = is_udev_cookie_cmd && *SYSV_SEM_SUPPORTED && !is_actual_suspend;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/dm_udev_sync.rs` at line 298, The one-liner computing requires_sync is dense; split the predicate into clearly named boolean temporaries to improve readability and testability: extract checks for the ioctl type (e.g., is_remove_or_rename_or_suspend using ioctl and dmi::DM_DEV_REMOVE_CMD | dmi::DM_DEV_RENAME_CMD | dmi::DM_DEV_SUSPEND_CMD), the environment support (is_sysv_supported using *SYSV_SEM_SUPPORTED) and the suspend-vs-resume check (is_not_resume using (hdr.flags & DmFlags::DM_SUSPEND.bits()) == 0), then set requires_sync = is_ioctl_of_type && is_sysv_supported && is_not_resume so intent and each condition are obvious and easy to unit test.
505-540: ⚡ Quick winGood end-to-end coverage; consider also asserting the early-return contract.
The test validates the
udev_disabledbranch for the three udev-cookie ioctls and asserts the right post-conditions (cookie == 0,semid == None,event_nr == 0). Nice.If you act on the early-return-clears-event_nr fix in the comment above, it would be worth adding a complementary case that exercises the
!requires_syncpath — e.g.,DM_DEV_SUSPEND_CMDwithhdr.flags |= DmFlags::DM_SUSPEND.bits()and udev flags set — to lock in thatevent_nris also cleared there. That guards against future regressions in either path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/dm_udev_sync.rs` around lines 505 - 540, Extend the test_udevsync_no_udev_options_all_sync_cmds test to also exercise the early-return (non-requires_sync) path: for the DM_DEV_SUSPEND_CMD case set hdr.flags |= DmFlags::DM_SUSPEND.bits() before calling UdevSync::begin so the code follows the !requires_sync branch, then assert the same post-conditions (sync.cookie == 0, sync.semid == None, and hdr.event_nr == 0) and that sync.end(...) returns Ok; this ensures both the requires_sync and early-return branches clear event_nr when udev is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/dm_udev_sync.rs`:
- Around line 295-329: The early-return in UdevSync::begin when !requires_sync
leaves hdr.event_nr non-zero in some cases (e.g. SYSV_SEM_SUPPORTED == false or
suspended DM_DEV_SUSPEND_CMD), which can send a udev cookie to the kernel
without a semaphore; modify begin (the block guarded by requires_sync / the
!requires_sync early return) to clear hdr.event_nr = 0 before returning the
inactive UdevSync (cookie 0, semid None), or refactor to always zero
hdr.event_nr before any inactive return; update the branch that currently
returns Ok(UdevSync { cookie: 0, semid: None }) when !requires_sync to match the
udev_disabled || !udev_running() path by zeroing hdr.event_nr.
In `@src/core/dm.rs`:
- Around line 131-140: Extract the repeated ioctl predicate into a single
function (e.g., is_udev_cookie_cmd(ioctl: u32) -> bool) that checks the
DM_DEV_REMOVE_CMD | DM_DEV_RENAME_CMD | DM_DEV_SUSPEND_CMD set and return true
for those that support udev cookies; replace the inline match in do_ioctl (where
hdr.event_nr is currently cleared based on ioctl) with a call to
is_udev_cookie_cmd, and also call the same is_udev_cookie_cmd from
UdevSync::begin to decide semaphore/allocation behavior so both sites use the
single source of truth.
---
Nitpick comments:
In `@src/core/dm_udev_sync.rs`:
- Line 298: The one-liner computing requires_sync is dense; split the predicate
into clearly named boolean temporaries to improve readability and testability:
extract checks for the ioctl type (e.g., is_remove_or_rename_or_suspend using
ioctl and dmi::DM_DEV_REMOVE_CMD | dmi::DM_DEV_RENAME_CMD |
dmi::DM_DEV_SUSPEND_CMD), the environment support (is_sysv_supported using
*SYSV_SEM_SUPPORTED) and the suspend-vs-resume check (is_not_resume using
(hdr.flags & DmFlags::DM_SUSPEND.bits()) == 0), then set requires_sync =
is_ioctl_of_type && is_sysv_supported && is_not_resume so intent and each
condition are obvious and easy to unit test.
- Around line 505-540: Extend the test_udevsync_no_udev_options_all_sync_cmds
test to also exercise the early-return (non-requires_sync) path: for the
DM_DEV_SUSPEND_CMD case set hdr.flags |= DmFlags::DM_SUSPEND.bits() before
calling UdevSync::begin so the code follows the !requires_sync branch, then
assert the same post-conditions (sync.cookie == 0, sync.semid == None, and
hdr.event_nr == 0) and that sync.end(...) returns Ok; this ensures both the
requires_sync and early-return branches clear event_nr when udev is disabled.
In `@src/core/dm.rs`:
- Around line 1181-1217: The test sudo_test_no_udev_device_lifecycle creates a
local binding opts = no_udev_dm_options() but then calls dm.device_remove(&id,
no_udev_dm_options()) instead of reusing opts; change the final dm.device_remove
call to use the existing opts variable (dm.device_remove(&id, opts).unwrap()) so
the test consistently reuses the same options and will detect any future
divergence in no_udev_dm_options().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6153e97b-0b6c-465e-b973-01587aa33be3
📒 Files selected for processing (2)
src/core/dm.rssrc/core/dm_udev_sync.rs
@Apokleos Thanks for the repush. Try running "make fmt" to fix the formatting issue and push again. I'll review the CodeRabbit remarks and let you know what I think about them. |
Apokleos
force-pushed
the
support-dm-without-udev
branch
from
cd043a1 to
ba8b1e5
Compare
@mulkieran Thanks very much for the patiance. code changes are updated now based on the CodeRabbit's advice. PTAL ! |
|
Hey @mulkieran Looking forward to your input and advice about this PR. If there's some places should be fixed or corrected ? I'd like to devote myself totally to move it forward. Really appreciate it. |
Apokleos
force-pushed
the
support-dm-without-udev
branch
from
ba8b1e5 to
464aa70
Compare
|
Hey @mulkieran FYI |
Previously, UdevSync::begin() returned a hard error when udev was not running, blocking ALL DM ioctl operations — even those that don't require udev sync (CREATE, TABLE_LOAD, STATUS, etc.). Setting DM_UDEV_DISABLE_LIBRARY_FALLBACK could not bypass this because the flag was checked after the udev_running() gate. This commit aims to address the issue with the following key changes: (1) Clear event_nr in do_ioctl() for commands that don't carry a udev cookie (only REMOVE, RENAME, and SUSPEND do). A non-zero event_nr on other commands could cause the kernel to return EINVAL or wait indefinitely for a udev completion that never arrives. (2) Reorder UdevSync::begin() to check whether the ioctl actually requires sync before checking udev_running(). Non-sync ioctls now return an inactive UdevSync immediately. When sync is required but udev is disabled or not running, clear event_nr and return inactive instead of erroring. (3) Extract ioctl_uses_udev_cookie(ioctl: u8) -> bool into dm_ioctl.rs as the single source of truth, replacing duplicated match arms in do_ioctl and UdevSync::begin. (4) Add unit test (test_udevsync_no_udev_options_all_sync_cmds) and integration test (sudo_test_no_udev_device_lifecycle) validating the full dm-verity device lifecycle with all DM_UDEV_DISABLE_* flags set. Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>
Apokleos
force-pushed
the
support-dm-without-udev
branch
from
464aa70 to
f4f4760
Compare
|
Hey @mulkieran Looking forward to your feedback. |
The kata guest VM runs without udev, so device-mapper nodes under /dev/mapper are never created automatically. Add the foundational helpers that subsequent dm-verity integration will rely on: It focus on the following key points: (1) DmOptions builders that disable all udev synchronization flags, with read-only and deferred-remove variants. (2) mknod-based device node creation/removal under /dev/mapper, since devtmpfs nodes are not auto-created without udev. Also add the devicemapper crate dependency (default-features = false). But note that the commit depends on device mapper with no-udev support with the PR:stratis-storage/devicemapper-rs#1036 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>
|
@Apokleos Thank you for keeping PR up-to-date. I hope to be able to give you a aresponse next week. |
The kata guest VM runs without udev, so device-mapper nodes under /dev/mapper are never created automatically. Add the foundational helpers that subsequent dm-verity integration will rely on: It focus on the following key points: (1) DmOptions builders that disable all udev synchronization flags, with read-only and deferred-remove variants. (2) mknod-based device node creation/removal under /dev/mapper, since devtmpfs nodes are not auto-created without udev. Also add the devicemapper crate dependency (default-features = false). But note that the commit depends on device mapper with no-udev support with the PR:stratis-storage/devicemapper-rs#1036 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>
2 participants
It aims to address issue #1035
Summary by CodeRabbit
Bug Fixes
Tests