Block refactor

[ShadowCurse]

Changes

Refactoring of the block devices architecture. Return of a single Block type which now will have virtio or vhost-user
implementation inside itself.

Reason

Lowers code complexity.

Currently Firecracker is designed with only virtio devices types in mind (i.e. net, block, vsock...). When Firecracker introduced second 'type' of block device (vhost-user-block) it created inconsistency and boilerplate code to distinguish between 2 block types. In order to reduce this newly introduced complexity we move back to a single Block type which plays nice with the rest of the system.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

• If a specific issue led to this PR, this PR closes the issue.
• The description of changes is clear and encompassing.
• Any required documentation changes (code and docs) are included in this PR.
• API changes follow the Runbook for Firecracker API changes.
• User-facing changes are mentioned in CHANGELOG.md.
• All added/changed functionality is tested.
• New TODOs link to an issue.
• Commits meet contribution quality standards.

---

• This functionality cannot be added in rust-vmm.

[codecov]

Codecov Report

Attention: 113 lines in your changes are missing coverage. Please review.

Comparison is base (c1413b7) 81.52% compared to head (963e2c5) 81.39%.

Files	Patch %	Lines
src/vmm/src/devices/virtio/block/device.rs	38.99%	97 Missing ⚠️
src/vmm/src/lib.rs	0.00%	8 Missing ⚠️
src/vmm/src/device_manager/persist.rs	76.92%	3 Missing ⚠️
src/vmm/src/device_manager/mmio.rs	0.00%	1 Missing ⚠️
src/vmm/src/devices/virtio/block/mod.rs	50.00%	1 Missing ⚠️
src/vmm/src/devices/virtio/block/persist.rs	50.00%	1 Missing ⚠️
src/vmm/src/devices/virtio/block/virtio/device.rs	0.00%	1 Missing ⚠️
src/vmm/src/vmm_config/drive.rs	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4285      +/-   ##
==========================================
- Coverage   81.52%   81.39%   -0.13%     
==========================================
  Files         241      243       +2     
  Lines       29296    29430     +134     
==========================================
+ Hits        23883    23955      +72     
- Misses       5413     5475      +62     

Flag	Coverage Δ
4.14-c5n.metal	78.67% <49.32%> (?)
4.14-c7g.metal	?
4.14-m5d.metal	?
4.14-m5n.metal	78.66% <49.32%> (?)
4.14-m6a.metal	77.78% <49.32%> (-0.13%)	⬇️
4.14-m6g.metal	76.74% <49.32%> (-0.13%)	⬇️
4.14-m6i.metal	78.65% <49.32%> (-0.13%)	⬇️
4.14-m7g.metal	76.74% <49.32%> (?)
5.10-c5n.metal	81.35% <49.32%> (?)
5.10-c7g.metal	?
5.10-m5d.metal	?
5.10-m5n.metal	81.34% <49.32%> (?)
5.10-m6a.metal	80.55% <49.32%> (-0.14%)	⬇️
5.10-m6g.metal	79.65% <49.32%> (-0.14%)	⬇️
5.10-m6i.metal	81.33% <49.32%> (-0.14%)	⬇️
5.10-m7g.metal	79.65% <49.32%> (?)
6.1-c5n.metal	81.35% <49.32%> (?)
6.1-m5n.metal	81.34% <49.32%> (?)
6.1-m6a.metal	80.55% <49.32%> (-0.14%)	⬇️
6.1-m6g.metal	79.65% <49.32%> (-0.14%)	⬇️
6.1-m6i.metal	81.33% <49.32%> (-0.14%)	⬇️
6.1-m7g.metal	79.65% <49.32%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bchalios]

Left a second round of comments. In general, I think I would prefer the flow of the commits to be the following:

1. Create the module structure under vmm::devices::virtio::block as you do now.
2. Create the new Block type that encapsulates the Virtio and Vhost devices
   • Maybe you need to mark this as #[allow(unused)]
3. Switch everything to use the new type.

My main motivation for this flow is that it makes reviewing a lot easier IMHO and it makes easier the process of following the git history of the code base as well.

edit: If I am the only one that thinks this is important I will not block the PR. So, I'd like the feedback of @kalyazin on this as well.

[bchalios]

Ok LGTM now, so I'm fine to approve it. I will just try to clarify my previous comment regarding the PR structure in terms of commits, mainly for future reference:

The way the PR was structured made it difficult for me to review, because:

1. It was difficult to follow the purpose of the changes.
2. There was a big re-direction in the PR with removing vhost-user-block and re-introducing it.

What the PR wanted to do (and did in the end) was to introduce a new type for block devices which encapsulates the two variants we support today (virtio and vhost-user). So, in my eyes it would be easier to:

1. create the new type without breaking the rest of the code. To ease review this could be done in 2 steps:
   • make the necessary changes in the file structure, moving existing types around as a preparatory step. Personally, I would group changes in fewer commits. I don't mind big commits as long as they do the same thing and having everything in one commit makes it easier to follow the purpose of a change.
   • add the new type as effectively dead code.
2. Plug the new type in the rest of the system.

I know that adding dead-code is counter intuitive and that's probably why my first thought would have been to do the change the way Egor did it here, as well. However, I think that doing it like that helps reviewing and, in reality, it doesn't have any issues. If the dead code of the first part is wrong, we will catch it in the second part where we plug in everything, so we can go back and amend the first part.

In any case, thanks a lot for the PR. I think this is a great improvement in our code structure and "cleaniness".

As I mentioned earlier I just wanted to write down my thoughts so we have data points in the future when we discuss PRs and git organization.