Hello,

I would like to start multiple KVM guests from one qcow2 image, and
discard the changes which the KVM guests done.

transient disk option is useful for discarding the changes, however,
we cannot start multiple KVM guest from one qcow2 image because the
image is write-locked by the first guest to be started.

I suppose the disk which transient option is enabled don't need to
get the write lock because any changes go to the overlay image, and
the overlay image is removed when the guest shutdown.

qemu has 'locking' option and the write lock is disabled when locking=off.
To implement that, I have two ideas. I would appreciate it if you could
give me the ideas which way is better (or another way).

1. Add an element to handle 'locking' qemu option. Like as:

    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' locking='off'/>
      <source file='/var/lib/libvirt/images/guest.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <transient/>
    </disk>

2. Add locking=off internally only if the transient disk option is enabled.
   The sample code is as follows:

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 23415b323c..6fafe22ca3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10186,6 +10186,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
             def->src->shared = true;
         } else if (virXMLNodeNameEqual(cur, "transient")) {
             def->transient = true;
+            def->src->transient = true;
         } else if (!encryption &&
                    virXMLNodeNameEqual(cur, "encryption")) {
             if (!(encryption = virStorageEncryptionParseNode(cur, ctxt)))
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 4640e339c0..3db888d08b 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1211,6 +1211,12 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
                                       "s:discard", "unmap",
                                       NULL) < 0)
                 return NULL;
+
+            if (src->transient) {
+                if (virJSONValueObjectAdd(fileprops,
+                                          "S:locking", "off", NULL) < 0)
+                    return NULL;
+            }
         }
     }
 
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 15494c3415..7823810df6 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1182,7 +1182,8 @@ qemuSnapshotDiskPrepareDisksTransient(virDomainObjPtr vm,
         snapdisk->src = virStorageSourceNew();
         snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
         snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
-        snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path);
+        snapdisk->src->path = g_strdup_printf("%s.TRANSIENT-%s",
+                                              domdisk->src->path, vm->def->name);
 
         if (virFileExists(snapdisk->src->path)) {
             virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 87763cf389..70c963bd42 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -384,6 +384,8 @@ struct _virStorageSource {
     /* these must not be used apart from formatting the output JSON in the qemu driver */
     char *ssh_user;
     bool ssh_host_key_check_disabled;
+
+    bool transient;
 };
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref);
--

Thanks,
Masa

On Fri, Dec 11, 2020 at 20:58:48 -0500, Masayoshi Mizuma wrote:
> Hello,
> 
> I would like to start multiple KVM guests from one qcow2 image, and
> discard the changes which the KVM guests done.
> 
> transient disk option is useful for discarding the changes, however,
> we cannot start multiple KVM guest from one qcow2 image because the
> image is write-locked by the first guest to be started.
> 
> I suppose the disk which transient option is enabled don't need to
> get the write lock because any changes go to the overlay image, and
> the overlay image is removed when the guest shutdown.
> 
> qemu has 'locking' option and the write lock is disabled when locking=off.
> To implement that, I have two ideas. I would appreciate it if you could
> give me the ideas which way is better (or another way).

There are two aspects of this.

1) Controlling the locking property of qemu may be worth in many cases,
so by itself this would be a worthwhile patchset to add control of
qemu locking for a per-backing store basis.

2) Disabling locking to achieve this is wrong though. The qemu image
locking infrastructure is justified and prevents many problems which
users might get into by wrong configuration.

For <transient/> disks, we should rather instantiate the top level
format node as 'readonly' and then put a read-write overlay on top of
it. This still prevents from using the file which became a backing file
in read-write mode in another VM.

You are welcome to propose the locking control as a proper patchset
(with proper splitting of patches) but it can't be described as a way to
share the backing image of <transient/> disks since that needs a
systemic fix.

On Sat, Dec 12, 2020 at 11:57:15AM +0100, Peter Krempa wrote:
> On Fri, Dec 11, 2020 at 20:58:48 -0500, Masayoshi Mizuma wrote:
> > Hello,
> > 
> > I would like to start multiple KVM guests from one qcow2 image, and
> > discard the changes which the KVM guests done.
> > 
> > transient disk option is useful for discarding the changes, however,
> > we cannot start multiple KVM guest from one qcow2 image because the
> > image is write-locked by the first guest to be started.
> > 
> > I suppose the disk which transient option is enabled don't need to
> > get the write lock because any changes go to the overlay image, and
> > the overlay image is removed when the guest shutdown.
> > 
> > qemu has 'locking' option and the write lock is disabled when locking=off.
> > To implement that, I have two ideas. I would appreciate it if you could
> > give me the ideas which way is better (or another way).
> 
> There are two aspects of this.
> 
> 1) Controlling the locking property of qemu may be worth in many cases,
> so by itself this would be a worthwhile patchset to add control of
> qemu locking for a per-backing store basis.
> 
> 2) Disabling locking to achieve this is wrong though. The qemu image
> locking infrastructure is justified and prevents many problems which
> users might get into by wrong configuration.
> 
> For <transient/> disks, we should rather instantiate the top level
> format node as 'readonly' and then put a read-write overlay on top of
> it. This still prevents from using the file which became a backing file
> in read-write mode in another VM.

Thank you for the idea!
I just tried to change the original image to read-only (changed the "read-only":false
to "read-only":true) simply, then created a read-write overlay.
qemu stopped with following assertion:

  qemu-system-x86_64: ../block/io.c:1874: bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed.

It looks like the qemu hit the assertion because the permission of the overlay image
is same as the original image.
Probably I'm missing something... I'll dive into that...

> 
> You are welcome to propose the locking control as a proper patchset
> (with proper splitting of patches) but it can't be described as a way to
> share the backing image of <transient/> disks since that needs a
> systemic fix.

Thanks. I understand the locking option is different topic from sharing
disks with <transient/> option.

Thanks!
Masa

On Mon, Dec 14, 2020 at 22:49:03 -0500, Masayoshi Mizuma wrote:
> On Sat, Dec 12, 2020 at 11:57:15AM +0100, Peter Krempa wrote:
> > On Fri, Dec 11, 2020 at 20:58:48 -0500, Masayoshi Mizuma wrote:
> > > Hello,
> > > 
> > > I would like to start multiple KVM guests from one qcow2 image, and
> > > discard the changes which the KVM guests done.
> > > 
> > > transient disk option is useful for discarding the changes, however,
> > > we cannot start multiple KVM guest from one qcow2 image because the
> > > image is write-locked by the first guest to be started.
> > > 
> > > I suppose the disk which transient option is enabled don't need to
> > > get the write lock because any changes go to the overlay image, and
> > > the overlay image is removed when the guest shutdown.
> > > 
> > > qemu has 'locking' option and the write lock is disabled when locking=off.
> > > To implement that, I have two ideas. I would appreciate it if you could
> > > give me the ideas which way is better (or another way).
> > 
> > There are two aspects of this.
> > 
> > 1) Controlling the locking property of qemu may be worth in many cases,
> > so by itself this would be a worthwhile patchset to add control of
> > qemu locking for a per-backing store basis.
> > 
> > 2) Disabling locking to achieve this is wrong though. The qemu image
> > locking infrastructure is justified and prevents many problems which
> > users might get into by wrong configuration.
> > 
> > For <transient/> disks, we should rather instantiate the top level
> > format node as 'readonly' and then put a read-write overlay on top of
> > it. This still prevents from using the file which became a backing file
> > in read-write mode in another VM.
> 
> Thank you for the idea!
> I just tried to change the original image to read-only (changed the "read-only":false
> to "read-only":true) simply, then created a read-write overlay.
> qemu stopped with following assertion:
> 
>   qemu-system-x86_64: ../block/io.c:1874: bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed.
> 
> It looks like the qemu hit the assertion because the permission of the overlay image
> is same as the original image.
> Probably I'm missing something... I'll dive into that...

Could you please post the command line and the QMP commands? Maybe
something isn't configured right in libvirt. Alternatively qemu might
need modification.

On Thu, Dec 17, 2020 at 10:39:36AM +0100, Peter Krempa wrote:
> On Mon, Dec 14, 2020 at 22:49:03 -0500, Masayoshi Mizuma wrote:
> > On Sat, Dec 12, 2020 at 11:57:15AM +0100, Peter Krempa wrote:
> > > On Fri, Dec 11, 2020 at 20:58:48 -0500, Masayoshi Mizuma wrote:
> > > > Hello,
> > > > 
> > > > I would like to start multiple KVM guests from one qcow2 image, and
> > > > discard the changes which the KVM guests done.
> > > > 
> > > > transient disk option is useful for discarding the changes, however,
> > > > we cannot start multiple KVM guest from one qcow2 image because the
> > > > image is write-locked by the first guest to be started.
> > > > 
> > > > I suppose the disk which transient option is enabled don't need to
> > > > get the write lock because any changes go to the overlay image, and
> > > > the overlay image is removed when the guest shutdown.
> > > > 
> > > > qemu has 'locking' option and the write lock is disabled when locking=off.
> > > > To implement that, I have two ideas. I would appreciate it if you could
> > > > give me the ideas which way is better (or another way).
> > > 
> > > There are two aspects of this.
> > > 
> > > 1) Controlling the locking property of qemu may be worth in many cases,
> > > so by itself this would be a worthwhile patchset to add control of
> > > qemu locking for a per-backing store basis.
> > > 
> > > 2) Disabling locking to achieve this is wrong though. The qemu image
> > > locking infrastructure is justified and prevents many problems which
> > > users might get into by wrong configuration.
> > > 
> > > For <transient/> disks, we should rather instantiate the top level
> > > format node as 'readonly' and then put a read-write overlay on top of
> > > it. This still prevents from using the file which became a backing file
> > > in read-write mode in another VM.
> > 
> > Thank you for the idea!
> > I just tried to change the original image to read-only (changed the "read-only":false
> > to "read-only":true) simply, then created a read-write overlay.
> > qemu stopped with following assertion:
> > 
> >   qemu-system-x86_64: ../block/io.c:1874: bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed.
> > 
> > It looks like the qemu hit the assertion because the permission of the overlay image
> > is same as the original image.
> > Probably I'm missing something... I'll dive into that...
> 
> Could you please post the command line and the QMP commands? Maybe
> something isn't configured right in libvirt. Alternatively qemu might
> need modification.

Sure, here is the qemu command line options and QMP commands:

qemu command line options:

qemu-system-x86_64 \
	-M q35,accel=kvm,usb=off,vmport=off,smm=on,dump-guest-core=off \
	-smp 4 \
	-m 4096 \
	-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2","node-name":"storage1","auto-read-only":true,"discard":"unmap"}' \
	-blockdev '{"node-name":"format1","read-only":true,"driver":"qcow2","file":"storage1","backing":null}' \
	-device virtio-blk-pci,drive=format1,id=virtio-disk0,bootindex=1 \
	-nographic \
	-nodefaults \
	-no-user-config \
	-serial telnet::1235,server,nowait \
	-qmp tcp::1335,server,nowait \
	-S

QMP commands:
(Before running following QMP commands, I create '/var/lib/libvirt/images/guest.qcow2.TRANSIENT'
 by touch command)

   {"execute":"qmp_capabilities"}
   {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2.TRANSIENT","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
   {"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"storage2","size":10737418240,"cluster-size":65536,"backing-file":"/var/lib/libvirt/images/guest.qcow2","backing-fmt":"qcow2"}}}
   {"execute":"blockdev-add","arguments":{"node-name":"format2","read-only":false,"driver":"qcow2","file":"storage2","backing":null}}
   {"execute":"blockdev-snapshot","arguments":{"node":"format1","overlay":"format2"}}
   {"execute":"cont"}

After "cont" command executed, qemu stops as following assertion:

    qemu-system-x86_64: ../block/io.c:1865: bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed.

Thanks,
Masa

On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:
> On Thu, Dec 17, 2020 at 10:39:36AM +0100, Peter Krempa wrote:
> > On Mon, Dec 14, 2020 at 22:49:03 -0500, Masayoshi Mizuma wrote:
> > > On Sat, Dec 12, 2020 at 11:57:15AM +0100, Peter Krempa wrote:
> > > > On Fri, Dec 11, 2020 at 20:58:48 -0500, Masayoshi Mizuma wrote:
> > > > > Hello,
> > > > > 
> > > > > I would like to start multiple KVM guests from one qcow2 image, and
> > > > > discard the changes which the KVM guests done.
> > > > > 
> > > > > transient disk option is useful for discarding the changes, however,
> > > > > we cannot start multiple KVM guest from one qcow2 image because the
> > > > > image is write-locked by the first guest to be started.
> > > > > 
> > > > > I suppose the disk which transient option is enabled don't need to
> > > > > get the write lock because any changes go to the overlay image, and
> > > > > the overlay image is removed when the guest shutdown.
> > > > > 
> > > > > qemu has 'locking' option and the write lock is disabled when locking=off.
> > > > > To implement that, I have two ideas. I would appreciate it if you could
> > > > > give me the ideas which way is better (or another way).
> > > > 
> > > > There are two aspects of this.
> > > > 
> > > > 1) Controlling the locking property of qemu may be worth in many cases,
> > > > so by itself this would be a worthwhile patchset to add control of
> > > > qemu locking for a per-backing store basis.
> > > > 
> > > > 2) Disabling locking to achieve this is wrong though. The qemu image
> > > > locking infrastructure is justified and prevents many problems which
> > > > users might get into by wrong configuration.
> > > > 
> > > > For <transient/> disks, we should rather instantiate the top level
> > > > format node as 'readonly' and then put a read-write overlay on top of
> > > > it. This still prevents from using the file which became a backing file
> > > > in read-write mode in another VM.
> > > 
> > > Thank you for the idea!
> > > I just tried to change the original image to read-only (changed the "read-only":false
> > > to "read-only":true) simply, then created a read-write overlay.
> > > qemu stopped with following assertion:
> > > 
> > >   qemu-system-x86_64: ../block/io.c:1874: bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed.
> > > 
> > > It looks like the qemu hit the assertion because the permission of the overlay image
> > > is same as the original image.
> > > Probably I'm missing something... I'll dive into that...
> > 
> > Could you please post the command line and the QMP commands? Maybe
> > something isn't configured right in libvirt. Alternatively qemu might
> > need modification.
> 
> Sure, here is the qemu command line options and QMP commands:
> 
> qemu command line options:
> 
> qemu-system-x86_64 \
> 	-M q35,accel=kvm,usb=off,vmport=off,smm=on,dump-guest-core=off \
> 	-smp 4 \
> 	-m 4096 \
> 	-blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2","node-name":"storage1","auto-read-only":true,"discard":"unmap"}' \
> 	-blockdev '{"node-name":"format1","read-only":true,"driver":"qcow2","file":"storage1","backing":null}' \
> 	-device virtio-blk-pci,drive=format1,id=virtio-disk0,bootindex=1 \
> 	-nographic \
> 	-nodefaults \
> 	-no-user-config \
> 	-serial telnet::1235,server,nowait \
> 	-qmp tcp::1335,server,nowait \
> 	-S
> 
> QMP commands:
> (Before running following QMP commands, I create '/var/lib/libvirt/images/guest.qcow2.TRANSIENT'
>  by touch command)
> 
>    {"execute":"qmp_capabilities"}
>    {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2.TRANSIENT","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
>    {"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"storage2","size":10737418240,"cluster-size":65536,"backing-file":"/var/lib/libvirt/images/guest.qcow2","backing-fmt":"qcow2"}}}
>    {"execute":"blockdev-add","arguments":{"node-name":"format2","read-only":false,"driver":"qcow2","file":"storage2","backing":null}}
>    {"execute":"blockdev-snapshot","arguments":{"node":"format1","overlay":"format2"}}
>    {"execute":"cont"}
> 
> After "cont" command executed, qemu stops as following assertion:
> 
>     qemu-system-x86_64: ../block/io.c:1865: bdrv_co_write_req_prepare: Assertion `child->perm & BLK_PERM_WRITE' failed.

I think following qemu command line options and QMP commands work for sharing
the qcow2 disks. The following uses disk hotplug instead of snapshot overlay.
Does that make sense for libvirt...?

qemu command line options:

  qemu-system-x86_64 \
	-M q35,accel=kvm,usb=off,vmport=off,smm=on,dump-guest-core=off \
	-smp 1 \
	-m 4096 \
	-blockdev '{"driver":"file","filename":"/home/mmizuma/debug/guest.qcow2","node-name":"storage1","auto-read-only":true,"discard":"unmap"}' \
	-blockdev '{"node-name":"format1","read-only":true,"driver":"qcow2","file":"storage1","backing":null}' \
	-nographic \
	-nodefaults \
	-no-user-config \
	-serial telnet::10000,server,nowait \
	-qmp tcp::10001,server,nowait \
	-S \
	-device pcie-root-port,id=pci.1

QMP commands:

  {"execute":"qmp_capabilities"}
  {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
  {"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"storage2","size":4294967296,"cluster-size":65536,"backing-file":"/var/lib/libvirt/images/guest.TRANSIENT","backing-fmt":"qcow2"}}}
  {"execute":"blockdev-add","arguments":{"node-name":"format2","read-only":false,"driver":"qcow2","file":"storage2"}}
  {"execute":"device_add","arguments":{"driver":"virtio-blk-pci","drive":"format2","id":"transient-disk","bootindex":1,"bus":"pci.1","addr":0}}
  {"execute":"cont"}

Thanks,
Masa

On Mon, Jan 04, 2021 at 15:30:19 -0500, Masayoshi Mizuma wrote:
> On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:

[...]

> I think following qemu command line options and QMP commands work for sharing
> the qcow2 disks. The following uses disk hotplug instead of snapshot overlay.
> Does that make sense for libvirt...?
> 
> qemu command line options:

So you are proposing to ...

> 
>   qemu-system-x86_64 \
> 	-M q35,accel=kvm,usb=off,vmport=off,smm=on,dump-guest-core=off \
> 	-smp 1 \
> 	-m 4096 \
> 	-blockdev '{"driver":"file","filename":"/home/mmizuma/debug/guest.qcow2","node-name":"storage1","auto-read-only":true,"discard":"unmap"}' \
> 	-blockdev '{"node-name":"format1","read-only":true,"driver":"qcow2","file":"storage1","backing":null}' \

... start with the disk already in 'read-only' mode _and_ skip addition
of the disk ...

> 	-nographic \
> 	-nodefaults \
> 	-no-user-config \
> 	-serial telnet::10000,server,nowait \
> 	-qmp tcp::10001,server,nowait \
> 	-S \
> 	-device pcie-root-port,id=pci.1
> 
> QMP commands:
> 
>   {"execute":"qmp_capabilities"}
>   {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
>   {"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"storage2","size":4294967296,"cluster-size":65536,"backing-file":"/var/lib/libvirt/images/guest.TRANSIENT","backing-fmt":"qcow2"}}}
>   {"execute":"blockdev-add","arguments":{"node-name":"format2","read-only":false,"driver":"qcow2","file":"storage2"}}

... and then add a writable overlay ...

>   {"execute":"device_add","arguments":{"driver":"virtio-blk-pci","drive":"format2","id":"transient-disk","bootindex":1,"bus":"pci.1","addr":0}}

... and hotplug the disk.
>   {"execute":"cont"}

So that is a no-go. Some disk bus-es such as IDE don't support hotplug:

https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_hotplug.c#L1074

You could try to just instantiate the backend of the disk as read-only,
and then create a writable overlay. You just need to make sure that the
disk will be writable and that it works even for IDE/SATA which doesn't
support read-only:

https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L2634

On Tue, Jan 05, 2021 at 15:12:55 +0100, Peter Krempa wrote:
> On Mon, Jan 04, 2021 at 15:30:19 -0500, Masayoshi Mizuma wrote:
> > On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:
 
 [...]
 
 >   {"execute":"cont"}
> 
> So that is a no-go. Some disk bus-es such as IDE don't support hotplug:
> 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_hotplug.c#L1074
> 
> You could try to just instantiate the backend of the disk as read-only,
> and then create a writable overlay. You just need to make sure that the
> disk will be writable and that it works even for IDE/SATA which doesn't
> support read-only:
> 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L2634

Okay, I've actually tried implementing my suggestion and that doesn't
work. With scsi disks qemu crashes on an assertion failure when I try
writing to the disk after setting it up as suggested and virtio disk
returns an I/O error as it remembers that it was read-only when
starting.

I'm considering whether it's worth implementing this via hotplug then.

Arguably simple deployments don't need it and complex ones have some
kind of orchestration which can do it externally.

This specifically comes with a caveat of the above, as currently the
overlay used to discard writes is created under the same path as the
image and can't be controlled, which might be a problem for complex
deployments.

Also the above algorithm with a constant suffix added to the image
prevents to use it with multiple VMs anyways, since the overlay file
name will collide (since it's generated based on the same rules).

Didn't you run into the collision?

On Thu, Jan 07, 2021 at 09:05:42AM +0100, Peter Krempa wrote:
> On Tue, Jan 05, 2021 at 15:12:55 +0100, Peter Krempa wrote:
> > On Mon, Jan 04, 2021 at 15:30:19 -0500, Masayoshi Mizuma wrote:
> > > On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:
>  
>  [...]
>  
>  >   {"execute":"cont"}
> > 
> > So that is a no-go. Some disk bus-es such as IDE don't support hotplug:
> > 
> > https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_hotplug.c#L1074
> > 
> > You could try to just instantiate the backend of the disk as read-only,
> > and then create a writable overlay. You just need to make sure that the
> > disk will be writable and that it works even for IDE/SATA which doesn't
> > support read-only:
> > 
> > https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L2634
> 
> Okay, I've actually tried implementing my suggestion and that doesn't
> work. With scsi disks qemu crashes on an assertion failure when I try
> writing to the disk after setting it up as suggested and virtio disk
> returns an I/O error as it remembers that it was read-only when
> starting.
> 
> I'm considering whether it's worth implementing this via hotplug then.
> 
> Arguably simple deployments don't need it and complex ones have some
> kind of orchestration which can do it externally.
> 
> This specifically comes with a caveat of the above, as currently the
> overlay used to discard writes is created under the same path as the
> image and can't be controlled, which might be a problem for complex
> deployments.
> 
> Also the above algorithm with a constant suffix added to the image
> prevents to use it with multiple VMs anyways, since the overlay file
> name will collide (since it's generated based on the same rules).
> 
> Didn't you run into the collision?

Yes, I needed to set the different filename for each guests with blockdev-add QMP command.
Is it good idea to add the guest name as the suffix to the filename? Like as:

Guest0 QMP:

  {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest0","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}

Guest1 QMP:

  {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest1","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}

Thanks,
Masa

On Thu, Jan 07, 2021 at 16:32:59 -0500, Masayoshi Mizuma wrote:
> On Thu, Jan 07, 2021 at 09:05:42AM +0100, Peter Krempa wrote:
> > On Tue, Jan 05, 2021 at 15:12:55 +0100, Peter Krempa wrote:
> > > On Mon, Jan 04, 2021 at 15:30:19 -0500, Masayoshi Mizuma wrote:
> > > > On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:

[...]

> Guest0 QMP:
> 
>   {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest0","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
> 
> Guest1 QMP:
> 
>   {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest1","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}

Anyways, if you want, you can try implement this using the
frontend-hotplug approach for 'virtio' and 'scsi' disks, but I can't
guarantee that it will be accepted upstream in cases when the
implementation will turn out to be too hairy.

On Fri, Jan 15, 2021 at 01:49:54PM +0100, Peter Krempa wrote:
> On Thu, Jan 07, 2021 at 16:32:59 -0500, Masayoshi Mizuma wrote:
> > On Thu, Jan 07, 2021 at 09:05:42AM +0100, Peter Krempa wrote:
> > > On Tue, Jan 05, 2021 at 15:12:55 +0100, Peter Krempa wrote:
> > > > On Mon, Jan 04, 2021 at 15:30:19 -0500, Masayoshi Mizuma wrote:
> > > > > On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:
> 
> [...]
> 
> > Guest0 QMP:
> > 
> >   {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest0","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
> > 
> > Guest1 QMP:
> > 
> >   {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest1","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
> 
> Anyways, if you want, you can try implement this using the
> frontend-hotplug approach for 'virtio' and 'scsi' disks, but I can't
> guarantee that it will be accepted upstream in cases when the
> implementation will turn out to be too hairy.

Thanks.
I'm trying to implement this. I will submit the patches as RFC or WIP.

- Masa