Re: [libvirt] [PATCH v1 29/31] qemu: Generate command line of NVMe disks
On 7/16/19 5:35 PM, Peter Krempa wrote:
On Thu, Jul 11, 2019 at 17:54:16 +0200, Michal Privoznik wrote:
> Now, that we have everything prepared, we can generate command
> line for NVMe disks.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_block.c | 25 ++++++++-
> src/qemu/qemu_command.c | 3 ++
> src/qemu/qemu_process.c | 7 +++
> .../disk-nvme.x86_64-latest.args | 52 +++++++++++++++++++
> tests/qemuxml2argvtest.c | 1 +
> 5 files changed, 87 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/disk-nvme.x86_64-latest.args
Note that when you enable this you did not disallow snapshots (as in
creating a local file snapshot on top of the NVMe image) nor
implement the backing store string parser for this.
This means that once you create the snapshot, restarting the VM will
become impossible as we will not be able to parse the backing store
string (which is probably a bad idea altogether, since the disk can
change PCI addresses in the meanwhile so refering to it via the one
stored in the backing file would be wrong anyways).
You'll probably need to disable snapshots (see
qemuDomainSnapshotPrepareDiskExternalActive) if the even 'domdisk' is
NVMe too at least until we enable -blockdev support.
Fair enough.
Inactive external snapshots should be fine since we validate that only
BLOCK and FILE disks are allowed in
qemuDomainSnapshotPrepareDiskExternalInactive.
At any rate please also add TEST_DISK_TO_JSON case for this in
tests/qemublocktest.c
Okay.
Alternatively depending on how much we want to prevent from parsing
nvme:// from the backing store string we'll also need changes to
virStorageSourceNewFromBackingAbsolute which will deny the nvme backing
store.
The unfortunate part is that doing all these limitations basically
removes all the advantages of using NVMe disks via the qemu block layer.
But those limitations would exist only for the time being until we
switch to -blockdev, right? I am willing to take that risk, and allow
snapshots only with -blockdev. Also, given that we are already at 31
patches in this series, we can forbid snapshots for now and save the
work for a follow up series (if somebody really needs to do snapshots).
Note that, these patches still add some value even if we disallow
snapshots - domain can still migrate for instance.
Michal