[PATCH 0/1] RFC for allowing updating some disk driver
QEMU has provided QMP blockdev-reopen to allow reconfigure some driver properties while virtual machine is running. Such properties are including cachmode, detect-zeroes and discard. This PS modifies update-device a little bit to allow updating those properties. ushen (1): WIP: allow update more disk properties src/qemu/qemu_block.c | 2 +- src/qemu/qemu_block.h | 5 +++++ src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 4 files changed, 23 insertions(+), 3 deletions(-) -- 2.17.1
From: ushen <yshxxsjt715@gmail.com> --- src/qemu/qemu_block.c | 2 +- src/qemu/qemu_block.h | 5 +++++ src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 5e700eff99..778aeebb8f 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -2981,7 +2981,7 @@ qemuBlockReopenFormatMon(qemuMonitor *mon, * that @src must be already properly configured for the desired outcome. The * nodenames of @src are used to identify the specific image in qemu. */ -static int +int qemuBlockReopenFormat(virDomainObj *vm, virStorageSource *src, virDomainAsyncJob asyncJob) diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 5a61a19da2..9ef692278d 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -245,6 +245,11 @@ int qemuBlockReopenFormatMon(qemuMonitor *mon, virStorageSource *src); +int +qemuBlockReopenFormat(virDomainObj *vm, + virStorageSource *src, + virDomainAsyncJob asyncJob); + int qemuBlockReopenReadWrite(virDomainObj *vm, virStorageSource *src, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e9bc0f375d..e5b5fef87e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8239,7 +8239,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_STREQ_NULLABLE(product, "product"); - CHECK_EQ(cachemode, "cache", true); CHECK_EQ(error_policy, "error_policy", true); CHECK_EQ(rerror_policy, "rerror_policy", true); CHECK_EQ(iomode, "io", true); @@ -8267,7 +8266,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); - CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true); CHECK_STREQ_NULLABLE(domain_name, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6154fe9bfe..bcb68d6c66 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6802,6 +6802,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm, virDomainDiskDef *orig_disk = NULL; virDomainStartupPolicy origStartupPolicy; virDomainDeviceDef oldDev = { .type = dev->type }; + int ret; if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6840,6 +6841,22 @@ qemuDomainChangeDiskLive(virDomainObj *vm, } dev->data.disk->src = NULL; + } else { + // reopen disk device with more parameters + disk->src->backingStore = orig_disk->src->backingStore; + ret = qemuBlockReopenFormat(vm, disk->src, false); + disk->src->backingStore = NULL; + if (!ret) { + orig_disk->cachemode = disk->cachemode; + orig_disk->src->cachemode = disk->src->cachemode; + orig_disk->detect_zeroes = disk->detect_zeroes; + orig_disk->src->detect_zeroes = disk->src->detect_zeroes; + orig_disk->discard = disk->discard; + orig_disk->src->discard = disk->src->discard; + disk->src->backingStore = NULL; + } else { + return ret; + } } /* in case when we aren't updating disk source we update startup policy here */ -- 2.17.1
On Mon, Feb 20, 2023 at 10:57:55 +0800, jshen28 wrote:
From: ushen <yshxxsjt715@gmail.com>
You should put your explanation and justification of the patch here rather than into the cover-letter. Also you need to comply with the following contributor guideline: https://www.libvirt.org/hacking.html#developer-certificate-of-origin
--- src/qemu/qemu_block.c | 2 +- src/qemu/qemu_block.h | 5 +++++ src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 4 files changed, 23 insertions(+), 3 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e9bc0f375d..e5b5fef87e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8239,7 +8239,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_STREQ_NULLABLE(product, "product");
- CHECK_EQ(cachemode, "cache", true);
Note that 'cachemode' actually influences 3 separate caching properties, one of which is a frontend-device property. Specifically that is the 'write-cache' property of the frontend device. This patch as-is can't change the fronted property so it would be incorrect to allow arbitrary change of the caching mode. The two others are indeed properties of the image itself thus are addressed by blockdev-reopen. See qemuDomainDiskCachemodeFlags
CHECK_EQ(error_policy, "error_policy", true); CHECK_EQ(rerror_policy, "rerror_policy", true); CHECK_EQ(iomode, "io", true); @@ -8267,7 +8266,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); - CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true);
Also note that discard options similarly to the cache mode are copied over from the disk definition into all backing images' definitions. Thus changing those actually requires doing a blockdev-reopen on all of the backing chain.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6154fe9bfe..bcb68d6c66 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6802,6 +6802,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm, virDomainDiskDef *orig_disk = NULL; virDomainStartupPolicy origStartupPolicy; virDomainDeviceDef oldDev = { .type = dev->type }; + int ret;
if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6840,6 +6841,22 @@ qemuDomainChangeDiskLive(virDomainObj *vm, }
dev->data.disk->src = NULL; + } else { + // reopen disk device with more parameters + disk->src->backingStore = orig_disk->src->backingStore; + ret = qemuBlockReopenFormat(vm, disk->src, false);
3rd property of qemuBlockReopenFormat is 'virDomainAsyncJob asyncJob' passing 'false' is wrong.
+ disk->src->backingStore = NULL; + if (!ret) {
The return value is not a boolean or pointer thus '!ret' is not how you are supposed to check it. Also as noted you'll need to do this for the whole backing chain. Also customary in our code, on failure we jump out, and just then process the rest of the success code path ...
+ orig_disk->cachemode = disk->cachemode; + orig_disk->src->cachemode = disk->src->cachemode; + orig_disk->detect_zeroes = disk->detect_zeroes; + orig_disk->src->detect_zeroes = disk->src->detect_zeroes; + orig_disk->discard = disk->discard; + orig_disk->src->discard = disk->src->discard; + disk->src->backingStore = NULL
So this should not be in a block.
+ } else {
This case also leaves 'backingStore' of the original disk assigned in the definition which will be thrown away. Since you didn't increase the reference count on failure the backin store would be freed but used later.
+ return ret; + }
Hello, Sorry for not keeping communication on list, forgot to click reply to button. please take a look at patch https://listman.redhat.com/archives/libvir-list/2023-February/238033.html. Thank you and appologize for bringing inconvenience. Best, Jiatong Shen At 2023-02-20 23:23:09, "Peter Krempa" <pkrempa@redhat.com> wrote:
On Mon, Feb 20, 2023 at 10:57:55 +0800, jshen28 wrote:
From: ushen <yshxxsjt715@gmail.com>
You should put your explanation and justification of the patch here rather than into the cover-letter.
Also you need to comply with the following contributor guideline:
https://www.libvirt.org/hacking.html#developer-certificate-of-origin
--- src/qemu/qemu_block.c | 2 +- src/qemu/qemu_block.h | 5 +++++ src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 4 files changed, 23 insertions(+), 3 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e9bc0f375d..e5b5fef87e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8239,7 +8239,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_STREQ_NULLABLE(product, "product");
- CHECK_EQ(cachemode, "cache", true);
Note that 'cachemode' actually influences 3 separate caching properties, one of which is a frontend-device property. Specifically that is the 'write-cache' property of the frontend device.
This patch as-is can't change the fronted property so it would be incorrect to allow arbitrary change of the caching mode.
The two others are indeed properties of the image itself thus are addressed by blockdev-reopen.
See qemuDomainDiskCachemodeFlags
CHECK_EQ(error_policy, "error_policy", true); CHECK_EQ(rerror_policy, "rerror_policy", true); CHECK_EQ(iomode, "io", true); @@ -8267,7 +8266,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); CHECK_EQ(sgio, "sgio", true); - CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true);
Also note that discard options similarly to the cache mode are copied over from the disk definition into all backing images' definitions.
Thus changing those actually requires doing a blockdev-reopen on all of the backing chain.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6154fe9bfe..bcb68d6c66 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6802,6 +6802,7 @@ qemuDomainChangeDiskLive(virDomainObj *vm, virDomainDiskDef *orig_disk = NULL; virDomainStartupPolicy origStartupPolicy; virDomainDeviceDef oldDev = { .type = dev->type }; + int ret;
if (!(orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6840,6 +6841,22 @@ qemuDomainChangeDiskLive(virDomainObj *vm, }
dev->data.disk->src = NULL; + } else { + // reopen disk device with more parameters + disk->src->backingStore = orig_disk->src->backingStore; + ret = qemuBlockReopenFormat(vm, disk->src, false);
3rd property of qemuBlockReopenFormat is 'virDomainAsyncJob asyncJob' passing 'false' is wrong.
+ disk->src->backingStore = NULL; + if (!ret) {
The return value is not a boolean or pointer thus '!ret' is not how you are supposed to check it.
Also as noted you'll need to do this for the whole backing chain.
Also customary in our code, on failure we jump out, and just then process the rest of the success code path ...
+ orig_disk->cachemode = disk->cachemode; + orig_disk->src->cachemode = disk->src->cachemode; + orig_disk->detect_zeroes = disk->detect_zeroes; + orig_disk->src->detect_zeroes = disk->src->detect_zeroes; + orig_disk->discard = disk->discard; + orig_disk->src->discard = disk->src->discard; + disk->src->backingStore = NULL
So this should not be in a block.
+ } else {
This case also leaves 'backingStore' of the original disk assigned in the definition which will be thrown away. Since you didn't increase the reference count on failure the backin store would be freed but used later.
+ return ret; + }
participants (3)
-
jshen28 -
Norman
-
Peter Krempa