diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index f5cb626c00..7184fa8182 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -248,6 +248,8 @@ impl LockGranularity { let _ = self.release_unneeded_locks_qemu(file, current_lock_status); return Err(error); } + + self.release_unneeded_locks_qemu(file, lock_type)?; Ok(()) } @@ -369,3 +371,31 @@ impl FromStr for LockGranularityChoice { } } } + +#[cfg(test)] +mod tests { + use std::fs::OpenOptions; + + use vmm_sys_util::tempfile::TempFile; + + use super::{LockGranularity, LockType}; + + #[test] + fn qemu_lock_downgrade_allows_another_reader() { + let disk = TempFile::new().unwrap(); + let other_reader = OpenOptions::new() + .read(true) + .write(true) + .open(disk.as_path()) + .unwrap(); + let lock = LockGranularity::QemuCompatible; + + lock.try_acquire_lock(disk.as_file(), LockType::Write, LockType::Unlock) + .unwrap(); + lock.try_acquire_lock(disk.as_file(), LockType::Read, LockType::Write) + .unwrap(); + + lock.try_acquire_lock(&other_reader, LockType::Read, LockType::Unlock) + .unwrap(); + } +} diff --git a/virtio-devices/src/block.rs b/virtio-devices/src/block.rs index edb3650ab0..b674d4fad6 100644 --- a/virtio-devices/src/block.rs +++ b/virtio-devices/src/block.rs @@ -13,7 +13,7 @@ use std::collections::{BTreeMap, HashMap, VecDeque}; use std::num::Wrapping; use std::ops::Deref; use std::os::unix::io::AsRawFd; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, AtomicU8, AtomicU64, AtomicUsize, Ordering}; use std::sync::{Arc, Barrier}; use std::time::{Duration, Instant}; @@ -979,16 +979,20 @@ impl Block { has_feature(self.features(), VIRTIO_BLK_F_RO.into()) } - /// Returns the granularity for the advisory lock for this disk. - fn lock_granularity(&mut self) -> LockGranularity { + /// Returns the configured advisory lock granularity for `disk_image`. + fn lock_granularity( + &self, + disk_image: &dyn AsyncFullDiskFile, + disk_path: &Path, + ) -> LockGranularity { match self.lock_granularity_choice { LockGranularityChoice::Full => LockGranularity::WholeFile, LockGranularityChoice::ByteRange => { // Byte range lock covering [0, max(logical, physical)) // logical > physical for sparse files, physical > logical // for small dense files due to filesystem block rounding. - let logical = self.disk_image.logical_size(); - let physical = self.disk_image.physical_size(); + let logical = disk_image.logical_size(); + let physical = disk_image.physical_size(); match (logical, physical) { (Ok(l), Ok(p)) => LockGranularity::ByteRange(0, max(l, p)), (Ok(l), Err(_)) => LockGranularity::ByteRange(0, l), @@ -998,7 +1002,7 @@ impl Block { warn!( "Can't get disk size for id={},path={}, falling back to {:?}: error: {e}", self.id, - self.disk_path.display(), + disk_path.display(), fallback ); fallback @@ -1009,46 +1013,63 @@ impl Block { } } - /// Tries to set an advisory lock for the corresponding disk image. - pub fn try_lock_image(&mut self) -> Result<()> { - let lock_type = match self.read_only() { - true => LockType::Read, - false => LockType::Write, - }; - let granularity = self.lock_granularity(); + /// Acquires an advisory lock for an arbitrary disk backend. + fn try_lock_disk_image( + &self, + disk_image: &dyn AsyncFullDiskFile, + disk_path: &Path, + lock_type: LockType, + current_lock: LockType, + ) -> Result<()> { + let granularity = self.lock_granularity(disk_image, disk_path); debug!( "Attempting to acquire {lock_type:?} lock for disk image: id={},path={},granularity={granularity:?}", self.id, - self.disk_path.display() + disk_path.display() ); - let fd = self.disk_image.fd(); + let fd = disk_image.fd(); granularity - .try_acquire_lock(&fd, lock_type, self.held_lock) + .try_acquire_lock(&fd, lock_type, current_lock) .map_err(|error| { error!( "Cannot acquire {lock_type:?} lock for disk image: id={},path={},granularity={granularity:?}", self.id, - self.disk_path.display() + disk_path.display() ); Error::LockDiskImage { - path: self.disk_path.clone(), + path: disk_path.to_path_buf(), error, lock_type, } })?; - self.held_lock = lock_type; info!( "Acquired {lock_type:?} lock for disk image id={},path={}", self.id, - self.disk_path.display() + disk_path.display() ); Ok(()) } + /// Tries to set an advisory lock for the corresponding disk image. + pub fn try_lock_image(&mut self) -> Result<()> { + let lock_type = match self.read_only() { + true => LockType::Read, + false => LockType::Write, + }; + self.try_lock_disk_image( + self.disk_image.as_ref(), + &self.disk_path, + lock_type, + self.held_lock, + )?; + self.held_lock = lock_type; + Ok(()) + } + /// Releases the advisory lock held for the corresponding disk image. pub fn unlock_image(&mut self) -> Result<()> { - let granularity = self.lock_granularity(); + let granularity = self.lock_granularity(self.disk_image.as_ref(), &self.disk_path); // It is very unlikely that this fails; // Should we remove the Result to simplify the error propagation on