disks: use advisory locks for disk image files #6974

phip1611 · 2025-03-11T15:50:38Z

About

Add advisory file locks for disk images, aligning the behavior with QEMU. Please check out the commit messages for more details.

Hints for Reviewers

Please review this commit-by-commit.

Steps to Undraft

basic functionality
add command line option to make this configurable (context)
solution for live migration

iggy · 2025-03-11T16:00:52Z

This should either be opt-in (for backwards compat) or at the very least have a way to opt-out. I'm pretty sure there's a test that migrates on the same host that will fail.

phip1611 · 2025-03-11T16:05:47Z

@iggy I think you're right. I'll look into how QEMU solves this and add a second commit soon.

vmm/src/device_manager.rs

rbradford · 2025-03-11T17:04:46Z

There is an intentional use case here where you

@iggy I think you're right. I'll look into how QEMU solves this and add a second commit soon.

Yes - you will need to add an option to --block - I propose lock with an enum of "off, warn, on" . Default to warn for this this release and then move to "on" from a future release.

phip1611 · 2025-03-11T19:40:42Z

From what I've seen, there is no code I could reuse for a tristate parsing (off|warn|on), it also sounds a little feature creep to me 🤔. I'd much rather stick with two states so that I can reuse the Toggle abstraction.

I'd like to suggest that the default behavior from now on is:

off => always log::warn if the file is locked but don't throw an error
on => return an error

What do you think, @rbradford?

We discussed [0] that the new behaviour should be made configurable with a tristate: off,warn,on. We add it to the next release with "warn" by default and "on" for the release after that. [0] cloud-hypervisor#6974 (comment)

rbradford · 2025-03-11T22:41:22Z

From what I've seen, there is no code I could reuse for a tristate parsing (off|warn|on), it also sounds a little feature creep to me 🤔. I'd much rather stick with two states so that I can reuse the Toggle abstraction.

I'd like to suggest that the default behavior from now on is:
* `off` => always `log::warn` if the file is locked but don't throw an error

* `on` => return an error
What do you think, @rbradford?

I think I would prefer the enum of three different values as if you know what you're doing and you don't want a warn spamming you then you can choose "off". You can use vm_config::HotplugMethod as an example on how to implement. If you give it a general name like WarnToggle it might get used elsewhere.

We discussed [0] that the new behaviour should be made configurable with a tristate: off,warn,on. off => ignore warn => Warn when disk-images are already used by other processes and this VM wants to have exclusive access. Don't acquire any locks. on => Enforce the acquiring of advisory file-system locks for disk image files. "warn" is the default for now and we will soon switch to "on" [0] cloud-hypervisor#6974 (comment)

We discussed [0] that the new behaviour should be made configurable with a tristate: off,warn,on. off => ignore warn => Warn when disk-images are already used by other processes and this VM wants to have exclusive access. Don't acquire any locks. on => Enforce the acquiring of advisory file-system locks for disk image files. "warn" is the default for now and we will soon switch to "on" [0] cloud-hypervisor#6974 (comment) Signed-off-by: Philipp Schuster <[email protected]>

phip1611 · 2025-03-12T09:54:59Z

I adjusted the CLI as discussed. I'm happy for a review of that part. I'll look into a solution for the live-migration as last step.

~~It is unfortunate that gitlint doesn't ignore links. How do we solve the failing CI?~~
Fixed that in a dedicated commit, hope this is fine.

We discussed [0] that the new behaviour should be made configurable with a tristate: off,warn,on. off => ignore warn => Warn when disk-images are already used by other processes and this VM wants to have exclusive access. Don't acquire any locks. on => Enforce the acquiring of advisory file-system locks for disk image files. "warn" is the default for now and we will soon switch to "on" [0] cloud-hypervisor#6974 (comment) Signed-off-by: Philipp Schuster <[email protected]>

rbradford · 2025-04-17T12:58:52Z

@blitz What's the reason this PR is still draft? Can we move ahead with it?

phip1611 · 2025-05-14T10:54:21Z

Yikes, took quite a while. The CI integration tests are finally green.

Since your last major review, the following changes have been added:

Device hot plugging of disks and also unplugging
DiskFile::fd() is safer to use

@rbradford @likebreath

A comprehensive write-up is in the commit message. Excerpt:

block: introduce advisory locks for disk image files

# What

This commit introduces file-based advisory locking for the files backing
up the block devices by using the fcntl() syscall with OFD locks. The
per-open-file-descriptor (OFD) locks are more robust than traditional
POSIX locks (F_SETLK) as they are not tied to process IDs and avoid
common issues in multithreaded or multi-fd scenarios [1]. Therefore,
we don't use `std::fs::File::try_lock()`, which is backed by F_SETLKW.

The locking mechanism is aware of the `readonly` property and allows
`n` readers or `1` writer (exclusive mode).

As the locks are advisory, multiple cloud-hypervisor processes can
prevent themselves from writing to the same file. However, this is not
a system-wide file-system level locking mechanism preventing to open()
a file.

The introduced new locking mechanism does not cover vhost-user devices.

# Why

To prevent misconfiguration and improve safety, it is good practice to
protect disk image files with a locking mechanism. Experience and common
best practices suggest that advisory locks are preferable over mandatory
locks due to better compatibility and fewer pitfalls (in fs space).

The introduced functionality is aligned with the approach taken by
QEMU [0], and is also recommended in [1].

# Implementation Details

We need to ensure that not only normal operation keeps working but also
state save/resume and live-migration. Especially for live migration,
it is crucial that the sender VMM releases the locks when the VM stops
so the receiver VMM can acquire them right after that.

Therefore, the locking and releasing happen directly on the block
device struct. The device manager knows all block devices and can
forward requests to these types.

Last but not least, this commit uses on explicit lock acquiring
but implicit lock releasing (FD close). It only explicitly releases
the locks where this integrates more smoothly into the existing
code.

# Testing

I tested
- normal operation
- state save/resume,
- device hot plugging,
- and live-migration
with read/shared and write/exclusive locks.

One can use the `fcntl-tool` to test if locks are actually acquired
or released [2].

# Links

[0] https://github.com/qemu/qemu/blob/825b96dbcee23d134b691fc75618b59c5f53da32/util/osdep.c#L266
[1] https://apenwarr.ca/log/20101213
[2] https://crates.io/crates/fcntl-tool

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP <[email protected]>

tests/integration.rs

vmm/src/device_manager.rs

rbradford

Just some minor feedback about the last commit.

fuzz/Cargo.toml

This simplifies debugging of running VMs. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

This is a prerequisite for the next steps. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

As we can't use BorrowedFd, we should at least create a similar safe alternative. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

This is a prerequisite for the next steps. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

# What This commit introduces file-based advisory locking for the files backing up the block devices by using the fcntl() syscall with OFD locks. The per-open-file-descriptor (OFD) locks are more robust than traditional POSIX locks (F_SETLK) as they are not tied to process IDs and avoid common issues in multithreaded or multi-fd scenarios [1]. Therefore, we don't use `std::fs::File::try_lock()`, which is backed by F_SETLKW. The locking mechanism is aware of the `readonly` property and allows `n` readers or `1` writer (exclusive mode). As the locks are advisory, multiple cloud-hypervisor processes can prevent themselves from writing to the same file. However, this is not a system-wide file-system level locking mechanism preventing to open() a file. The introduced new locking mechanism does not cover vhost-user devices. # Why To prevent misconfiguration and improve safety, it is good practice to protect disk image files with a locking mechanism. Experience and common best practices suggest that advisory locks are preferable over mandatory locks due to better compatibility and fewer pitfalls (in fs space). The introduced functionality is aligned with the approach taken by QEMU [0], and is also recommended in [1]. # Implementation Details We need to ensure that not only normal operation keeps working but also state save/resume and live-migration. Especially for live migration, it is crucial that the sender VMM releases the locks when the VM stops so the receiver VMM can acquire them right after that. Therefore, the locking and releasing happen directly on the block device struct. The device manager knows all block devices and can forward requests to these types. Last but not least, this commit uses on explicit lock acquiring but implicit lock releasing (FD close). It only explicitly releases the locks where this integrates more smoothly into the existing code. # Testing I tested - normal operation - state save/resume, - device hot plugging, - and live-migration with read/shared and write/exclusive locks. One can use the `fcntl-tool` to test if locks are actually acquired or released [2]. # Links [0] https://github.com/qemu/qemu/blob/825b96dbcee23d134b691fc75618b59c5f53da32/util/osdep.c#L266 [1] https://apenwarr.ca/log/20101213 [2] https://crates.io/crates/fcntl-tool Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

The new locking behavior uncovered that unfortunate test situation: Many tests running in parallel access the same disk image with rw permissions. Luckily, none of the tests actually writes to the disk. Therefore, we can set it to readonly=true. In case this changes, the test needs to be moved to the sequential test module. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

phip1611 · 2025-05-16T09:15:03Z

Great that this is merged! Feel free to ping me in case any bug reports are incoming; I'm happy to help out

phip1611 requested a review from a team as a code owner March 11, 2025 15:50

phip1611 force-pushed the disk-lock branch 2 times, most recently from bceac2b to 2b03eb1 Compare March 11, 2025 15:53

phip1611 force-pushed the disk-lock branch 2 times, most recently from 8560a88 to 17f6654 Compare March 11, 2025 16:04

phip1611 force-pushed the disk-lock branch 2 times, most recently from 1767409 to 0d71e3f Compare March 11, 2025 16:08

alyssais reviewed Mar 11, 2025

View reviewed changes

vmm/src/device_manager.rs Outdated Show resolved Hide resolved

phip1611 marked this pull request as draft March 11, 2025 19:28

phip1611 force-pushed the disk-lock branch from 0d71e3f to de643e5 Compare March 11, 2025 19:43

phip1611 force-pushed the disk-lock branch from dd82d48 to 226f26d Compare March 12, 2025 09:42

phip1611 force-pushed the disk-lock branch from 226f26d to 8a3f3b0 Compare March 12, 2025 09:44

phip1611 force-pushed the disk-lock branch 2 times, most recently from ba8b35c to d404fd4 Compare March 12, 2025 09:50

phip1611 force-pushed the disk-lock branch from a125145 to 99dbf4d Compare March 12, 2025 10:05

phip1611 force-pushed the disk-lock branch 2 times, most recently from 0ec8668 to 1ca0780 Compare March 12, 2025 10:21

phip1611 mentioned this pull request Mar 18, 2025

ci: gitlint now ignores long lines from links #6989

Merged

phip1611 marked this pull request as draft May 14, 2025 10:53

phip1611 force-pushed the disk-lock branch from 9970ac8 to e333229 Compare May 14, 2025 12:42

phip1611 marked this pull request as ready for review May 14, 2025 12:42

phip1611 force-pushed the disk-lock branch from e333229 to f16c6b7 Compare May 14, 2025 12:44

snue reviewed May 14, 2025

View reviewed changes

tests/integration.rs Show resolved Hide resolved

phip1611 force-pushed the disk-lock branch 2 times, most recently from e117b5f to 6ef9f73 Compare May 14, 2025 13:17

phip1611 requested a review from rbradford May 14, 2025 13:17

phip1611 commented May 14, 2025

View reviewed changes

vmm/src/device_manager.rs Show resolved Hide resolved

phip1611 force-pushed the disk-lock branch from 6ef9f73 to a0e142b Compare May 14, 2025 13:41

phip1611 mentioned this pull request May 15, 2025

Nice Error Messages on Exit for Cloud Hypervisor and ch-remote #7066

Merged

11 tasks

rbradford approved these changes May 15, 2025

View reviewed changes

fuzz/Cargo.toml Show resolved Hide resolved

phip1611 force-pushed the disk-lock branch from a0e142b to 623eeec Compare May 16, 2025 07:16

phip1611 added 7 commits May 16, 2025 09:18

devices: silence IRQ debug!() spam

0ca7618

This simplifies debugging of running VMs. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

block: fixing typo, increase clarity

0268c14

Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

block: enable to get a raw FD of each block device's DiskFile

30d9717

This is a prerequisite for the next steps. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

block: bind FD lifetime of DiskFile

cfc0a17

As we can't use BorrowedFd, we should at least create a similar safe alternative. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

block: add fcntl module for locking

2377d5b

This is a prerequisite for the next steps. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]

phip1611 force-pushed the disk-lock branch from 623eeec to bfbd002 Compare May 16, 2025 07:18

rbradford added this pull request to the merge queue May 16, 2025

Merged via the queue into cloud-hypervisor:main with commit d4718a9 May 16, 2025
39 checks passed

phip1611 deleted the disk-lock branch May 16, 2025 09:14

likebreath moved this to ✅ Done in Cloud Hypervisor Roadmap May 22, 2025

likebreath added this to Cloud Hypervisor Roadmap May 22, 2025

phip1611 mentioned this pull request Nov 21, 2025

block: advisory locks: use byte-range locks to match QEMU behavior #7494

Merged

disks: use advisory locks for disk image files #6974

disks: use advisory locks for disk image files #6974

Uh oh!

Conversation

phip1611 commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About

Hints for Reviewers

Steps to Undraft

Uh oh!

iggy commented Mar 11, 2025

Uh oh!

phip1611 commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rbradford commented Mar 11, 2025

Uh oh!

phip1611 commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbradford commented Mar 11, 2025

Uh oh!

phip1611 commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbradford commented Apr 17, 2025

Uh oh!

phip1611 commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rbradford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

phip1611 commented May 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

phip1611 commented Mar 11, 2025 •

edited

Loading

phip1611 commented Mar 11, 2025 •

edited

Loading

phip1611 commented Mar 11, 2025 •

edited

Loading

phip1611 commented Mar 12, 2025 •

edited

Loading

phip1611 commented May 14, 2025 •

edited

Loading