1. Patchew
  2. Libvirt
  3. View series

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

Han Han posted 1 patch 3 years, 11 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200507063711.53516-1-hhan@redhat.com
src/qemu/qemu_migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[libvirt PATCH] qemu: Skip pre-creation of NVME disk
Posted by Han Han 3 years, 11 months ago 
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@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,
-- 
2.25.0
Re: [libvirt PATCH] qemu: Skip pre-creation of NVME disk
Posted by Michal Privoznik 3 years, 11 months ago 
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@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);
+            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
Re: [libvirt PATCH] qemu: Skip pre-creation of NVME disk
Posted by Han Han 3 years, 11 months ago 
On Thu, May 7, 2020 at 3:55 PM Michal Privoznik <mprivozn@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@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@redhat.com
Phone: +861065339333

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.