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

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(a)redhat.com&gt; > --- > 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