<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 7, 2020 at 3:55 PM Michal Privoznik <<a href="mailto:mprivozn@redhat.com">mprivozn@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 5/7/20 8:37 AM, Han Han wrote:<br>
> Unlike the file based disk, NVME disk cannot be pre-created by libvirt.<br>
> So skip the step of pre-creation.<br>
> <br>
> <a href="https://bugzilla.redhat.com/show_bug.cgi?id=1823639" rel="noreferrer" target="_blank">https://bugzilla.redhat.com/show_bug.cgi?id=1823639</a><br>
> <br>
> Signed-off-by: Han Han <<a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a>><br>
> ---<br>
> src/qemu/qemu_migration.c | 4 ++--<br>
> 1 file changed, 2 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c<br>
> index 02e8271e..e4bd9077 100644<br>
> --- a/src/qemu/qemu_migration.c<br>
> +++ b/src/qemu/qemu_migration.c<br>
> @@ -221,13 +221,13 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,<br>
> break;<br>
> <br>
> case VIR_STORAGE_TYPE_NETWORK:<br>
> - VIR_DEBUG("Skipping creation of network disk '%s'",<br>
> + case VIR_STORAGE_TYPE_NVME:<br>
> + VIR_DEBUG("Skipping creation of disk '%s'",<br>
> disk->dst);<br>
> return 0;<br>
> <br>
> case VIR_STORAGE_TYPE_BLOCK:<br>
> case VIR_STORAGE_TYPE_DIR:<br>
> - case VIR_STORAGE_TYPE_NVME:<br>
> case VIR_STORAGE_TYPE_NONE:<br>
> case VIR_STORAGE_TYPE_LAST:<br>
> virReportError(VIR_ERR_INTERNAL_ERROR,<br>
> <br>
<br>
Actually, my bugzilla comment might have been premature.<br>
This function you are changing is called from <br>
qemuMigrationDstPrecreateStorage() and that is also the place where we <br>
check whether disk source exists. If if doesn't then this function is <br>
called. However, the check there is not NVMe aware - it's only file based:<br>
<br>
diskSrcPath = virDomainDiskGetSource(disk);<br>
<br>
/* Skip disks we don't want to migrate and already existing <br>
disks. */<br>
if (!qemuMigrationAnyCopyDisk(disk, nmigrate_disks, <br>
migrate_disks) ||<br>
(diskSrcPath && virFileExists(diskSrcPath))) {<br>
continue;<br>
}<br>
<br>
<br>
I think this is the place we need to tweak. Something like (untested):<br>
<br>
diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c<br>
index e4bd90774c..fbc4474ccf 100644<br>
--- i/src/qemu/qemu_migration.c<br>
+++ w/src/qemu/qemu_migration.c<br>
@@ -315,6 +315,7 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,<br>
for (i = 0; i < nbd->ndisks; i++) {<br>
virDomainDiskDefPtr disk;<br>
const char *diskSrcPath;<br>
+ g_autofree char *nvmePath = NULL;<br>
<br>
VIR_DEBUG("Looking up disk target '%s' (capacity=%llu)",<br>
nbd->disks[i].target, nbd->disks[i].capacity);<br>
@@ -326,7 +327,12 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,<br>
goto cleanup;<br>
}<br>
<br>
- diskSrcPath = virDomainDiskGetSource(disk);<br>
+ if (disk->src->type == VIR_STORAGE_TYPE_NVME) {<br>
+ virPCIDeviceAddressGetSysfsFile(&disk->src->nvme->pciAddr, <br>
&nvmePath);<br></blockquote><div>I am not sure if it could cause NULL pointer dereference when empty cdrom source in nvme disk xml.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ diskSrcPath = nvmePath;<br>
+ } else {<br>
+ diskSrcPath = virDomainDiskGetSource(disk);<br>
+ }<br>
<br>
/* Skip disks we don't want to migrate and already existing <br>
disks. */<br>
if (!qemuMigrationAnyCopyDisk(disk, nmigrate_disks, <br>
migrate_disks) ||<br>
<br>
<br>
<br>
Can you check if that works and if so, then post is v2 please? Thanks.<br>
This merely checks if PCI device exists at given address. It doesn't <br>
check if its a NVMe disk. If we want to do that, we would need to use <br>
virPCIDeviceGetDriverPathAndName() plus some string comparison (see <br>
virHostdevPrepareOneNVMeDevice()). But that will be done when detaching <br>
NVMe disk for the domain startup, so maybe we don't need to duplicate it.<br></blockquote><div>I'll test that with these changes. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Michal<br>
<br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">Best regards,</div><div dir="ltr">-----------------------------------<br></div><div dir="ltr">Han Han<br>Quality Engineer<br>Redhat.<br><br>Email: <a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a><br>Phone: +861065339333<br></div></div></div></div></div></div></div>