Re: [libvirt PATCH] qemu: Skip pre-creation of NVME disk

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 5/7/20 8:37 AM, Han Han wrote:
> Unlike the file based disk, NVME disk cannot be pre-created by libvirt.
> So skip the step of pre-creation.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1823639
>
> Signed-off-by: Han Han <hhan@xxxxxxxxxx>
> ---
>   src/qemu/qemu_migration.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 02e8271e..e4bd9077 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -221,13 +221,13 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
>           break;
>   
>       case VIR_STORAGE_TYPE_NETWORK:
> -        VIR_DEBUG("Skipping creation of network disk '%s'",
> +    case VIR_STORAGE_TYPE_NVME:
> +        VIR_DEBUG("Skipping creation of disk '%s'",
>                     disk->dst);
>           return 0;
>   
>       case VIR_STORAGE_TYPE_BLOCK:
>       case VIR_STORAGE_TYPE_DIR:
> -    case VIR_STORAGE_TYPE_NVME:
>       case VIR_STORAGE_TYPE_NONE:
>       case VIR_STORAGE_TYPE_LAST:
>           virReportError(VIR_ERR_INTERNAL_ERROR,

Actually, my bugzilla comment might have been premature.
This function you are changing is called from 
qemuMigrationDstPrecreateStorage() and that is also the place where we 
check whether disk source exists. If if doesn't then this function is 
called. However, the check there is not NVMe aware - it's only file based:

        diskSrcPath = virDomainDiskGetSource(disk);

        /* Skip disks we don't want to migrate and already existing 
disks. */
        if (!qemuMigrationAnyCopyDisk(disk, nmigrate_disks, 
migrate_disks) ||
            (diskSrcPath && virFileExists(diskSrcPath))) {
            continue;
        }


I think this is the place we need to tweak. Something like (untested):

diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c
index e4bd90774c..fbc4474ccf 100644
--- i/src/qemu/qemu_migration.c
+++ w/src/qemu/qemu_migration.c
@@ -315,6 +315,7 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
     for (i = 0; i < nbd->ndisks; i++) {
         virDomainDiskDefPtr disk;
         const char *diskSrcPath;
+        g_autofree char *nvmePath = NULL;

         VIR_DEBUG("Looking up disk target '%s' (capacity=%llu)",
                   nbd->disks[i].target, nbd->disks[i].capacity);
@@ -326,7 +327,12 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
             goto cleanup;
         }

-        diskSrcPath = virDomainDiskGetSource(disk);
+        if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
+            virPCIDeviceAddressGetSysfsFile(&disk->src->nvme->pciAddr, 
&nvmePath);
+            diskSrcPath = nvmePath;
+        } else {
+            diskSrcPath = virDomainDiskGetSource(disk);
+        }

         /* Skip disks we don't want to migrate and already existing 
disks. */
         if (!qemuMigrationAnyCopyDisk(disk, nmigrate_disks, 
migrate_disks) ||



Can you check if that works and if so, then post is v2 please? Thanks.
This merely checks if PCI device exists at given address. It doesn't 
check if its a NVMe disk. If we want to do that, we would need to use 
virPCIDeviceGetDriverPathAndName() plus some string comparison (see 
virHostdevPrepareOneNVMeDevice()). But that will be done when detaching 
NVMe disk for the domain startup, so maybe we don't need to duplicate it.

Michal