[libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
Alter wipeVol to do same refresh operation as pool refresh would do. John Ferlan (2): storage: Introduce virStorageBackendRefreshVolTargetUpdate storage: Use virStorageBackendRefreshVolTargetUpdate after wipeVol src/storage/storage_driver.c | 7 ++-- src/storage/storage_util.c | 82 +++++++++++++++++++++++++++++--------------- src/storage/storage_util.h | 3 ++ 3 files changed, 62 insertions(+), 30 deletions(-) -- 2.9.5
Create a separate function to handle the volume target update via probe processing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 82 ++++++++++++++++++++++++++++++---------------- src/storage/storage_util.h | 3 ++ 2 files changed, 57 insertions(+), 28 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index e1fe162..b0a698a 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3517,6 +3517,58 @@ storageBackendProbeTarget(virStorageSourcePtr target, /** + * virStorageBackendRefreshVolTargetUpdate: + * @vol: Volume def that needs updating + * + * Attempt to probe the volume in order to get more details. + * + * Returns 0 on success, -2 to ignore failure, -1 on failure + */ +int +virStorageBackendRefreshVolTargetUpdate(virStorageVolDefPtr vol) +{ + int err; + + /* Real value is filled in during probe */ + vol->target.format = VIR_STORAGE_FILE_RAW; + + if ((err = storageBackendProbeTarget(&vol->target, + &vol->target.encryption)) < 0) { + if (err == -2) { + return -2; + } else if (err == -3) { + /* The backing file is currently unavailable, its format is not + * explicitly specified, the probe to auto detect the format + * failed: continue with faked RAW format, since AUTO will + * break virStorageVolTargetDefFormat() generating the line + * <format type='...'/>. */ + } else { + return -1; + } + } + + /* directory based volume */ + if (vol->target.format == VIR_STORAGE_FILE_DIR) + vol->type = VIR_STORAGE_VOL_DIR; + + if (vol->target.format == VIR_STORAGE_FILE_PLOOP) + vol->type = VIR_STORAGE_VOL_PLOOP; + + if (vol->target.backingStore) { + ignore_value(storageBackendUpdateVolTargetInfo(VIR_STORAGE_VOL_FILE, + vol->target.backingStore, + false, + VIR_STORAGE_VOL_OPEN_DEFAULT, 0)); + /* If this failed, the backing file is currently unavailable, + * the capacity, allocation, owner, group and mode are unknown. + * An error message was raised, but we just continue. */ + } + + return 0; +} + + +/** * Iterate over the pool's directory and enumerate all disk images * within it. This is non-recursive. */ @@ -3552,7 +3604,6 @@ virStorageBackendRefreshLocal(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; vol->type = VIR_STORAGE_VOL_FILE; - vol->target.format = VIR_STORAGE_FILE_RAW; /* Real value is filled in during probe */ if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) < 0) @@ -3561,40 +3612,15 @@ virStorageBackendRefreshLocal(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_STRDUP(vol->key, vol->target.path) < 0) goto cleanup; - if ((err = storageBackendProbeTarget(&vol->target, - &vol->target.encryption)) < 0) { + if ((err = virStorageBackendRefreshVolTargetUpdate(vol)) < 0) { if (err == -2) { /* Silently ignore non-regular files, * eg 'lost+found', dangling symbolic link */ virStorageVolDefFree(vol); vol = NULL; continue; - } else if (err == -3) { - /* The backing file is currently unavailable, its format is not - * explicitly specified, the probe to auto detect the format - * failed: continue with faked RAW format, since AUTO will - * break virStorageVolTargetDefFormat() generating the line - * <format type='...'/>. */ - } else { - goto cleanup; } - } - - /* directory based volume */ - if (vol->target.format == VIR_STORAGE_FILE_DIR) - vol->type = VIR_STORAGE_VOL_DIR; - - if (vol->target.format == VIR_STORAGE_FILE_PLOOP) - vol->type = VIR_STORAGE_VOL_PLOOP; - - if (vol->target.backingStore) { - ignore_value(storageBackendUpdateVolTargetInfo(VIR_STORAGE_VOL_FILE, - vol->target.backingStore, - false, - VIR_STORAGE_VOL_OPEN_DEFAULT, 0)); - /* If this failed, the backing file is currently unavailable, - * the capacity, allocation, owner, group and mode are unknown. - * An error message was raised, but we just continue. */ + goto cleanup; } if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index 6f2a1b1..00793ff 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -90,6 +90,9 @@ int virStorageBackendDeleteLocal(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags); +int +virStorageBackendRefreshVolTargetUpdate(virStorageVolDefPtr vol); + int virStorageBackendRefreshLocal(virConnectPtr conn, virStoragePoolObjPtr pool); -- 2.9.5
https://bugzilla.redhat.com/show_bug.cgi?id=1437797 Rather than using refreshVol which essentially only updates the allocation, capacity, and permissions for the volume, but not the format which does get updated in a pool refresh - let's use the same helper that pool refresh uses in order to update the volume target. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8552120..7cf5943 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2496,8 +2496,11 @@ storageVolWipePattern(virStorageVolPtr vol, if (backend->wipeVol(vol->conn, obj, voldef, algorithm, flags) < 0) goto cleanup; - if (backend->refreshVol && - backend->refreshVol(vol->conn, obj, voldef) < 0) + /* Instead of using the refreshVol, since much changes on the target + * volume, let's update using the same function as refreshPool would + * use when it discovers a volume. The only failure to capture is -1, + * we can ignore -2. */ + if (virStorageBackendRefreshVolTargetUpdate(voldef) == -1) goto cleanup; ret = 0; -- 2.9.5
On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
Alter wipeVol to do same refresh operation as pool refresh would do.
I think we should rather keep the format as it is. Did I miss something here or isn't the source of the problem just the fact that we wipe the volume without keeping the format?
John Ferlan (2): storage: Introduce virStorageBackendRefreshVolTargetUpdate storage: Use virStorageBackendRefreshVolTargetUpdate after wipeVol
src/storage/storage_driver.c | 7 ++-- src/storage/storage_util.c | 82 +++++++++++++++++++++++++++++--------------- src/storage/storage_util.h | 3 ++ 3 files changed, 62 insertions(+), 30 deletions(-)
-- 2.9.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 08/25/2017 05:44 AM, Martin Kletzander wrote:
On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
Alter wipeVol to do same refresh operation as pool refresh would do.
I think we should rather keep the format as it is. Did I miss something here or isn't the source of the problem just the fact that we wipe the volume without keeping the format?
Once you "wipe" the file/image the format that was there (such as qcow2 as noted in the bz from patch2) is no longer there. Consider the following sequence: virsh vol-create-as default bz 10M --format qcow2 virsh vol-dumpxml bz default | grep format <format type='qcow2'/> qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: qcow2 virtual size: 10M (10485760 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 0.10 refcount bits: 16 virsh vol-wipe bz default qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: raw virtual size: 196K (200704 bytes) disk size: 196K virsh vol-dumpxml bz default | grep format <format type='qcow2'/> (without the patch in 2/2 of course) BTW: I did consider just changing the format to RAW regardless, but figured that was just too simple and may not be the right answer for every case. John
John Ferlan (2): storage: Introduce virStorageBackendRefreshVolTargetUpdate storage: Use virStorageBackendRefreshVolTargetUpdate after wipeVol
src/storage/storage_driver.c | 7 ++-- src/storage/storage_util.c | 82 +++++++++++++++++++++++++++++--------------- src/storage/storage_util.h | 3 ++ 3 files changed, 62 insertions(+), 30 deletions(-)
-- 2.9.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Aug 25, 2017 at 07:52:25AM -0400, John Ferlan wrote:
On 08/25/2017 05:44 AM, Martin Kletzander wrote:
On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
Alter wipeVol to do same refresh operation as pool refresh would do.
I think we should rather keep the format as it is. Did I miss something here or isn't the source of the problem just the fact that we wipe the volume without keeping the format?
Once you "wipe" the file/image the format that was there (such as qcow2 as noted in the bz from patch2) is no longer there.
Consider the following sequence:
virsh vol-create-as default bz 10M --format qcow2
virsh vol-dumpxml bz default | grep format <format type='qcow2'/>
qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: qcow2 virtual size: 10M (10485760 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 0.10 refcount bits: 16
virsh vol-wipe bz default
qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: raw virtual size: 196K (200704 bytes) disk size: 196K
virsh vol-dumpxml bz default | grep format <format type='qcow2'/>
(without the patch in 2/2 of course)
BTW: I did consider just changing the format to RAW regardless, but figured that was just too simple and may not be the right answer for every case.
Sure, but that's not my point. My point is that instead of rewriting the data with zeros, we could re-initialize it. Anyway, from what I see in the docs, we "fixed" it by documenting this behaviour already, so my point is moo: https://libvirt.org/html/libvirt-libvirt-storage.html#virStorageVolWipe (I guess I should know that when I pushed that documentation into the tree) So ACK series.
On 08/25/2017 08:28 AM, Martin Kletzander wrote:
On Fri, Aug 25, 2017 at 07:52:25AM -0400, John Ferlan wrote:
On 08/25/2017 05:44 AM, Martin Kletzander wrote:
On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
Alter wipeVol to do same refresh operation as pool refresh would do.
I think we should rather keep the format as it is. Did I miss something here or isn't the source of the problem just the fact that we wipe the volume without keeping the format?
Once you "wipe" the file/image the format that was there (such as qcow2 as noted in the bz from patch2) is no longer there.
Consider the following sequence:
virsh vol-create-as default bz 10M --format qcow2
virsh vol-dumpxml bz default | grep format <format type='qcow2'/>
qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: qcow2 virtual size: 10M (10485760 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 0.10 refcount bits: 16
virsh vol-wipe bz default
qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: raw virtual size: 196K (200704 bytes) disk size: 196K
virsh vol-dumpxml bz default | grep format <format type='qcow2'/>
(without the patch in 2/2 of course)
BTW: I did consider just changing the format to RAW regardless, but figured that was just too simple and may not be the right answer for every case.
Sure, but that's not my point. My point is that instead of rewriting the data with zeros, we could re-initialize it. Anyway, from what I see in the docs, we "fixed" it by documenting this behaviour already, so my point is moo:
https://libvirt.org/html/libvirt-libvirt-storage.html#virStorageVolWipe
Doh! I should have looked there too... Interesting though... So should we instead close this bug as documented to work that way? Or should the text there be modified as well since the bz will cause the alteration to at least RAW for "most" storage backends - I think it's only the disk backend w/ an extended partition and a sparse logical volume that cannot be wiped, but those fail the wipe - so we'd never get to the change the target.format code. John
(I guess I should know that when I pushed that documentation into the tree)
So ACK series.
On Fri, Aug 25, 2017 at 06:11:49PM -0400, John Ferlan wrote:
On 08/25/2017 08:28 AM, Martin Kletzander wrote:
On Fri, Aug 25, 2017 at 07:52:25AM -0400, John Ferlan wrote:
On 08/25/2017 05:44 AM, Martin Kletzander wrote:
On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
Alter wipeVol to do same refresh operation as pool refresh would do.
I think we should rather keep the format as it is. Did I miss something here or isn't the source of the problem just the fact that we wipe the volume without keeping the format?
Once you "wipe" the file/image the format that was there (such as qcow2 as noted in the bz from patch2) is no longer there.
Consider the following sequence:
virsh vol-create-as default bz 10M --format qcow2
virsh vol-dumpxml bz default | grep format <format type='qcow2'/>
qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: qcow2 virtual size: 10M (10485760 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 0.10 refcount bits: 16
virsh vol-wipe bz default
qemu-img info /home/vm-images/bz image: /home/vm-images/bz file format: raw virtual size: 196K (200704 bytes) disk size: 196K
virsh vol-dumpxml bz default | grep format <format type='qcow2'/>
(without the patch in 2/2 of course)
BTW: I did consider just changing the format to RAW regardless, but figured that was just too simple and may not be the right answer for every case.
Sure, but that's not my point. My point is that instead of rewriting the data with zeros, we could re-initialize it. Anyway, from what I see in the docs, we "fixed" it by documenting this behaviour already, so my point is moo:
https://libvirt.org/html/libvirt-libvirt-storage.html#virStorageVolWipe
Doh! I should have looked there too... Interesting though... So should we instead close this bug as documented to work that way? Or should the text there be modified as well since the bz will cause the alteration to at least RAW for "most" storage backends - I think it's only the disk backend w/ an extended partition and a sparse logical volume that cannot be wiped, but those fail the wipe - so we'd never get to the change the target.format code.
I think that all the problems are essentially caused by people misunderstanding the API's purpose. That's why I ACKed it, as we at least report the right information. I think your solution is precisely the midpoint between not doing anything and fixing all corner cases.
John
(I guess I should know that when I pushed that documentation into the tree)
So ACK series.
participants (2)
-
John Ferlan -
Martin Kletzander