On Thu, May 7, 2020 at 3:55 PM Michal Privoznik <mprivozn@xxxxxxxxxx> 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 <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);
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,
-----------------------------------
