Add udev semaphore synchronization for uevent-generating commands #774

bmr-cymru · 2022-07-26T16:57:48Z

This branch adds support for udev synchronization using SysV semaphores and the device-mapper ioctl/uevent "cookie" mechanism.

The branch adds a new sub-module, dm_udev_sync that wraps the low-level semaphore operations and provides an interface to core::dm.

All udev synchronization is carried out insde DM::do_ioctl(). This centralises and simplifies handling ioctl errors (which require the udev semaphore to be cleaned up before propagating the error) and minimises the changes needed to synchronize with uevents.

This branch also adds basic logging infrastructure to the crate, using the log facade and the env_logger crate: this was helpful in developing and debugging the udev interactions and may be useful for debugging other problems involving device-mapper.

mulkieran · 2022-08-02T21:03:01Z

@bmr-cymru Plz put up a stratisd PR that uses these changes in the manner intended. You can just edit that PRs Cargo.toml to point the devicemapper-rs dependency at this PR. Sort of like stratis-storage/stratisd#2907, although that is tremendously obsolete now.

bmr-cymru · 2022-08-04T20:23:55Z

Pulling this back into draft for now to fix some problems I found while testing with stratis.

mulkieran

Still working...one minor comment.

Cargo.toml

bmr-cymru · 2023-02-22T16:22:37Z

Good point. I'll update the commit to drop that mention at least - I can test removing the allowlist although in some ways I think it's desirable even with the fix for the await keyword. It keeps the bindings to the minimum set that we need and avoids pulling in unwanted types and interfaces.

bmr-cymru · 2023-02-22T16:28:49Z

I think it's best to keep the allow listing - this avoids conflicts between the types in the generated bindings that result from the same types being exposed in both the device-mapper and SysV semaphore headers:

error[E0428]: the name `__uid_t` is defined multiple times
    --> /home/breeves/src/git/devicemapper-rs/target/debug/build/devicemapper-sys-ab734666951c00f5/out/sem-bindings.rs:14:1
     |
14   | pub type __uid_t = ::std::os::raw::c_uint;
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `__uid_t` redefined here
     |
    ::: /home/breeves/src/git/devicemapper-rs/target/debug/build/devicemapper-sys-ab734666951c00f5/out/dm-bindings.rs:1294:1
     |
1294 | pub type __uid_t = ::std::os::raw::c_uint;
     | ------------------------------------------ previous definition of the type `__uid_t` here
     |
     = note: `__uid_t` must be defined only once in the type namespace of this module

error[E0428]: the name `__gid_t` is defined multiple times
    --> /home/breeves/src/git/devicemapper-rs/target/debug/build/devicemapper-sys-ab734666951c00f5/out/sem-bindings.rs:15:1
     |
15   | pub type __gid_t = ::std::os::raw::c_uint;
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `__gid_t` redefined here
     |
    ::: /home/breeves/src/git/devicemapper-rs/target/debug/build/devicemapper-sys-ab734666951c00f5/out/dm-bindings.rs:1295:1
     |
1295 | pub type __gid_t = ::std::os::raw::c_uint;
     | ------------------------------------------ previous definition of the type `__gid_t` here
     |
     = note: `__gid_t` must be defined only once in the type namespace of this module

I'll edit the commit message accordingly.

mulkieran

Last batch of minor suggestions.

src/core/dm_udev_sync.rs

mulkieran · 2023-02-28T19:58:35Z

@bmr-cymru Ok! It looks like we're good to go. Let me know when you've squashed all the commits that need to be squashed, and then I'll merge the PR and make a new release.

Add the log crate for the logging facade macros and enable the Rust env_logger in the testing module. Related: stratis-storage#768

Add simple debug!() logging statements to each device-mapper operation in the dm module. Resolves: stratis-storage#768

The udev primary source flag will be set automatically by DM::do_ioctl() when issuing a device-mapper ioctl command that generates uevents: remove the setting of the flag from the thindev new, setup, and snapshot methods.

A device-mapper cookie is a 32-bit value comprising 16 device-mapper udev rule control flags in the upper sixteen bits and a randomly generated base value in the lower 16 bits: +----------------+----------------+ | udev flags | random base | +----------------+----------------+ The devicemapper-rs DmCookie type is mis-named since it represents the possible values of the udev flags in the upper bits of a cookie value: re-name the type as DmUdevFlags to make the distinction more clear.

Avoid manually re-defining the constants from libdevmapper (for e.g. the udev flag values) by incorporating these definitions into the bindings generated for devicemapper-rs-sys. An allowlist is used to control the visibility of types and constants in the bindings to prevent including unwanted interfaces included from the libdevmapper headers: this avoids conflicts when the same types are defined in the SysV semaphore interface. Use the symbolic names for device-mapper udev flags from the devicemapper-rs-sys bindings rather than hard coding the literal flag values and make the type a plain u32 rather than dmi::__u32. Resolves: stratis-storage#730

Add bindings for the SysV semaphore interface to provide constants and structs currently missing from the libc bindings for some target environments and expose these definitions in a new core::sysvsem sub-module.

Add a new sub-module to encapsulate creating, destroying, and waiting on device-mapper Udev synchronisation semaphores. The module contains the low-level wrappers to the unsafe SysV semaphore bindings provided by the libc crate as well as higher level UdevSync interface for use by other sub-modules: UdevSync::begin() UdevSync::end() UdevSync::cancel() This provides the necessary primitives to implement the ioctl-issuing side of the DM cookie protocol. Completion of DM cookie semaphores is not implemented as this is always carried out by the dmsetup program via udev rule callbacks. This simplifies integrating udev synchronization with the other DM methods and avoids the need to expose the low-level semaphore operations outside the dm_udev_sync module. Since it is possible for the kernel to be compiled without semaphore support or for the administrator to dynamically disable their use via sysctl, runtime semaphore limits are checked in a new helper function, dm_udev_sync::sysv_sem_supported(). Testing the maxid value returned by semctl(0, 0, SEM_INFO, ...) as is done by libdevmapper is not sufficient to determine whether semaphores can be used. It is possible for the administrator to disable them completely via sysctl without causing the maxid value to be less than zero: # echo 0 0 0 0 > /proc/sys/kernel/sem Check that the following values in the returned seminfo struct are greater than zero: semmsl - Maximum number of semaphores in a set semmni - Maximum number of semaphore sets semmns - Maximum number of semaphores in all semaphore sets Additionally log a warning if the value of any of these settings is less than 1000 which is an exceptional situation based on typical modern distribution defaults for these settings. Related: stratis-storage#774

Add udev synchronization to device-mapper ioctl operations that generate uevents by calling UdevSync::begin(), sync.cancel() and sync.end() directly from DM::do_ioctl(). This simplifies and unifies error handling and udev sync clean up vs. implementing synchronization in specific device_*() methods that require it. DM::do_ioctl() decodes the ioctl command to determine whether or not to set the DM_UDEV_PRIMARY_SOURCE_FLAG, which also determines whether or not udev synchronisation should be used for the operation. Related: stratis-storage#774

Allow up to five attempts to remove a device-mapper device when the ioctl returns EBUSY: this implements the same scheme as libdevmapper but with a lower maximum retry count (5 vs. 25): this has been found to be adequate in testing devicemapper-rs. Related: stratis-storage#774

…cache_dev Allowing udev time to interact with the devices invalidates some of the assumptions in loop_test_minimal_cache_dev regarding the state of the cache since udev probes will issue reads to the device to determine its content, meaning that the cache is no longer completely pristine. Remove the following assertions: assert_eq!(usage.used_cache, DataBlocks(0)); assert_eq!(performance.read_hits, 0); assert_eq!(performance.read_misses, 0); assert_eq!(performance.promotions, 0); The alternative would be to expose the DmOptions used to create the cache device stack to users of the CacheDev, allowing device-mapper udev flags to be used to suppress udev rule processing however this would be a quite invasive change solely for the use of the test suite.

Prevent xfs_admin -U from racing with udev inotify disarm while processing the previous (mkfs.xfs) generated uevent by calling udev_settle() after invoking any process that opens a block device for writing.

bmr-cymru · 2023-03-01T17:37:47Z

@mulkieran I've squashed all the fixup commits into their parents and tidied the branch up a little. It's still 12 commits but I think it makes a logical sequence of changes. Let me know if you'd like me to squash more aggressively.

bmr-cymru requested a review from jbaublitz July 26, 2022 16:57

bmr-cymru force-pushed the bmr-udev-sync branch 17 times, most recently from 2a1bd12 to 1cc7d09 Compare August 1, 2022 16:39

bmr-cymru marked this pull request as ready for review August 1, 2022 16:54

bmr-cymru requested a review from mulkieran August 1, 2022 16:54

bmr-cymru force-pushed the bmr-udev-sync branch 2 times, most recently from e44f5c3 to f5e10b3 Compare August 2, 2022 12:18

mulkieran assigned bmr-cymru Aug 2, 2022

mulkieran mentioned this pull request Aug 2, 2022

trigger udev rules processing when adding linear devices #727

Closed

This was referenced Aug 2, 2022

Set DM_UDEV_PRIMARY_SOURCE_FLAG in ioctl #740

Closed

Pass DM_UDEV_PRIMARY_SOURCE_FLAG with every ioctl #741

Closed

Proposal to send DM_UDEV_PRIMARY_SOURCE_FLAG w/ every ioctl sent in devicemapper-rs stratis-storage/project#394

Closed

bmr-cymru marked this pull request as draft August 4, 2022 20:24

bmr-cymru force-pushed the bmr-udev-sync branch from d12e103 to 6d114a9 Compare February 22, 2023 15:05

mulkieran requested changes Feb 22, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

bmr-cymru force-pushed the bmr-udev-sync branch from 6d114a9 to 6bf146c Compare February 22, 2023 15:47

bmr-cymru force-pushed the bmr-udev-sync branch from 6bf146c to 18882d7 Compare February 22, 2023 16:30

mulkieran requested changes Feb 24, 2023

View reviewed changes

mulkieran approved these changes Feb 27, 2023

View reviewed changes

mulkieran mentioned this pull request Feb 28, 2023

devicemapper-rs 0.33.0 release #833

Closed

jbaublitz approved these changes Feb 28, 2023

View reviewed changes

bmr-cymru force-pushed the bmr-udev-sync branch 3 times, most recently from cc2a352 to b5ac771 Compare March 1, 2023 17:31

bmr-cymru added 12 commits March 1, 2023 17:33

Add log infrastructure and use env_logger in testing

b6d6156

Add the log crate for the logging facade macros and enable the Rust env_logger in the testing module. Related: stratis-storage#768

Add basic debug logging to dm module

66e1fdf

Add simple debug!() logging statements to each device-mapper operation in the dm module. Resolves: stratis-storage#768

Don't set DM_UDEV_PRIMARY_SOURCE_FLAG in thindev methods

f27153e

The udev primary source flag will be set automatically by DM::do_ioctl() when issuing a device-mapper ioctl command that generates uevents: remove the setting of the flag from the thindev new, setup, and snapshot methods.

Add SysV semaphore bindings to devicemapper-sys

6f9f675

Add bindings for the SysV semaphore interface to provide constants and structs currently missing from the libc bindings for some target environments and expose these definitions in a new core::sysvsem sub-module.

Fix udev race with xfs_admin -U <new uuid> in loop_test_basic_udev

e03a712

Prevent xfs_admin -U from racing with udev inotify disarm while processing the previous (mkfs.xfs) generated uevent by calling udev_settle() after invoking any process that opens a block device for writing.

Update CI dependencies for libdevmapper build requirement

22dfc2c

bmr-cymru force-pushed the bmr-udev-sync branch from b5ac771 to 22dfc2c Compare March 1, 2023 17:35

mulkieran merged commit e28c016 into stratis-storage:master Mar 1, 2023

bmr-cymru mentioned this pull request Apr 3, 2023

Make internal devices private by default #845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add udev semaphore synchronization for uevent-generating commands #774

Add udev semaphore synchronization for uevent-generating commands #774

Uh oh!

bmr-cymru commented Jul 26, 2022 •

edited

Loading

Uh oh!

mulkieran commented Aug 2, 2022 •

edited

Loading

Uh oh!

bmr-cymru commented Aug 4, 2022

Uh oh!

mulkieran left a comment

Uh oh!

Uh oh!

bmr-cymru commented Feb 22, 2023

Uh oh!

bmr-cymru commented Feb 22, 2023 •

edited

Loading

Uh oh!

mulkieran left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mulkieran commented Feb 28, 2023

Uh oh!

bmr-cymru commented Mar 1, 2023

Uh oh!

Uh oh!

Add udev semaphore synchronization for uevent-generating commands #774

Add udev semaphore synchronization for uevent-generating commands #774

Uh oh!

Conversation

bmr-cymru commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mulkieran commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmr-cymru commented Aug 4, 2022

Uh oh!

mulkieran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmr-cymru commented Feb 22, 2023

Uh oh!

bmr-cymru commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mulkieran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mulkieran commented Feb 28, 2023

Uh oh!

bmr-cymru commented Mar 1, 2023

Uh oh!

Uh oh!

bmr-cymru commented Jul 26, 2022 •

edited

Loading

mulkieran commented Aug 2, 2022 •

edited

Loading

bmr-cymru commented Feb 22, 2023 •

edited

Loading