On Thu, May 7, 2020 at 3:55 PM Michal Privoznik <[email protected]> wrote:
> 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 <[email protected]> > > --- > > 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); > I am not sure if it could cause NULL pointer dereference when empty cdrom source in nvme disk xml. > + 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. > I'll test that with these changes. > > Michal > > -- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: [email protected] Phone: +861065339333
