[libvirt] [PATCH] Block info query: Add flag to allow failure if not active
Currently the qemuDomainGetBlockInfo will return allocation == physical for most backing stores. For a qcow2 block backed device it's possible to return the highest lv extent allocated from qemu for an active guest. That is a value where allocation != physical and one would hope be less. However, if the guest is not running, then the code falls back to returning allocation == physical. This turns out to be problematic for rhev which monitors the size of the backing store. During a migration, before the VM has been started on the target and while it is deemed inactive on the source, allocation is returned as physical triggering the code to extend the file unnecessarily. Thus, this patch will check a new flag to the call to determine whether a failure should be returned to indicate to the caller that the code is unable to fetch the allocation value. This will allow the caller to decide how to proceed. Note that I chose not to return a failure as a more general rule mostly because of backwards compatability and current expectations reasons. Changing a default return action could be an unexpected action. Although returning allocation == physical perhaps is incorrect, it is how the code has been functioning. Having a 'virsh domblkinfo' return a failure for "some" inactive domains while succeeding for others could be confusing. A flag forces the caller to decide how to operate. As it turns out, there seems to be quite a bit of history regarding this. Details can be found in the BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1040507 Signed-off-by: John Ferlan <jferlan@redhat.com> --- NOTE: Trying to keep this as simple as possible since it needs to be backported. If someone has a better idea for a flag name - I'm open to suggestions... include/libvirt/libvirt.h.in | 23 ++++++++++++++++++++--- src/libvirt.c | 11 ++++++++++- src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++------ 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6f79c49..420642b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2095,7 +2095,8 @@ int virDomainBlockResize (virDomainPtr dom, /** virDomainBlockInfo: * - * This struct provides information about the size of a block device backing store + * This struct provides information about the size of a block device + * backing store * * Examples: * @@ -2108,13 +2109,29 @@ int virDomainBlockResize (virDomainPtr dom, * * - qcow2 file in filesystem * * capacity: logical size from qcow2 header - * * allocation, physical: logical size of the file / highest qcow extent (identical) + * * allocation, physical: logical size of the file / + * highest qcow extent (identical) * * - qcow2 file in a block device * * capacity: logical size from qcow2 header - * * allocation: highest qcow extent written + * * allocation: highest qcow extent written for an active domain * * physical: size of the block device container */ + +/** + * virDomainGetBlockInfoFlags: + * + * Flags available for virDomainGetBlockInfo(). + */ +typedef enum { + VIR_DOMAIN_BLOCK_CHECK_ACTIVE = 1 << 0, /* If not active, then return + * failure when attempting to + * fetch data from an inactive + * domain, such as allocation + * from a qcow2 block device + */ +} virDomainGetBlockInfoFlags; + typedef struct _virDomainBlockInfo virDomainBlockInfo; typedef virDomainBlockInfo *virDomainBlockInfoPtr; struct _virDomainBlockInfo { diff --git a/src/libvirt.c b/src/libvirt.c index d15d617..2159eaf 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -9027,7 +9027,7 @@ error: * @domain: a domain object * @disk: path to the block device, or device shorthand * @info: pointer to a virDomainBlockInfo structure allocated by the user - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of supported virDomainGetBlockInfoFlags * * Extract information about a domain's block device. * @@ -9038,6 +9038,15 @@ error: * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * + * If the VIR_DOMAIN_BLOCK_CHECK_ACTIVE flag is set, then for any block + * information data that could differ depending on whether the domain was + * active or not, the hypervisor will return a failure when the domain + * is deemed to be inactive or unable to provide the data, such as may be + * the case during a migration. For example, the 'allocation' value for + * a qcow2 block device backed storage can return the highest extent written. + * However, this can only be determined if the domain is active. For an + * inactive domain, the allocation and physical values would be returned + * as identical. * Returns 0 in case of success and -1 in case of failure. */ int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45d11cd..6303ce2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9799,9 +9799,10 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, struct stat sb; int idx; int format; + int activeFail = false; virQEMUDriverConfigPtr cfg = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_CHECK_ACTIVE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -9897,15 +9898,24 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, /* Set default value .. */ info->allocation = info->physical; - /* ..but if guest is running & not using raw - disk format and on a block device, then query - highest allocated extent from QEMU */ + /* ..but if guest is not using raw disk format and on a block device, + * then query highest allocated extent from QEMU + */ if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && format != VIR_STORAGE_FILE_RAW && - S_ISBLK(sb.st_mode) && - virDomainObjIsActive(vm)) { + S_ISBLK(sb.st_mode)) { qemuDomainObjPrivatePtr priv = vm->privateData; + /* If the guest is not running, then success/failure return + * depends on whether the caller requested a failure on an + * inactive domain. + */ + if (!virDomainObjIsActive(vm)) { + activeFail = true; + ret = 0; + goto cleanup; + } + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; @@ -9916,6 +9926,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, &info->allocation); qemuDomainObjExitMonitor(driver, vm); } else { + activeFail = true; ret = 0; } @@ -9931,6 +9942,16 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(cfg); + + /* If we failed to get data from a domain because it's inactive and + * this flag is set, then we'll return failure + */ + if (activeFail && (flags & VIR_DOMAIN_BLOCK_CHECK_ACTIVE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unable to get allocation, domain is not running")); + ret = -1; + } + return ret; } -- 1.8.3.1
On Wed, Dec 18, 2013 at 06:58:10AM -0500, John Ferlan wrote:
Currently the qemuDomainGetBlockInfo will return allocation == physical for most backing stores. For a qcow2 block backed device it's possible to return the highest lv extent allocated from qemu for an active guest. That is a value where allocation != physical and one would hope be less. However, if the guest is not running, then the code falls back to returning allocation == physical. This turns out to be problematic for rhev which monitors the size of the backing store. During a migration, before the VM has been started on the target and while it is deemed inactive on the source, allocation is returned as physical triggering the code to extend the file unnecessarily.
Thus, this patch will check a new flag to the call to determine whether a failure should be returned to indicate to the caller that the code is unable to fetch the allocation value. This will allow the caller to decide how to proceed.
Note that I chose not to return a failure as a more general rule mostly because of backwards compatability and current expectations reasons. Changing a default return action could be an unexpected action. Although returning allocation == physical perhaps is incorrect, it is how the code has been functioning. Having a 'virsh domblkinfo' return a failure for "some" inactive domains while succeeding for others could be confusing. A flag forces the caller to decide how to operate.
As it turns out, there seems to be quite a bit of history regarding this. Details can be found in the BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1040507 Signed-off-by: John Ferlan <jferlan@redhat.com> --- NOTE: Trying to keep this as simple as possible since it needs to be backported. If someone has a better idea for a flag name - I'm open to suggestions...
include/libvirt/libvirt.h.in | 23 ++++++++++++++++++++--- src/libvirt.c | 11 ++++++++++- src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++------ 3 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6f79c49..420642b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2095,7 +2095,8 @@ int virDomainBlockResize (virDomainPtr dom,
/** virDomainBlockInfo: * - * This struct provides information about the size of a block device backing store + * This struct provides information about the size of a block device + * backing store * * Examples: * @@ -2108,13 +2109,29 @@ int virDomainBlockResize (virDomainPtr dom, * * - qcow2 file in filesystem * * capacity: logical size from qcow2 header - * * allocation, physical: logical size of the file / highest qcow extent (identical) + * * allocation, physical: logical size of the file / + * highest qcow extent (identical) * * - qcow2 file in a block device * * capacity: logical size from qcow2 header - * * allocation: highest qcow extent written + * * allocation: highest qcow extent written for an active domain * * physical: size of the block device container */ + +/** + * virDomainGetBlockInfoFlags: + * + * Flags available for virDomainGetBlockInfo(). + */ +typedef enum { + VIR_DOMAIN_BLOCK_CHECK_ACTIVE = 1 << 0, /* If not active, then return + * failure when attempting to + * fetch data from an inactive + * domain, such as allocation + * from a qcow2 block device + */ +} virDomainGetBlockInfoFlags;
I'm not convinced this flag is desirable. Can't apps just check whether the guest is running themselves, or indeed something like RHEV surely knows what its own VM is doing already. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/18/2013 09:35 AM, Daniel P. Berrange wrote:
On Wed, Dec 18, 2013 at 06:58:10AM -0500, John Ferlan wrote:
I'm not convinced this flag is desirable. Can't apps just check whether the guest is running themselves, or indeed something like RHEV surely knows what its own VM is doing already.
I do agree with you and that's been my argument in the referenced BZ; however, as I understand it - they have a thread that continually polls for blockInfo information and a separate thread that handles the migration. The blockInfo thread doesn't have the state information. I contend it's just as simple to add a check about the domain state and to get/check the reason value as well. That is - I think the blockInfo thread should be more aware of state. Of course, the return argument is that libvirt shouldn't return different answers on consecutive fetches where the first fetch is when the guest is "active" and the second is when it's not. To be honest this feels a bit hackish and it requires a deliberate change to their code to not only provide the flag, but handle the error. Since they'd already be changing their code to handle this, then why not change to check state. There is a downside to their checking state though and that's a timing condition where their check could indicate the guest was active, but the "next clock tick" it becomes inactive. Thus, their code would be in the same position of getting what is felt is the wrong answer. I figure opening it up to community discussion will help "decide" one way or another the best approach. I'm all for adjusting the documentation and leaving the code the way it is now as well. John
On Wed, Dec 18, 2013 at 09:49:46AM -0500, John Ferlan wrote:
On 12/18/2013 09:35 AM, Daniel P. Berrange wrote:
On Wed, Dec 18, 2013 at 06:58:10AM -0500, John Ferlan wrote:
I'm not convinced this flag is desirable. Can't apps just check whether the guest is running themselves, or indeed something like RHEV surely knows what its own VM is doing already.
I do agree with you and that's been my argument in the referenced BZ; however, as I understand it - they have a thread that continually polls for blockInfo information and a separate thread that handles the migration. The blockInfo thread doesn't have the state information.
But it must have a virDomainPtr instance, so it can use virDomainGetState() or virDomainIsActive() if it cares about this.
I contend it's just as simple to add a check about the domain state and to get/check the reason value as well. That is - I think the blockInfo thread should be more aware of state. Of course, the return argument is that libvirt shouldn't return different answers on consecutive fetches where the first fetch is when the guest is "active" and the second is when it's not.
Sure, I agree that libvirt isn't ideal here - our hands are tied by the fact that QEMU doesn't make this data available to us offline. If we changed anything on the libvirt side, then I'd want to see us do a proper fix to get the data but that'd require qemu help;
To be honest this feels a bit hackish and it requires a deliberate change to their code to not only provide the flag, but handle the error. Since they'd already be changing their code to handle this, then why not change to check state. There is a downside to their checking state though and that's a timing condition where their check could indicate the guest was active, but the "next clock tick" it becomes inactive. Thus, their code would be in the same position of getting what is felt is the wrong answer.
If they check the state after querying the data, then yes they could get bogus data. If they check the state before querying the info, then the race condition scenario does not lead to bad behaviour - they would be simply be discarding the "allocation" value unncessarily which is exactly the same situation they'd be in if the libvirt API itself checked this too, and the guest shutoff after libvirt checked its state. So I don't see a need for this libvirt change here.
I figure opening it up to community discussion will help "decide" one way or another the best approach.
I'm all for adjusting the documentation and leaving the code the way it is now as well.
Updating docs would be my recommendation. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
----- Original Message -----
From: "Daniel P. Berrange" <berrange@redhat.com> To: "John Ferlan" <jferlan@redhat.com> Cc: libvir-list@redhat.com Sent: Wednesday, December 18, 2013 3:57:22 PM Subject: Re: [libvirt] [PATCH] Block info query: Add flag to allow failure if not active
On Wed, Dec 18, 2013 at 09:49:46AM -0500, John Ferlan wrote:
On 12/18/2013 09:35 AM, Daniel P. Berrange wrote:
On Wed, Dec 18, 2013 at 06:58:10AM -0500, John Ferlan wrote:
I'm not convinced this flag is desirable. Can't apps just check whether the guest is running themselves, or indeed something like RHEV surely knows what its own VM is doing already.
I do agree with you and that's been my argument in the referenced BZ; however, as I understand it - they have a thread that continually polls for blockInfo information and a separate thread that handles the migration. The blockInfo thread doesn't have the state information.
But it must have a virDomainPtr instance, so it can use virDomainGetState() or virDomainIsActive() if it cares about this.
oVirt is using only transient domains, so it seems to me that the assumption here is that blockInfo should either return a value from a running qemu process or an error (because the domain doesn't exist anymore). Having a short moment where it's treated as a non-transient domain is confusing. It's fine by me if you want to maintain the current implementation for non-transient domains (and therefore the new flag is not needed).
I contend it's just as simple to add a check about the domain state and to get/check the reason value as well. That is - I think the blockInfo thread should be more aware of state. Of course, the return argument is that libvirt shouldn't return different answers on consecutive fetches where the first fetch is when the guest is "active" and the second is when it's not.
Sure, I agree that libvirt isn't ideal here - our hands are tied by the fact that QEMU doesn't make this data available to us offline. If we changed anything on the libvirt side, then I'd want to see us do a proper fix to get the data but that'd require qemu help;
I added the support for this to qemu-img. It scans a qcow2 image and reports the highest allocated cluster. Sadly the only way to implement it was on the image check (takes a long time). -- Federico
participants (3)
-
Daniel P. Berrange -
Federico Simoncelli -
John Ferlan