[PATCH v2] qemuProcessStop: Reattach NVMe disks a domain is mirroring into

Michal Privoznik posted 1 patch 4 years, 6 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/54c21d447691e86e518f638e25e4693b5009986f.1588844958.git.mprivozn@redhat.com
src/qemu/qemu_process.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
[PATCH v2] qemuProcessStop: Reattach NVMe disks a domain is mirroring into
Posted by Michal Privoznik 4 years, 6 months ago
In v5.10.0-rc1~42 (which was later fixed in v6.0.0-rc1~487) I am
removing XATTRs for a file that QEMU is mirroring a disk into but
it is killed meanwhile. Well, we can call
qemuSecurityRestoreImageLabel() which will not only remove XATTRs
but also use them to restore the original owner of the file.
Moreover, if the mirror destination is not a file but a NVMe
disk, then call qemuHostdevReAttachOneNVMeDisk() to reattach the
NVMe back to the host.

All of this would be done by blockjob code when the job finishes,
but in this case the job won't finish - QEMU is killed meanwhile.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1825785

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

v2 of:

https://www.redhat.com/archives/libvir-list/2020-May/msg00210.html

diff to v1:
- per Peter's review, instead of calling qemuDomainStorageSourceAccessRevoke()
  call only some functions it would call.

 src/qemu/qemu_process.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8ea470f75f..65d51e58c9 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7604,8 +7604,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
         for (i = 0; i < def->ndisks; i++) {
             virDomainDiskDefPtr disk = def->disks[i];
 
-            if (disk->mirror)
-                qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror);
+            if (disk->mirror) {
+                if (qemuSecurityRestoreImageLabel(driver, vm, disk->mirror, false) < 0)
+                    VIR_WARN("Unable to restore security label on %s", disk->dst);
+
+                if (virStorageSourceChainHasNVMe(disk->mirror))
+                    qemuHostdevReAttachOneNVMeDisk(driver, vm->def->name, disk->mirror);
+            }
 
             qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src);
         }
-- 
2.26.2

Re: [PATCH v2] qemuProcessStop: Reattach NVMe disks a domain is mirroring into
Posted by Peter Krempa 4 years, 6 months ago
On Thu, May 07, 2020 at 11:50:40 +0200, Michal Privoznik wrote:
> In v5.10.0-rc1~42 (which was later fixed in v6.0.0-rc1~487) I am
> removing XATTRs for a file that QEMU is mirroring a disk into but
> it is killed meanwhile. Well, we can call
> qemuSecurityRestoreImageLabel() which will not only remove XATTRs
> but also use them to restore the original owner of the file.
> Moreover, if the mirror destination is not a file but a NVMe
> disk, then call qemuHostdevReAttachOneNVMeDisk() to reattach the
> NVMe back to the host.
> 
> All of this would be done by blockjob code when the job finishes,
> but in this case the job won't finish - QEMU is killed meanwhile.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1825785
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---

IMO there are two things done in this commit. I agree with both but I
don't agree with having them both in same commit.

You can use:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On the both fragments without the need to re-send.