QEMU compatible file locking by arctic-alpaca · Pull Request #180 · cyberus-technology/cloud-hypervisor

arctic-alpaca · 2026-07-06T12:17:58Z

Introduces QEMU compatible file locking.
Please see the commit messages for additional information.

References:

QEMU locking: https://github.com/qemu/qemu/blob/master/block/file-posix.c#L868-L940
QEMU lock checking: https://github.com/qemu/qemu/blob/master/block/file-posix.c#L942-L982
QEMU lock offsets: https://github.com/qemu/qemu/blob/master/block/file-posix.c#L131-L134
QEMU lock permission bits: https://github.com/qemu/qemu/blob/master/include/block/block-common.h#L392-L437

Testing was done on customer infra and the fix is confirmed to fix the reported issues.

The default is set to QEMU compatible file locking to avoid the need for changes in libvirt. We can revert that at a later point and use libvirt to set the appropriate config. In the interest of speed, I favor setting the default for now.
The BLK_PERM_RESIZE permission will be added as a follow-up as to not delay the fix since testing showed the current implementation to be sufficient for the moment (https://github.com/cobaltcore-dev/cobaltcore/issues/627).

Fixes https://github.com/cobaltcore-dev/cobaltcore/issues/595

Libvirt Pipeline: https://gitlab.cyberus-technology.de/cyberus/cloud/libvirt/-/merge_requests/253

phip1611

I know it is still in draft. I reviewed because my vacation starts in ~90min. I think this is looking good and is going into the right direction!

Thanks for the fix and the nice commit history!

Coffeeri

Amazing PR, Julian! Also love the easy-to-follow refactoring commits.

amphi

I really like your refactors and think it made the code more clear in the beginning, but then the naming and descriptions became a bit confusing to me.

You took the name CONSISTENT_READ from Qemu, but without describing why it is "consistent read" and not just "read", this is confusing I think. It makes sense when looking at the Qemu code base, which explains it, and also has the BLK_PERM_WRITE_UNCHANGED permission. As we only have read or write, I think calling it just QEMU_READ_BYTE is much less confusing.

Then you changed RAW_LOCK_SHARED_BASE to QEMU_UNSHARE_LOCK_OFFSET, and I think that name is more confusing than the original one. Also from the commit description it was not clear to me what that means, and why you lock the bytes in try_acquire_lock_qemu the way you do it. I think changing the name to QEMU_SHARE_LOCK_OFFSET is more clear.

That being said, I am not 100% sure I understood what exactly the shared permissions are.

The function suffers from a TOCTOU problem. There is no guarantee that the locks set on the file at the time of the state check are the same as at the time of the locking attempt. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

Rust typically doesn't use `get_` prefixes. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

All functions take a `LockGranularity` already and making them part of `LockGranularity` makes it easier to add associated helpers. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

By extracting the full file and file range locking, it's easier to add file lock handling that's not following this pattern. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

Avoids repeating the retry/error logic when adding new users of the function. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

Reference to pointer coercion doesn't happen for variadic functions like `libc::fcntl`. While this doesn't pose a problem by itself, it removes the check whether a reference can be coerced to the appropriate pointer type or mutability. By explicitly casting to a pointer, we ensure refactors don't accidentally break the safety requirements. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

arctic-alpaca · 2026-07-09T16:18:36Z

I really like your refactors and think it made the code more clear in the beginning, but then the naming and descriptions became a bit confusing to me.

You took the name CONSISTENT_READ from Qemu, but without describing why it is "consistent read" and not just "read", this is confusing I think. It makes sense when looking at the Qemu code base, which explains it, and also has the BLK_PERM_WRITE_UNCHANGED permission. As we only have read or write, I think calling it just QEMU_READ_BYTE is much less confusing. [...]

I'll drop the QEMU copied names and adapt them to be more self-explanatory 👍

That being said, I am not 100% sure I understood what exactly the shared permissions are.

Shared permissions are what isn't set in the 200 offset range. Every permission byte that is locked there is "unshared", so it may not be shared. The naming is a bit confusing, but I'm not sure how to improve it, do you have any suggestions?

QEMU uses locks on specific bytes for modeling file lock permissions. This commit only implements what's needed to model the permissions cloud-hypervisor currently uses. This is needed to be compatible with storage management systems that expect QEMU compatible file locking like NetApp's NFS implementation. QEMU uses two offsets from the start of the file to separate permissions and "unshared" permissions. "unshared" permissions are permissions that cannot be shared between lock holders [0] and can be considered exclusive. Starting from each offset, locks are placed with a length of one byte. Each corresponds to a specific permission [1][2]. The locking is done by first signaling intent by locking the required marker bytes. Next any conflicts with existing locks are detected and in the failure case, the locks are rolled back. Locking is done via `F_RDLCK`, which allows other parties to set the same locks and check for conflicts with `F_WRLCK`. [0]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/block/file-posix.c#L131-L134 [1]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/include/block/block-common.h#L392-L437 [2]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/block/file-posix.c#L868-L940 On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

amphi

Thanks for the additional documentation, I think it is way easier to understand the qemu-like locking now!

arctic-alpaca force-pushed the qemu-compatible-locking branch 2 times, most recently from f3b11b1 to 6763d5e Compare July 6, 2026 12:28

phip1611 reviewed Jul 7, 2026

View reviewed changes

Comment thread block/src/fcntl.rs

Comment thread block/src/fcntl.rs Outdated

Comment thread block/src/fcntl.rs

arctic-alpaca force-pushed the qemu-compatible-locking branch 2 times, most recently from 4925935 to dac9921 Compare July 7, 2026 10:37

arctic-alpaca marked this pull request as ready for review July 7, 2026 11:29

arctic-alpaca requested review from Coffeeri and amphi July 7, 2026 11:29

Coffeeri requested changes Jul 8, 2026

View reviewed changes

Comment thread block/src/fcntl.rs

Comment thread virtio-devices/src/block.rs Outdated

Comment thread block/src/fcntl.rs Outdated

Comment thread block/src/fcntl.rs Outdated

Comment thread block/src/fcntl.rs Outdated

arctic-alpaca force-pushed the qemu-compatible-locking branch 2 times, most recently from 3a963d2 to 0ccd243 Compare July 8, 2026 14:38

Coffeeri reviewed Jul 9, 2026

View reviewed changes

Comment thread block/src/fcntl.rs

arctic-alpaca force-pushed the qemu-compatible-locking branch from 0ccd243 to b0d5ab2 Compare July 9, 2026 13:14

Coffeeri approved these changes Jul 9, 2026

View reviewed changes

amphi reviewed Jul 9, 2026

View reviewed changes

Comment thread block/src/fcntl.rs

Comment thread block/src/fcntl.rs Outdated

Comment thread block/src/fcntl.rs

arctic-alpaca added 6 commits July 9, 2026 17:46

vmm: remove get_ prefix

d1e04b7

Rust typically doesn't use `get_` prefixes. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

vmm: move error handling into fcntl function

3d2b9e9

Avoids repeating the retry/error logic when adding new users of the function. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

arctic-alpaca force-pushed the qemu-compatible-locking branch 2 times, most recently from 3641609 to eca8c81 Compare July 9, 2026 16:14

arctic-alpaca added 2 commits July 10, 2026 10:36

vmm: set QemuCompatible to the default LockGranularityChoice

28aab6c

On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel <julian.schindel@cyberus-technology.de>

arctic-alpaca force-pushed the qemu-compatible-locking branch from eca8c81 to 28aab6c Compare July 10, 2026 08:37

arctic-alpaca requested a review from amphi July 10, 2026 08:37

amphi approved these changes Jul 10, 2026

View reviewed changes

arctic-alpaca merged commit 72c474d into cyberus-technology:gardenlinux Jul 10, 2026
19 checks passed

arctic-alpaca deleted the qemu-compatible-locking branch July 10, 2026 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

QEMU compatible file locking#180

QEMU compatible file locking#180
arctic-alpaca merged 8 commits into
cyberus-technology:gardenlinuxfrom
arctic-alpaca:qemu-compatible-locking

arctic-alpaca commented Jul 6, 2026 •

edited

Loading

Uh oh!

phip1611 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Coffeeri left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amphi left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arctic-alpaca commented Jul 9, 2026

Uh oh!

amphi left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

arctic-alpaca commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phip1611 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Coffeeri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amphi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arctic-alpaca commented Jul 9, 2026

Uh oh!

amphi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arctic-alpaca commented Jul 6, 2026 •

edited

Loading