On Tue, Feb 18, 2025 at 07:20:17PM +0100, Kevin Wolf wrote:
> +/// A request to a block driver
> +pub enum Request {
> +    Read { offset: u64, len: u64 },
> +}
> +
> +/// The target for a number of guest blocks, e.g. a location in a child node 
> or the information
> +/// that the described blocks are unmapped.
> +pub enum MappingTarget {
> +    /// The described blocks are unallocated. Reading from them yields zeros.
> +    Unmapped,
> +
> +    /// The described blocks are stored in a child node.
> +    Data {
> +        /// Child node in which the data is stored
> +        node: Arc<BdrvChild>,
> +
> +        /// Offset in the child node at which the data is stored
> +        offset: u64,
> +    },
> +}
> +
> +/// A mapping for a number of contiguous guest blocks
> +pub struct Mapping {
> +    /// Offset of the mapped blocks from the perspective of the guest
> +    pub offset: u64,
> +    /// Length of the mapping in bytes
> +    pub len: u64,
> +    /// Where the data for the described blocks is stored
> +    pub target: MappingTarget,
> +}
>  
>  /// A trait for writing block drivers.
>  ///
> @@ -37,6 +72,11 @@ unsafe fn open(
>  
>      /// Returns the size of the image in bytes
>      fn size(&self) -> u64;
> +
> +    /// Returns the mapping for the first part of `req`. If the returned 
> mapping is shorter than
> +    /// the request, the function can be called again with a shortened 
> request to get the mapping
> +    /// for the remaining part.
> +    async fn map(&self, req: &Request) -> io::Result<Mapping>;

Here are my thoughts about how the interface could evolve as more
functionality is added (beyond the scope of this patch). I'm sure you
have your own ideas that aren't in this patch series, so take it for
what it's worth.

The main point:
The current interface has Request, which tells the BlockDriver that this
is a Read. I think this interface is still based on I/O requests rather
than being about mappings. The caller should handle
read/write/discard/etc request logic themselves based on the mapping
information from the BlockDriver. Then the BlockDriver just worries
about mapping blocks rather than how to treat different types of
requests.

A sketch of the interface:
The interface consists of fetch(), alloc(), and update().

1. Fetch mappings located around a given range:
   fetch(offset, len) -> [Mapping]

   The returned array of mappings must cover <offset, len>, but it can
   also be larger. For example, it could return the entire extent that
   includes <offset, len>. Or it could even contain the <offset, len>
   mapping along with a bunch of other mappings.

   If the BlockDriver pays the cost of a metadata read operation to load
   mappings, then let's report all mappings even if they precede or
   follow the <offset, len> range. The exact amount of extra mappings
   reported depends on the BlockDriver implementation and the metadata
   layout, but in qcow2 it might be an L2 table, for example. The idea
   is that if the BlockDriver volunteers extra mapping information, then
   the caller can perform future operations without calling into the
   BlockDriver again.

   If it turns out that certain BlockDriver implementations don't
   benefit, that's okay, but let's include the flexibility to volunteer
   more mapping information than just <offset, len> in the interface.

2. Allocate new unmapped blocks:
   alloc(num_blocks) -> Result<(BdrvChild, Offset)>

   (A choice needs to be made whether the blocks are contiguous or not,
   I'm showing the contiguous function prototype here because it's
   simpler, but it could also support non-contiguous allocations.)

   These blocks are not mapped yet and the caller is expected to write
   to them eventually, populating them with data.

   I'm assuming the qcow2 crash consistency model where leaking clusters
   is acceptable here. Not sure whether this API makes sense for all
   other image formats, but please bear with me for the final part of
   the interface where everything fits together.

3. Update mapping:
   update(offset, len, MappingTarget) -> Result<()>

   This adds or changes mappings. It's the most complicated function and
   there are a lot of details about which inputs should be
   allowed/forbidden (e.g. creating completely new mappings, splitting
   existing mappings, toggling MappingTarget on an existing mapping).
   Perhaps in practice only a few valid combinations of inputs will be
   allowed rather than full ability to manipulate mappings.

   Anyway, here is how it can be used:
   - Allocating write. After blocks allocated with alloc() have been
     written with data, call update() to establish a mapping.
   - Discard. Change the MappingTarget of an existing mapping to unmap
     blocks. This frees blocks unless they should be anchored (aka
     pre-allocated).
   - Write to anchored blocks. If the mapping had blocks allocated but
     they were not mapped, then update() should be called after data has
     been written to the blocks to restore the mapping from anchored to
     normal mapped blocks.

The overall idea is that I/O requests are not part of the interface,
only mappings. The caller decides how to perform I/O requests and calls
the appropriate combination of fetch(), alloc(), and update() APIs to
ensure the necessary mapping state is present.

It will probably be necessary to add additional mapping information
indicating permissions, compression/encryption info, etc so that the
caller can process I/O requests rather than sending them into the
BlockDriver.

Stefan

Attachment: signature.asc
Description: PGP signature

Reply via email to