[libvirt] [PATCH v1 29/31] qemu: Generate command line of NVMe disks

[Michal Privoznik]

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 at 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