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

[Han Han]

On Thu, May 7, 2020 at 3:55 PM Michal Privoznik <mprivozn at redhat.com> 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 at redhat.com>
> > ---
> >   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: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200508/62a8af2c/attachment-0001.htm>