block: abstractions for request parsing and execution #29

lauralt · 2020-11-03T17:41:00Z

This RFC proposes the following abstractions for block device:

Request -> which handles the request parsing from a descriptor chain and keeps the necessary information for the request execution.
RequestExecutor -> which handles the request execution and wraps the block device file to which the requests are sent. The reason for having a separate abstraction for execution is the fact that there can be multiple ways to execute a given request (an example is given in the corresponding commit).

Request is mostly based on the cloud hypervisor implementation as it handles the case of multiple data descriptors in the chain (as opposed to firecracker, where only one data descriptor is assumed) (side note: writeback field is not kept since it is most likely a feature of the block device and/or a configuration option for the executor, rather than of the request object itself).

The downside to this Request proposal is we are still making assumptions which represent a particular case, but they are only used as part of the implementation, and do not constrain the interface of Request. For example, our implementation starts by assuming we get separate descriptors for the request header (i.e. type, sector, etc), data regions, and status. However, these assumptions are only leveraged within the parse method implementation, which is opaque to users.
In the future we might take into consideration the Reader and Writer abstractions from crosvm which just follow the only hard requirement from the virtio specification: The driver MUST place any device-writable descriptor elements after any device-readable descriptor elements. If we later switch to something similar to the crosvm approach, we can keep the same request interface, where the data regions are a sequence of (GuestAddress, length) pairs.

This PR misses a lot of tests, it contains some examples for request parsing and execution, but they are far from a thorough testing.
If the overall aproach makes sense I'll add more tests (also the documentation can be improved).

The last commit is a discard/write zeroes implementation tentative (more or less the one from crosvm at this moment :D) and it wasn't tested in any way yet, so it can be omitted as well for now (but any suggestion is welcome).

jiangliu · 2020-11-04T06:14:50Z

src/block/request.rs

+        // We will write an u32 status here after executing the request.
+        let _ = desc_chain
+            .memory()
+            .checked_offset(status_desc.addr(), mem::size_of::<u32>())


The Virtio spec defines that the status field is a u8, so should we check "u32" or "u8"?

The kernel uses a u8 too: https://elixir.bootlin.com/linux/latest/source/drivers/block/virtio_blk.c#L74

That's a very good point. We're probably getting away with this because the driver allocates buffers larger than 1 bytes and the little endianess ensures the status byte gets written to the correct position either way. We should write the status as an u8 if we're doing otherwise (this seems to be just a check). Also, I once again think we can omit the checked_offset validations altogether.

It was a miss from me, fixed it, thanks!

alxiord

There have been previous mentions in other PRs about which virtio spec we should adhere to, and some of my comments here also touch on that - I've been looking over the virtio 1.0 and 1.1 specs reviewing this, and also through the driver code in the latest kernel. Stuff differs. Should we use this opportunity to propose a support model (i.e. which version of the virtio spec) applicable to this crate?

alxiord · 2020-11-06T13:39:16Z

src/block/request.rs

+    fn from(value: u32) -> Self {
+        match value {
+            VIRTIO_BLK_T_IN => RequestType::In,
+            VIRTIO_BLK_T_OUT => RequestType::Out,


Looking in the (latest) kernel: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/virtio_blk.h (as the virtio spec appears to be sadly behind):

VIRTIO_BLK_T_OUT may be combined with VIRTIO_BLK_T_SCSI_CMD or VIRTIO_BLK_T_BARRIER.

Should we factor this in when parsing block requests? I think we could return Unsupported for VIRTIO_BLK_T_SCSI_CMD | VIRTIO_BLK_T_OUT but what about VIRTIO_BLK_T_BARRIER?

Hmm, looks like these are legacy features/ops which we don't have to support (I'm looking at 5.2.3.1 and 5.2.6.3 in the 1.1 standard)

I updated the module doc to state that we don't support the legacy interface.
I think we could add a mention about the supported virtio versions in the README as well.

alxiord · 2020-11-06T13:40:28Z

src/block/request.rs

+const VIRTIO_BLK_T_OUT: u32 = 1;
+const VIRTIO_BLK_T_FLUSH: u32 = 4;
+const VIRTIO_BLK_T_DISCARD: u32 = 11;
+const VIRTIO_BLK_T_WRITE_ZEROES: u32 = 13;


What about VIRTIO_BLK_T_GET_ID?

This does not appear to be in the published standard yet, but looks like we can add support for it oasis-tcs/virtio-spec#63. One question is whether we should support the operation in this PR or a follow up.

If it's ok, I would add it in a second PR (with discard, write zeroes), this one looks already big enough.

alxiord · 2020-11-06T15:25:33Z

src/queue.rs

@@ -733,6 +733,19 @@ pub(crate) mod tests {
        VolatileRef, VolatileSlice,
    };

+    impl Descriptor {
+        // Only available to unit tests within the local crate. Should this exist at the public


No, I don't think so.

alxiord · 2020-11-06T15:34:15Z

src/block/request.rs

+        let mut request = Request {
+            request_type: RequestType::from(request_header.request_type),
+            data: Vec::new(),
+            sector: request_header.sector,


There's a preliminary validation that we can do on this level (spec):

A driver MUST set sector to 0 for a VIRTIO_BLK_T_FLUSH request. A driver SHOULD NOT include any data in a VIRTIO_BLK_T_FLUSH request.

This is a very good item to add to the larger discussion of whether we should check for strict adherence to the standard, or we can omit validations which don't have direct implications on the device model (for example, whether sector is set to 0 or not above has no actual impact on the flush operation). Either way, we should stil document these things in comments at least. Same goes for the is_write_only, !is_write_only checks below.

After giving it some more thought, I think it makes sense to validate the MUSTs from the spec and also the descriptors that we are expecting to be the ones associated to the request header, the data and the status. Since we don't really do a nice separation between the memory regions from the device-readable descriptors and from the device-writable ones before parsing the request's chain, some checks here are worth it I think.
Does it make sense? 😕

It makes sense to me. Ultimately, the only reasons to consider not doing some of the validations are the performance impact and increasing code complexity too much. For now, I think our only worry is the former, and we can run some tests after we have an initial block device implementation in place as well (I guess we could also do a synthetic test that tries to parse many requests, but that might not be super relevant for real-world scenarios).

alxiord · 2020-11-06T15:38:00Z

src/block/request.rs

+        // We will write an u32 status here after executing the request.
+        let _ = desc_chain
+            .memory()
+            .checked_offset(status_desc.addr(), mem::size_of::<u32>())


The kernel uses a u8 too: https://elixir.bootlin.com/linux/latest/source/drivers/block/virtio_blk.c#L74

alxiord · 2020-11-06T15:49:00Z

src/block/request.rs

+                        Err(e) => Err(ExecuteError::Flush(e)),
+                    }
+                }
+                RequestType::Discard | RequestType::WriteZeroes => {}


Suggested change

RequestType::Discard | RequestType::WriteZeroes => {}

RequestType::Discard | RequestType::WriteZeroes => { unimplemented!() }

I think it's better to return Unsupported here for now.

alxiord · 2020-11-06T15:49:18Z

src/block/request.rs

+                    }
+                }
+                RequestType::Discard | RequestType::WriteZeroes => {}
+                RequestType::Unsupported(t) => return Err(ExecuteError::Unsupported(t)),


This is also cleaner if moved outside the for

I think that moving this outside the for would prob still be somewhat ugly due to needing some arm at the end anyway so the match is exhaustive :-s

I like it better with for inside.

src/block/request.rs

alxiord · 2020-11-06T16:12:59Z

src/block/request.rs

@@ -181,6 +189,7 @@ impl Request {
    }

    /// Parses a request from a given `desc_chain`.
+    /// TODO split this gigantic function in smaller pieces.


Hmm looks like the Discard and WriteZeroes support is noticeably more complex than the rest. Does it make sense to have two PRs? We can focus first on the previous commits and then create another PR for the rest.

Yep, sounds good, I'll open another PR with these ones and GetDeviceId.

src/block/request.rs

alexandruag

Hi! Left some comments, and will do another pass to focus more on the final commit in particular.

alexandruag · 2020-11-09T09:26:03Z

src/block/request.rs

+const VIRTIO_BLK_T_OUT: u32 = 1;
+const VIRTIO_BLK_T_FLUSH: u32 = 4;
+const VIRTIO_BLK_T_DISCARD: u32 = 11;
+const VIRTIO_BLK_T_WRITE_ZEROES: u32 = 13;


This does not appear to be in the published standard yet, but looks like we can add support for it oasis-tcs/virtio-spec#63. One question is whether we should support the operation in this PR or a follow up.

alexandruag · 2020-11-09T09:35:17Z

src/block/request.rs

+    fn from(value: u32) -> Self {
+        match value {
+            VIRTIO_BLK_T_IN => RequestType::In,
+            VIRTIO_BLK_T_OUT => RequestType::Out,


Hmm, looks like these are legacy features/ops which we don't have to support (I'm looking at 5.2.3.1 and 5.2.6.3 in the 1.1 standard)

alexandruag · 2020-11-09T09:40:54Z

src/block/request.rs

+        // We will write an u32 status here after executing the request.
+        let _ = desc_chain
+            .memory()
+            .checked_offset(status_desc.addr(), mem::size_of::<u32>())


That's a very good point. We're probably getting away with this because the driver allocates buffers larger than 1 bytes and the little endianess ensures the status byte gets written to the correct position either way. We should write the status as an u8 if we're doing otherwise (this seems to be just a check). Also, I once again think we can omit the checked_offset validations altogether.

alexandruag · 2020-11-09T09:46:27Z

src/block/request.rs

+                }
+
+                // Check that the address of the data descriptor is valid in guest memory.
+                let _ = desc_chain


I wonder whether we should do the check_offset validations, because the memory model will validate accesses when they happen anyway, so we end up with double checking (and more verbose code).

I think we should keep them (I'm not so sure about the status one, that one is pretty dummy) because, even if we replace the memory accesses with read_exact_from and write_all_to, those will indeed fail but some part of data might be transferred (which I assume it's not the behavior that we want).

alexandruag · 2020-11-09T09:53:33Z

src/block/request.rs

+        let mut request = Request {
+            request_type: RequestType::from(request_header.request_type),
+            data: Vec::new(),
+            sector: request_header.sector,


This is a very good item to add to the larger discussion of whether we should check for strict adherence to the standard, or we can omit validations which don't have direct implications on the device model (for example, whether sector is set to 0 or not above has no actual impact on the flush operation). Either way, we should stil document these things in comments at least. Same goes for the is_write_only, !is_write_only checks below.

alexandruag · 2020-11-09T10:34:00Z

src/block/request.rs

+                        .map_err(ExecuteError::Write)?;
+                }
+                RequestType::Flush => {
+                    return match self.fsync() {


Doesn't this work inside the loop as well?

alexandruag · 2020-11-09T10:34:25Z

src/block/request.rs

+                        Err(e) => Err(ExecuteError::Flush(e)),
+                    }
+                }
+                RequestType::Discard | RequestType::WriteZeroes => {}


I think it's better to return Unsupported here for now.

alexandruag · 2020-11-09T10:35:49Z

src/block/request.rs

+                    }
+                }
+                RequestType::Discard | RequestType::WriteZeroes => {}
+                RequestType::Unsupported(t) => return Err(ExecuteError::Unsupported(t)),


I think that moving this outside the for would prob still be somewhat ugly due to needing some arm at the end anyway so the match is exhaustive :-s

alexandruag · 2020-11-09T10:40:01Z

src/block/request.rs

@@ -181,6 +189,7 @@ impl Request {
    }

    /// Parses a request from a given `desc_chain`.
+    /// TODO split this gigantic function in smaller pieces.


Hmm looks like the Discard and WriteZeroes support is noticeably more complex than the rest. Does it make sense to have two PRs? We can focus first on the previous commits and then create another PR for the rest.

alexandruag · 2020-11-09T10:54:58Z

src/block/request.rs

+    }
+
+    /// Executes `request` Request on `D` and `mem`.
+    pub fn execute<M: GuestMemory>(


From a high level, it seems more convenient if the executor writes the status of the request as well. Does that make sense? I that case we probably no longer need Unsupported as an error variant, but haven't really looked into the details.

Hmm, you mean in the same function? If yes, I wouldn't complicate the map_err()s to also take care of writing the status. Or are you thinking of writing the VIRTIO_BLK_S_UNSUPP status in execute where necessary and VIRTIO_BLK_S_IOERRs in some other fn?

bonzini

I think the use of Deref and DerefMut in RequestExecutor is a symptom of a wrong design. In particular, RequestExecutor actually has two parts:

caching the number of sectors in the backend, and converting virtio-blk's offset/count API to separate seek and read/write operations. This can be implemented in a trait SectorAccess with a corresponding implementation for Read + Write + Seek + PunchHole + WriteZeroes + FileSync:

pub trait SectorAccess: FileSync {
    // Notice that the start/offset API allows for interior mutability
    // This will be needed in order to process multiple requests
    // in parallel in the future
    pub fn read(&self, sector_start: u64, value: &mut[u8] ) -> Result<usize>;
    pub fn write(&self, sector_start: u64, value: &[u8]) -> Result<usize>;
    pub fn unmap(&self, sector_start: u64, sector_count: usize) -> Error;
    pub fn punch_hole(&self, sector_start: u64, sector_count: usize) -> Error;

    pub fn num_sectors(&self) -> u64;
    pub fn sector_size(&self) -> u32;
}

pub struct DiskFile<D: Read + Write + Seek + PunchHole + WriteZeroes + FileSync> {
    // Mutex needed to avoid interruption between seek and read
    file: Mutex<D>,
    num_sectors: u64
}

impl<D: Read + Write + Seek + PunchHole + WriteZeroes + FileSync> SectorAccess for DiskFile<D> { /* ... */ }

This allows SectorAccess implementations that do not use the Rust std::io traits, for example using io_uring, or other APIs that have native support for offset/count.

If you want, you can also add a file pointer field to DiskFile, and use it to implement Read/Write/Seek, for example:

impl Read for DiskFile {
     fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
         let g = self.file.lock().unwrap();
         g.seek(SeekFrom::Start(self.pointer));
         let nread = g.read(buf)?;
         self.pointer += nread;
         Ok(nread)
    }
}

impl Seek for DiskFile {
     fn seek(&mut self, pos: SeekFrom) -> Result<u64> {
         let g = self.file.lock().unwrap();
         let oldpos = self.pointer;
         self.pointer = g.seek(pos)?;
         Ok(oldpos)
    }
}

the parsing of requests and their forwarding to a SectorAccess. This would be a separate class that operates on a SectorAccess through a reference or an Rc/Arc:

// B can be an &A, Rc<A>, Arc<A> etc.
pub struct RequestParser<A: SectorAccess, B: Deref<Target = A>> {
    backend: B,
}

Once you split the backend this way, you don't need anymore to access the disk through the RequestExecutor. Instead you can just clone the Rc/Arc and let interior mutability do its job.

alexandruag · 2020-11-09T14:09:25Z

Hi Paolo! The Deref implementation for RequestExecutor can be removed, and the parsing of request objects (which is a generic operation in itself) is already separated from the execution part (which can be implemented in multiple ways). RequestExecutor (I guess a better name would have been SimpleRequestExecutor) represents a simple initial implementation of block request execution, while we figure out what the best approach is towards a more powerful abstraction that, like you mentioned, works with stuff like io_uring and others.

The simple implementation can later be superseded (or coexist) with more complex solutions. Do you think it's reasonable to start with the simplified approach and then add an advanced abstraction as well?

bonzini · 2020-11-09T16:43:22Z

RequestExecutor (I guess a better name would have been SimpleRequestExecutor) represents a simple initial implementation of block request execution, while we figure out what the best approach is towards a more powerful abstraction that, like you mentioned, works with stuff like io_uring and others.

What I don't like is that the match statement on requests is so much tied to Read and Write, which are the "wrong" API to access files for a virtio-blk backend (i.e. an API that has to be worked around). It happens to work because vm-memory's Bytes trait supports Read and Write, but it's basically impossible to turn it into a scalable implementation without a complete rewrite.

I could buy renaming RequestExecutor into StdIoBackend, I guess. I'm just not sure if it's a good idea to base the reference implementation on suboptimal traits from std::io instead of defining our own.

alexandruag · 2020-11-10T12:26:42Z

The proposed request executor is more of a minimal viable implementation than a reference one (we can make this even clearer in the documentation), which can already enable use cases like Firecracker. The original intention was to have multiple solutions for block request execution, based on different sets of simplifying assumptions. We def want to add more powerful abstractions as well, but it would be great to start with this very simple one that’s immediately useful. We’re committed to evolving and improving things as we figure out what the best path forward looks like. Is that ok with you?

lauralt

Hello, thanks for the comments! I renamed RequestExecutor to StdIoBackend and updated the documentation to something that is hopefully clearer (also removed the Derefs). I like the SectorAccess proposal and played a bit with it, but it doesn't look so straightforward (at least for me), maybe we can focus on this abstraction in a future PR (when we'll define what executors we plan to have, for example).

I removed for now the last commit and will open another PR with discard, write zeroes and get device id ops if it's okay for everyone. I also added more tests and the documentation is better now (I hope).

lauralt · 2020-11-12T06:27:14Z

src/block/request.rs

+const VIRTIO_BLK_T_OUT: u32 = 1;
+const VIRTIO_BLK_T_FLUSH: u32 = 4;
+const VIRTIO_BLK_T_DISCARD: u32 = 11;
+const VIRTIO_BLK_T_WRITE_ZEROES: u32 = 13;


If it's ok, I would add it in a second PR (with discard, write zeroes), this one looks already big enough.

src/block/request.rs

lauralt · 2020-11-12T06:33:26Z

src/block/request.rs

+
+/// Trait that keeps as supertraits the ones that are necessary for virtio block
+/// request execution on a block device (v1.1 virtio specification).
+pub trait DiskFile: Read + Write + Seek + FileSync + PunchHole + WriteZeroes {}


I prefer it like this too. Renamed it.

lauralt · 2020-11-12T06:35:30Z

src/block/request.rs

+    fn from(value: u32) -> Self {
+        match value {
+            VIRTIO_BLK_T_IN => RequestType::In,
+            VIRTIO_BLK_T_OUT => RequestType::Out,


I updated the module doc to state that we don't support the legacy interface.
I think we could add a mention about the supported virtio versions in the README as well.

lauralt · 2020-11-12T06:35:41Z

src/block/request.rs

+    }
+
+    /// Returns the (address, len) pairs where the request data is in the guest memory.
+    pub fn data(&self) -> &Vec<(GuestAddress, u32)> {


lauralt · 2020-11-12T07:18:26Z

src/block/request.rs

+            .map_err(ExecuteError::Seek)?;
+        let mut len = 0;
+
+        for (data_addr, data_len) in request.data() {


I moved the fors inside In and Out branches.
Hmm, maybe a warning would make sense, but I'm not so sure, I left it as it is for now.

lauralt · 2020-11-12T07:22:44Z

src/block/request.rs

+                    mem.write_to(*data_addr, self.deref_mut(), *data_len as usize)
+                        .map_err(ExecuteError::Write)?;
+                }
+                RequestType::Flush => {


I added a features field as well.
From what I understand, only fsync guarantees that data reaches the disk.

lauralt · 2020-11-12T07:22:51Z

src/block/request.rs

+                        .map_err(ExecuteError::Write)?;
+                }
+                RequestType::Flush => {
+                    return match self.fsync() {


lauralt · 2020-11-12T07:24:31Z

src/block/request.rs

+                    }
+                }
+                RequestType::Discard | RequestType::WriteZeroes => {}
+                RequestType::Unsupported(t) => return Err(ExecuteError::Unsupported(t)),


I like it better with for inside.

lauralt · 2020-11-12T07:54:28Z

src/lib.rs

 mod device;
 mod queue;

+pub use self::block::*;


Makes sense, I also changed mod block to pub (at least until it gets clear what stays in vm-virtio), but just curious, wasn't there some kind of rule that you either make a module public or just some items from it (but not both of them). Should we try to stick to it? :-?

alexandruag

Cool, left some comments for now, and I'll have a better look at the tests during a second pass.

alexandruag · 2020-11-16T11:39:38Z

src/block/request.rs

+//! approach.
+//!
+//! # Note
+//! Wo offer support only for virtio v1.0+


Maybe we can take advantage of this PR to make the note more generic and move it to the crate level, since this applies to everything in vm-virtio for now.

I moved the note in README.

alexandruag · 2020-11-16T11:41:20Z

src/block/request.rs

+/// Block request parsing errors.
+#[derive(Debug)]
+pub enum Error {
+    /// Guest gave us too few descriptors in a descriptor chain.


nit: "Driver" is prob better terminology than "Guest". It might also look nicer if we simply omit the "X gave us" part altogether.

alexandruag · 2020-11-16T11:46:13Z

src/block/request.rs

+    }
+
+    // Checks that a descriptor meets the minimal requirements for a valid status descriptor.
+    fn check_status_desc<M: GuestAddressSpace>(mem: &<M>::M, desc: Descriptor) -> Result<()> {


I think we can simplify things and use a M: GuestMemory bound here directly, instead of the GuestAddressSpace bound and then &<M>::M. This method only needs to be aware of a temporary GuestMemory borrow, so we can eliminate the use of extra abstractions in this particular case. The same comment applies to the following methods as well.

alexandruag · 2020-11-16T11:51:25Z

src/block/request.rs

+
+        // Check that the address of the status descriptor is valid in guest memory.
+        // We will write an u8 status here after executing the request.
+        let _ = mem


I think we can skip this check because the memory model will implicitly validate things when a request gets executed. We can also leave it (and other checks) for now, and maybe create an issue to determine if there's any performance impact when we're in a better position to measure it.

As mentioned in another comment, I would leave these checks so that we don't have partial execution of requests before the error is thrown (for read, write) or entire request execution for the case when the status address is invalid.

alexandruag · 2020-11-16T11:53:22Z

src/block/request.rs

+    ///
+    /// # Arguments
+    /// * `desc_chain` - A reference to the descriptor chain that should point to the buffers of a
+    ///                - virtio block request.


nit: the dash on this line should prob not be there

alexandruag · 2020-11-16T11:58:55Z

src/block/request.rs

+        let mut request = Request {
+            request_type: RequestType::from(request_header.request_type),
+            data: Vec::new(),
+            sector: request_header.sector,


It makes sense to me. Ultimately, the only reasons to consider not doing some of the validations are the performance impact and increasing code complexity too much. For now, I think our only worry is the former, and we can run some tests after we have an initial block device implementation in place as well (I guess we could also do a synthetic test that tries to parse many requests, but that might not be super relevant for real-world scenarios).

alexandruag · 2020-11-16T19:25:22Z

src/block/executor.rs

+
+//! Virtio block request execution abstractions.
+//!
+//! This module provides, for now, the following minimal abstraction for executing a virtio block


How about we rename the module to something like simple_executor just to convey this is essentially a minimal implementation, and also that it can make sense to have more than one execution abstraction? Is that a bit exaggerated?

I renamed this file to stdio_executor. If it's vague, I can think of some other name, but simple doesn't say much to me.

alexandruag · 2020-11-16T19:27:20Z

src/block/executor.rs

+use crate::block::request::{Request, RequestType};
+
+/// Read-only device.
+pub const VIRTIO_BLK_F_RO: u32 = 5;


At some point we should move the feature definitions in another part of the block module since they are global in that scope, but we can do that after we get a better idea about how the modules are going to be organized. Also, seems like virtio feature flags can be naturally represented by u64 values after they get shifted, so I think it's cleaner if we use u64 instead of u32 here and in the other places.

Yep, I changed to u64.

alexandruag · 2020-11-16T19:28:15Z

src/block/executor.rs

+const SECTOR_SIZE: u64 = (0x01 as u64) << SECTOR_SHIFT;
+
+/// Trait that keeps as supertraits the ones that are necessary for the `StdIoBackend` abstraction
+/// used for the virtio block request execution (v1.1 virtio specification).


I think things become cleaner if we remove the "(v1.1 virtio specification)", because right now it reads a little like the specification somehow implies these traits :-s

alexandruag · 2020-11-16T20:44:29Z

src/block/executor.rs

+            RequestType::In => {
+                for (data_addr, data_len) in request.data() {
+                    self.check_access(
+                        StdIoBackend::<B>::len_to_num_sectors(*data_len + bytes_from_dev),


Is it possible to change check_access so the conversion from number of bytes to number of sectors happens within, so we can get rid of len_to_num_sectors as a separate method as well?

The reason behind this split was the fact that for discard/write zeroes we receive the number of sectors in the request and it seemed cleaner to perform the check directly based on that number for those ops. If it hurts readability much, I can revert to one function.

That function is no longer needed, in the meantime I found this commit which clearly states that data for IN and OUT must be a multiple of 512B, so now I'm returning an error if this condition is not met.

lauralt · 2020-11-27T13:36:06Z

@bonzini @alexandruag @aghecenco @jiangliu I addressed your comments, can you please take another look? The documentation is now updated to clearly state that the StdIoBackend is just a simple executor, not a reference one. Do you think there is something else we should do here or can we proceed in merging this one (and open an issue for future abstractions/executors etc.)?

bonzini · 2020-11-27T15:15:41Z

Can you hide StdIoBackend behind a feature flag?

lauralt · 2020-11-27T16:46:17Z

Sure, done.

alexandruag

Looks good to me; left just one small comment.

alexandruag · 2020-11-29T15:19:23Z

src/block/stdio_executor.rs

+
+/// Errors encountered during request execution.
+#[derive(Debug)]
+pub enum Error {


Can we add something like this method to ease converting an error to the appropriate status value?

I wanted to give this a bit more thought in the next PR when I'll have a better idea of how to write the status too and regarding the method you've mentioned, for example, I would've liked to have some sort of From impl.
Anyway, I can quickly add the status method as is if it is really required for your PR in vmm-reference (but I expect to change it in the next PR).

It's not immediately required, the temporary solution we're using is fine for the time being.

lauralt · 2020-12-03T14:36:29Z

I opened an issue for the file access abstraction and also added here a commit for the backend-stdio feature. @bonzini can you please take another look? Thanks!

Dismissing my review after changes by the submitter.

Added the Request abstraction that handles the virtio block device request parsing via `parse()` method and keeps the necessary information for the request execution. Also added a mention in README about the supported virtio versions. vm-memory dependency is now pointing to 0.4.0 since 0.3.0 no longer compiles because of the v1.0.0 arc-swap update. Signed-off-by: Laura Loghin <[email protected]>

Signed-off-by: Alexandru Agache <[email protected]>

Signed-off-by: Laura Loghin <[email protected]>

This commit proposes a separate abstraction for block device request execution. The reason behind this is the fact that there can be multiple ways to execute these requests, e.g. asynchronous dispatch of requests and result collection. Such complex executors can be implemented as separate components which coexist with the current proposal. Signed-off-by: Laura Loghin <[email protected]> Suggested-by: Alexandru Agache <[email protected]>

Signed-off-by: Laura Loghin <[email protected]>

andreeaflorescu · 2020-12-09T15:06:29Z

@bonzini we are going to merge this PR so we can also publish the next one that adds the discard/write zeroes operations. If there are any remaining concerns, we will address them in a following PR.

lauralt requested review from alexandruag, jiangliu and slp as code owners November 3, 2020 17:41

lauralt force-pushed the request branch from cad9d57 to 172d36b Compare November 3, 2020 17:52

jiangliu reviewed Nov 4, 2020

View reviewed changes

alxiord self-requested a review November 5, 2020 11:34

alxiord reviewed Nov 6, 2020

View reviewed changes

alexandruag reviewed Nov 9, 2020

View reviewed changes

bonzini previously requested changes Nov 9, 2020

View reviewed changes

lauralt commented Nov 12, 2020

View reviewed changes

lauralt force-pushed the request branch from 172d36b to cb49b5c Compare November 12, 2020 09:21

alxiord mentioned this pull request Nov 13, 2020

[RFC] Block device proposal rust-vmm/vmm-reference#32

Closed

alexandruag reviewed Nov 16, 2020

View reviewed changes

lauralt force-pushed the request branch 5 times, most recently from 39f1a76 to 73b778c Compare November 23, 2020 09:57

alexandruag mentioned this pull request Nov 29, 2020

Add block device support rust-vmm/vmm-reference#41

Merged

alexandruag reviewed Nov 29, 2020

View reviewed changes

lauralt force-pushed the request branch from 1d8b4e7 to 9c6ea9a Compare December 2, 2020 15:10

alexandruag previously approved these changes Dec 2, 2020

View reviewed changes

lauralt mentioned this pull request Dec 3, 2020

block: add abstraction for file access #31

Open

lauralt changed the title ~~[RFC] block: abstractions for request parsing and execution~~ block: abstractions for request parsing and execution Dec 3, 2020

lauralt and others added 5 commits December 9, 2020 15:57

add build_desc_chain helper function

c32a5b9

Signed-off-by: Alexandru Agache <[email protected]>

add tests for request parsing

949639d

Signed-off-by: Laura Loghin <[email protected]>

hide StdIoBackend behind backend-stdio feature

1ce8f59

Signed-off-by: Laura Loghin <[email protected]>

lauralt dismissed alexandruag’s stale review via 1ce8f59 December 9, 2020 13:58

lauralt force-pushed the request branch from 9c6ea9a to 1ce8f59 Compare December 9, 2020 13:58

alexandruag approved these changes Dec 9, 2020

View reviewed changes

alxiord approved these changes Dec 9, 2020

View reviewed changes

andreeaflorescu merged commit ddc4505 into rust-vmm:master Dec 9, 2020

	RequestType::Discard \| RequestType::WriteZeroes => {}
	RequestType::Discard \| RequestType::WriteZeroes => { unimplemented!() }

block: abstractions for request parsing and execution #29

block: abstractions for request parsing and execution #29

Conversation

lauralt commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxiord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonzini left a comment

Choose a reason for hiding this comment

alexandruag commented Nov 9, 2020

bonzini commented Nov 9, 2020 • edited Loading

alexandruag commented Nov 10, 2020

lauralt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonzini commented Nov 9, 2020 •

edited

Loading