Skip to content

Commit

Permalink
disk_backend: remove GetLbaStatus (#413)
Browse files Browse the repository at this point in the history 
There is no support in any backend for querying LBA status, and the
trait for querying it is not really suitable for use with async disks.
All devices that opt into supporting LBA status queries just end up
using the default implementation, which reports that all blocks are
fully mapped.

Perhaps a future change will actually support these queries properly.
For now, move this functionality entirely into the SCSI emulator.
  • Loading branch information
@jstarks
jstarks authored Dec 2, 2024
1 parent 57ca806 commit e2935ef
Show file tree
Hide file tree
Showing 10 changed files with 93 additions and 132 deletions.
1 change: 1 addition & 0 deletions openhcl/underhill_core/src/dispatch/vtl2_settings_worker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,7 @@ fn make_disk_config_inner(
scsi_disk_size_in_bytes: disk_params.scsi_disk_size_in_bytes,
odx: disk_params.odx,
unmap: disk_params.unmap,
get_lba_status: true,
max_transfer_length: disk_params.max_transfer_length,
optimal_unmap_sectors: None, // TODO
})
Expand Down
84 changes: 0 additions & 84 deletions vm/devices/storage/disk_backend/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,6 @@ pub trait DiskIo: 'static + Send + Sync + Inspect {
None::<NoUnmap>
}

/// Optionally returns a trait object to issue get LBA status requests.
fn lba_status(&self) -> Option<&dyn GetLbaStatus> {
None
}

/// Optionally returns a trait object to issue persistent reservation
/// requests.
fn pr(&self) -> Option<&dyn pr::PersistentReservation> {
Expand Down Expand Up @@ -182,7 +177,6 @@ impl Disk {
fn inspect_extra(&self, resp: &mut inspect::Response<'_>) {
resp.field("disk_type", self.0.disk.disk_type())
.field("sector_count", self.0.disk.sector_count())
.field("supports_lba_status", self.0.disk.lba_status().is_some())
.field("supports_pr", self.0.disk.pr().is_some())
.field(
"optimal_unmap_sectors",
Expand Down Expand Up @@ -302,11 +296,6 @@ impl Disk {
}
}

/// Optionally returns a trait object to issue get LBA status requests.
pub fn lba_status(&self) -> Option<&dyn GetLbaStatus> {
self.0.disk.lba_status()
}

/// Optionally returns a trait object to issue persistent reservation
/// requests.
pub fn pr(&self) -> Option<&dyn pr::PersistentReservation> {
Expand Down Expand Up @@ -376,74 +365,6 @@ impl Unmap for DiskUnmap<'_> {
}
}

/// A trait to get LBA status and block index information.
pub trait GetLbaStatus {
/// Returns the block index information for the given file offset.
fn file_offset_to_device_block_index_and_length(
&self,
disk: &Disk,
_start_offset: u64,
_get_lba_status_range_length: u64,
_block_size: u64,
) -> DeviceBlockIndexInfo {
let sector_size = disk.sector_size() as u64;
let sector_count = disk.sector_count();
let disk_size = sector_size * sector_count;

// Treat fully allocation disk or fixed disk as one large block and just return
// enough descriptors from the LBA requested till the last LBA on disk.
//
// LbaPerBlock is a ULONG and technically with MAXULONG * 512 byte sectors,
// we can get upto 1.99 TB. The LBA descriptor also holds a ULONG
// LogicalBlockCount and can have an issue for larger than 2TB disks.
let lba_per_block = std::cmp::min(sector_count, u32::MAX.into());
let block_size_large = lba_per_block * sector_size;
let block_count = ((disk_size + block_size_large - 1) / block_size_large) as u32;
DeviceBlockIndexInfo {
first_partial_block_size: 0,
first_full_block_index: 0,
block_count,
last_partial_block_size: 0,
lba_per_block,
}
}

/// Returns the LBA status for the given block number.
fn get_block_lba_status(
&self,
_block_number: u32,
_leaf_node_state_only: bool,
) -> Result<LbaStatus, DiskError> {
Ok(LbaStatus::Mapped)
}
}

/// The LBA status of a block.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum LbaStatus {
/// The block is mapped.
Mapped,
/// The block is deallocated.
Deallocated,
/// The block is anchored.
Anchored,
}

/// Result of a get LBA status request.
#[derive(Debug, Default, Copy, Clone)]
pub struct DeviceBlockIndexInfo {
/// The size of the first partial block.
pub first_partial_block_size: u32,
/// The index of the first full block.
pub first_full_block_index: u32,
/// The number of blocks.
pub block_count: u32,
/// The size of the last partial block.
pub last_partial_block_size: u32,
/// The number of LBAs per block.
pub lba_per_block: u64,
}

/// Unmap disk sectors that are no longer in use.
pub trait Unmap: Send + Sync {
/// Unmaps the specified sectors.
Expand Down Expand Up @@ -505,7 +426,6 @@ trait DynDisk: Send + Sync + Inspect {
fn unmap(&self, sector_offset: u64, sector_count: u64, block_level_only: bool) -> IoFuture<'_>;

fn optimal_unmap_sectors(&self) -> u32;
fn lba_status(&self) -> Option<&dyn GetLbaStatus>;
fn pr(&self) -> Option<&dyn pr::PersistentReservation>;
fn eject(&self) -> IoFuture<'_>;

Expand Down Expand Up @@ -556,10 +476,6 @@ impl<T: DiskIo> DynDisk for T {
self.unmap().unwrap().optimal_unmap_sectors()
}

fn lba_status(&self) -> Option<&dyn GetLbaStatus> {
self.lba_status()
}

fn pr(&self) -> Option<&dyn pr::PersistentReservation> {
self.pr()
}
Expand Down
7 changes: 0 additions & 7 deletions vm/devices/storage/disk_blockdevice/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use disk_backend::resolve::ResolveDiskParameters;
use disk_backend::resolve::ResolvedDisk;
use disk_backend::DiskError;
use disk_backend::DiskIo;
use disk_backend::GetLbaStatus;
use disk_backend::Unmap;
use fs_err::PathExt;
use guestmem::MemoryRead;
Expand Down Expand Up @@ -527,10 +526,6 @@ impl DiskIo for BlockDevice {
(self.optimal_unmap_sectors != 0).then_some(self)
}

fn lba_status(&self) -> Option<&dyn GetLbaStatus> {
Some(self)
}

fn pr(&self) -> Option<&dyn PersistentReservation> {
if self.supports_pr {
Some(self)
Expand Down Expand Up @@ -710,8 +705,6 @@ impl Unmap for BlockDevice {
}
}

impl GetLbaStatus for BlockDevice {}

#[async_trait::async_trait]
impl PersistentReservation for BlockDevice {
fn capabilities(&self) -> ReservationCapabilities {
Expand Down
5 changes: 0 additions & 5 deletions vm/devices/storage/disk_crypt/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,6 @@ impl DiskIo for CryptDisk {
self.inner.unmap()
}

/// Optionally returns a trait object to issue get LBA status requests.
fn lba_status(&self) -> Option<&dyn disk_backend::GetLbaStatus> {
self.inner.lba_status()
}

/// Optionally returns a trait object to issue persistent reservation
/// requests.
fn pr(&self) -> Option<&dyn disk_backend::pr::PersistentReservation> {
Expand Down
7 changes: 0 additions & 7 deletions vm/devices/storage/disk_nvme/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use async_trait::async_trait;
use disk_backend::pr;
use disk_backend::DiskError;
use disk_backend::DiskIo;
use disk_backend::GetLbaStatus;
use disk_backend::MediumErrorDetails;
use disk_backend::Unmap;
use inspect::Inspect;
Expand Down Expand Up @@ -70,10 +69,6 @@ impl DiskIo for NvmeDisk {
self.namespace.supports_dataset_management().then_some(self)
}

fn lba_status(&self) -> Option<&dyn GetLbaStatus> {
Some(self)
}

fn pr(&self) -> Option<&dyn pr::PersistentReservation> {
(u8::from(self.namespace.reservation_capabilities()) != 0).then_some(self)
}
Expand Down Expand Up @@ -155,8 +150,6 @@ impl DiskIo for NvmeDisk {
}
}

impl GetLbaStatus for NvmeDisk {}

impl Unmap for NvmeDisk {
async fn unmap(
&self,
Expand Down
4 changes: 0 additions & 4 deletions vm/devices/storage/disk_prwrap/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,6 @@ impl DiskIo for DiskWithReservations {

// TODO: Implement unmap

fn lba_status(&self) -> Option<&dyn disk_backend::GetLbaStatus> {
self.inner.lba_status()
}

fn pr(&self) -> Option<&dyn pr::PersistentReservation> {
Some(self)
}
Expand Down
7 changes: 0 additions & 7 deletions vm/devices/storage/disk_striped/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use disk_backend::resolve::ResolvedDisk;
use disk_backend::Disk;
use disk_backend::DiskError;
use disk_backend::DiskIo;
use disk_backend::GetLbaStatus;
use disk_backend::Unmap;
use disk_backend_resources::StripedDiskHandle;
use futures::future::join_all;
Expand Down Expand Up @@ -368,10 +367,6 @@ impl DiskIo for StripedDisk {
self.supports_unmap.then_some(self)
}

fn lba_status(&self) -> Option<&dyn GetLbaStatus> {
Some(self)
}

fn disk_id(&self) -> Option<[u8; 16]> {
None
}
Expand Down Expand Up @@ -572,8 +567,6 @@ impl Unmap for StripedDisk {
}
}

impl GetLbaStatus for StripedDisk {}

async fn await_all_and_check<T, E>(futures: T) -> Result<(), E>
where
T: IntoIterator,
Expand Down
96 changes: 83 additions & 13 deletions vm/devices/storage/scsidisk/src/getlbastatus.rs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

//! Support for the SCSI "Get LBA Status" command.
//!
//! Currently, this command just returns that all blocks are "mapped".
use super::ScsiError;
use super::SimpleScsiDisk;
use disk_backend::LbaStatus;
use disk_backend::Disk;
use disk_backend::DiskError;
use guestmem::MemoryWrite;
use scsi::AdditionalSenseCode;
use scsi_buffers::RequestBuffers;
Expand All @@ -13,6 +18,72 @@ use zerocopy::AsBytes;
use zerocopy::FromBytes;
use zerocopy::FromZeroes;

/// Result of a get LBA status request.
#[derive(Debug, Default, Copy, Clone)]
struct DeviceBlockIndexInfo {
/// The size of the first partial block.
first_partial_block_size: u32,
/// The index of the first full block.
first_full_block_index: u32,
/// The number of blocks.
block_count: u32,
/// The size of the last partial block.
#[allow(dead_code)]
last_partial_block_size: u32,
/// The number of LBAs per block.
lba_per_block: u64,
}

/// The LBA status of a block.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum LbaStatus {
/// The block is mapped.
Mapped,
/// The block is deallocated.
#[allow(dead_code)]
Deallocated,
/// The block is anchored.
#[allow(dead_code)]
Anchored,
}

/// Returns the block index information for the given file offset.
fn file_offset_to_device_block_index_and_length(
disk: &Disk,
_start_offset: u64,
_get_lba_status_range_length: u64,
_block_size: u64,
) -> DeviceBlockIndexInfo {
let sector_size = disk.sector_size() as u64;
let sector_count = disk.sector_count();
let disk_size = sector_size * sector_count;

// Treat fully allocation disk or fixed disk as one large block and just return
// enough descriptors from the LBA requested till the last LBA on disk.
//
// LbaPerBlock is a ULONG and technically with MAXULONG * 512 byte sectors,
// we can get upto 1.99 TB. The LBA descriptor also holds a ULONG
// LogicalBlockCount and can have an issue for larger than 2TB disks.
let lba_per_block = std::cmp::min(sector_count, u32::MAX.into());
let block_size_large = lba_per_block * sector_size;
let block_count = ((disk_size + block_size_large - 1) / block_size_large) as u32;
DeviceBlockIndexInfo {
first_partial_block_size: 0,
first_full_block_index: 0,
block_count,
last_partial_block_size: 0,
lba_per_block,
}
}

/// Returns the LBA status for the given block number.
fn get_block_lba_status(
_block_number: u32,
_leaf_node_state_only: bool,
) -> Result<LbaStatus, DiskError> {
Ok(LbaStatus::Mapped)
}

impl SimpleScsiDisk {
pub(crate) fn handle_get_lba_status(
&self,
Expand Down Expand Up @@ -57,8 +128,7 @@ impl SimpleScsiDisk {

let mut lba_count_remaining = total_lba_count_requested;

let lba_status = self.disk.lba_status().unwrap();
let mut block_index_info = lba_status.file_offset_to_device_block_index_and_length(
let mut block_index_info = file_offset_to_device_block_index_and_length(
&self.disk,
start_offset,
get_lba_status_range_length,
Expand Down Expand Up @@ -118,11 +188,11 @@ impl SimpleScsiDisk {
// Get the LBA status for the very first block.
let leaf_node_state_only =
request.srb_flags & scsi::SRB_FLAGS_CONSOLIDATEABLE_BLOCKS_ONLY != 0;
let mut provisioning_status =
match lba_status.get_block_lba_status(block_number, leaf_node_state_only) {
Ok(status) => status,
Err(e) => return Err(ScsiError::Disk(e)),
};
let mut provisioning_status = match get_block_lba_status(block_number, leaf_node_state_only)
{
Ok(status) => status,
Err(e) => return Err(ScsiError::Disk(e)),
};

let mut provisioning_status_in_previous_block;
let mut lba_count_in_current_block;
Expand Down Expand Up @@ -150,11 +220,11 @@ impl SimpleScsiDisk {
while block_number < block_number_max {
// Get the LBA status for the next block.
// Usually it's a full block except when we get to the last block.
provisioning_status =
match lba_status.get_block_lba_status(block_number, leaf_node_state_only) {
Ok(status) => status,
Err(e) => return Err(ScsiError::Disk(e)),
};
provisioning_status = match get_block_lba_status(block_number, leaf_node_state_only)
{
Ok(status) => status,
Err(e) => return Err(ScsiError::Disk(e)),
};

// This block has a different status from what we are looking for.
// Break out of here so we can composite a new LBA_STATUS_DESCRIPTOR.
Expand Down
Loading

0 comments on commit e2935ef

Please sign in to comment.