[libvirt] [PATCH] Restore skipping of setting capacity
Commit id 'ac9a0963' refactored out the 'withCapacity' for the virStorageBackendUpdateVolInfo() API. See: http://www.redhat.com/archives/libvir-list/2014-April/msg00043.html This resulted in a difference in how 'virsh vol-info --pool <poolName> <volume>' or 'virsh vol-list vol-list --pool <poolName> --details' outputs the capacity information for a directory pool with a qcow2 sparse file. For example, using the following XML mkdir /home/TestPool cat testpool.xml <pool type='dir'> <name>TestPool</name> <uuid>6bf80895-10b6-75a6-6059-89fdea2aefb7</uuid> <source> </source> <target> <path>/home/TestPool</path> <permissions> <mode>0755</mode> <owner>0</owner> <group>0</group> </permissions> </target> </pool> virsh pool-create testpool.xml virsh vol-create-as --pool TestPool temp_vol_1 \ --capacity 1048576 --allocation 1048576 --format qcow2 virsh vol-info --pool TestPool temp_vol_1 Results in listing a Capacity value. Prior to the commit, the value would be '1.0 MiB' (1048576 bytes). However, after the commit the output would be (for example) '192.50 KiB', which for my system was the size of the volume in my file system (eg 'ls -l TestPool/temp_vol_1' results in '197120' bytes or 192.50 KiB). While perhaps technically correct, it's not necessarily what the user expected (certainly virt-test didn't expect it). This patch restores the code to not update the target capacity for this path Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 22 +++++++++++++++------- src/storage/storage_backend.h | 5 ++++- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 11 +++++++---- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 946196b..afedbf5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1403,6 +1403,7 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags) { @@ -1413,7 +1414,8 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, + updateCapacity)) < 0) goto cleanup; if (withBlockVolFormat) { @@ -1429,18 +1431,21 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags) { int ret; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, + updateCapacity, withBlockVolFormat, openflags)) < 0) return ret; if (vol->backingStore.path && (ret = virStorageBackendUpdateVolTargetInfo(&vol->backingStore, + updateCapacity, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) return ret; @@ -1453,15 +1458,15 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, * @target: target definition ptr of volume to update * @fd: fd of storage volume to update, via virStorageBackendOpenVol*, or -1 * @sb: details about file (must match @fd, if that is provided) - * @allocation: If not NULL, updated allocation information will be stored - * @capacity: If not NULL, updated capacity info will be stored + * @updateCapacity: If true, updated capacity info will be stored * * Returns 0 for success, -1 on a legitimate error condition. */ int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb) + struct stat *sb, + bool updateCapacity) { #if WITH_SELINUX security_context_t filecon = NULL; @@ -1477,10 +1482,12 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, /* Regular files may be sparse, so logical size (capacity) is not same * as actual allocation above */ - target->capacity = sb->st_size; + if (updateCapacity) + target->capacity = sb->st_size; } else if (S_ISDIR(sb->st_mode)) { target->allocation = 0; - target->capacity = 0; + if (updateCapacity) + target->capacity = 0; } else if (fd >= 0) { off_t end; /* XXX this is POSIX compliant, but doesn't work for CHAR files, @@ -1496,7 +1503,8 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, return -1; } target->allocation = end; - target->capacity = end; + if (updateCapacity) + target->capacity = end; } if (!target->perms && VIR_ALLOC(target->perms) < 0) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 5997077..456b9d7 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -137,14 +137,17 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, + bool updateCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb); + struct stat *sb, + bool updateCapacity); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 9cebcca..13336fc 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -113,7 +113,7 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, } /* Refresh allocation/capacity/perms */ - if (virStorageBackendUpdateVolInfo(vol, false, + if (virStorageBackendUpdateVolInfo(vol, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) return -1; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 3694c26..5a2add8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -84,7 +84,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto error; /* Take care to propagate ret, it is not always -1 */ fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) { + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, + &sb, true)) < 0) { goto error; } @@ -913,7 +914,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->backingStore.format = backingStoreFormat; ignore_value(virStorageBackendUpdateVolTargetInfo( - &vol->backingStore, false, + &vol->backingStore, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT)); /* If this failed, the backing file is currently unavailable, * the capacity, allocation, owner, group and mode are unknown. @@ -1190,8 +1191,10 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, { int ret; - /* Refresh allocation / permissions info in case its changed */ - ret = virStorageBackendUpdateVolInfo(vol, false, + /* Refresh allocation / permissions info in case its changed + * don't update the capacity value for this pass + */ + ret = virStorageBackendUpdateVolInfo(vol, false, false, VIR_STORAGE_VOL_FS_OPEN_FLAGS); if (ret < 0) return ret; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e0a25df..28db909 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -267,7 +267,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (VIR_ALLOC(vol) < 0) goto cleanup; - if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st, true) < 0) goto cleanup; if (virStorageBackendGlusterSetMetadata(state, vol, name) < 0) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ed3a012..a597e67 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -149,7 +149,7 @@ virStorageBackendLogicalMakeVol(char **const groups, if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, false, + if (virStorageBackendUpdateVolInfo(vol, true, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) goto cleanup; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index f0ed189..8c3b0df 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -60,7 +60,7 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, true, + if (virStorageBackendUpdateVolInfo(vol, true, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { goto cleanup; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c448d7f..71bcf56 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -199,7 +199,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; } - if (virStorageBackendUpdateVolInfo(vol, true, + if (virStorageBackendUpdateVolInfo(vol, true, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { retval = -1; goto free_vol; -- 1.9.0
On 04/23/2014 07:28 AM, John Ferlan wrote:
Commit id 'ac9a0963' refactored out the 'withCapacity' for the virStorageBackendUpdateVolInfo() API. See:
Fortunately, we haven't released this regression of mine :)
http://www.redhat.com/archives/libvir-list/2014-April/msg00043.html
This resulted in a difference in how 'virsh vol-info --pool <poolName> <volume>' or 'virsh vol-list vol-list --pool <poolName> --details' outputs
s/vol-list vol-list/vol-list/
the capacity information for a directory pool with a qcow2 sparse file.
Results in listing a Capacity value. Prior to the commit, the value would be '1.0 MiB' (1048576 bytes). However, after the commit the output would be (for example) '192.50 KiB', which for my system was the size of the volume in my file system (eg 'ls -l TestPool/temp_vol_1' results in '197120' bytes or 192.50 KiB). While perhaps technically correct, it's not necessarily what the user expected (certainly virt-test didn't expect it).
This patch restores the code to not update the target capacity for this path
More precisely, the code needs to be careful that we prefer capacity information learned from file format metadata (qcow2), and fall back to capacity from file system data; my refactoring accidentally swapped things so that file system capacity took precedence over metadata capacity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 22 +++++++++++++++------- src/storage/storage_backend.h | 5 ++++- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 11 +++++++---- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 31 insertions(+), 17 deletions(-)
This still applies without conflict after Peter's 18-patch series (I was a bit surprised on that point, but a nice surprise); hopefully that means that it doesn't matter whether you commit before or after Peter. The fact that we don't have any testsuite coverage of this directly in 'make check' is a bit disconcerting; but at least the external regression testing is covering it. However, I'm still a bit worried that we are just attacking the symptoms, instead of addressing things correctly. It seems like we should be smarter in storage_backend_* to not override a capacity that was already set when probing the file metadata. There's a pretty telling comment in virStorageFileGetMetadataInternal: /* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */ where I think that being a bit smarter about tracing which pieces of information are gathered in which order, and then prioritizing them so that metadata information takes priority over stat() information, could avoid the need to pass an updateCapacity flag through quite so many layers of function calls. We may still go with your patch, but let's wait a day or two to see if I can come up with something more elegant... -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Apr 23, 2014 at 11:58:44AM -0600, Eric Blake wrote:
On 04/23/2014 07:28 AM, John Ferlan wrote:
Commit id 'ac9a0963' refactored out the 'withCapacity' for the virStorageBackendUpdateVolInfo() API. See:
Fortunately, we haven't released this regression of mine :)
http://www.redhat.com/archives/libvir-list/2014-April/msg00043.html
This resulted in a difference in how 'virsh vol-info --pool <poolName> <volume>' or 'virsh vol-list vol-list --pool <poolName> --details' outputs
s/vol-list vol-list/vol-list/
the capacity information for a directory pool with a qcow2 sparse file.
Results in listing a Capacity value. Prior to the commit, the value would be '1.0 MiB' (1048576 bytes). However, after the commit the output would be (for example) '192.50 KiB', which for my system was the size of the volume in my file system (eg 'ls -l TestPool/temp_vol_1' results in '197120' bytes or 192.50 KiB). While perhaps technically correct, it's not necessarily what the user expected (certainly virt-test didn't expect it).
This patch restores the code to not update the target capacity for this path
More precisely, the code needs to be careful that we prefer capacity information learned from file format metadata (qcow2), and fall back to capacity from file system data; my refactoring accidentally swapped things so that file system capacity took precedence over metadata capacity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 22 +++++++++++++++------- src/storage/storage_backend.h | 5 ++++- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 11 +++++++---- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 31 insertions(+), 17 deletions(-)
This still applies without conflict after Peter's 18-patch series (I was a bit surprised on that point, but a nice surprise); hopefully that means that it doesn't matter whether you commit before or after Peter.
The fact that we don't have any testsuite coverage of this directly in 'make check' is a bit disconcerting; but at least the external regression testing is covering it.
However, I'm still a bit worried that we are just attacking the symptoms, instead of addressing things correctly. It seems like we should be smarter in storage_backend_* to not override a capacity that was already set when probing the file metadata. There's a pretty telling comment in virStorageFileGetMetadataInternal:
/* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */
where I think that being a bit smarter about tracing which pieces of information are gathered in which order, and then prioritizing them so that metadata information takes priority over stat() information, could avoid the need to pass an updateCapacity flag through quite so many layers of function calls.
We may still go with your patch, but let's wait a day or two to see if I can come up with something more elegant...
Can we get a unit test for this disk probing scenario so we can catch any future regression too. 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 :|
On 04/24/2014 02:11 AM, Daniel P. Berrange wrote:
On Wed, Apr 23, 2014 at 11:58:44AM -0600, Eric Blake wrote:
On 04/23/2014 07:28 AM, John Ferlan wrote:
Commit id 'ac9a0963' refactored out the 'withCapacity' for the virStorageBackendUpdateVolInfo() API. See:
Fortunately, we haven't released this regression of mine :)
However, I'm still a bit worried that we are just attacking the symptoms, instead of addressing things correctly. It seems like we should be smarter in storage_backend_* to not override a capacity that was already set when probing the file metadata. There's a pretty telling comment in virStorageFileGetMetadataInternal:
/* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */
where I think that being a bit smarter about tracing which pieces of information are gathered in which order, and then prioritizing them so that metadata information takes priority over stat() information, could avoid the need to pass an updateCapacity flag through quite so many layers of function calls.
After more searching, I've learned that when the storage volume is first populated, we are getting things right (all volumes went through virStorageBackendProbeTarget which populated capacity first from the file system then updated it from the metadata); but later when it is time for virsh vol-dumpxml, virStorageBackendUpdateVolTargetInfo is called to refresh the data but does not look at the metadata, thus overwriting things back to the file size. If a volume has been resized by 'virsh vol-resize' in the meantime, neither the original capacity, nor the the file size capacity are correct - the only correct solution is to have the backend once again read the metadata. I'm still trying to figure out the best way to do that, but posted a couple patches along the way while still working on the issue.
We may still go with your patch, but let's wait a day or two to see if I can come up with something more elegant...
Can we get a unit test for this disk probing scenario so we can catch any future regression too.
The existing tests/virstoragetest.c validated that we are getting capacity correct when probing; where it failed was that it doesn't test that the higher level vol-dumpxml refresh uses the information vs. throwing it away. Yes, I plan to enhance the testsuite to ensure this case gets covered. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/23/2014 11:58 AM, Eric Blake wrote:
On 04/23/2014 07:28 AM, John Ferlan wrote:
Commit id 'ac9a0963' refactored out the 'withCapacity' for the virStorageBackendUpdateVolInfo() API. See:
Fortunately, we haven't released this regression of mine :)
http://www.redhat.com/archives/libvir-list/2014-April/msg00043.html
However, I'm still a bit worried that we are just attacking the symptoms, instead of addressing things correctly. It seems like we should be smarter in storage_backend_* to not override a capacity that was already set when probing the file metadata. There's a pretty telling comment in virStorageFileGetMetadataInternal:
/* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */
where I think that being a bit smarter about tracing which pieces of information are gathered in which order, and then prioritizing them so that metadata information takes priority over stat() information, could avoid the need to pass an updateCapacity flag through quite so many layers of function calls.
We may still go with your patch, but let's wait a day or two to see if I can come up with something more elegant...
I have run out of time to come up with anything more elegant that doesn't feel like it is violating release candidate freeze, so ACK to your patch for 1.2.4. We can then tackle the problem more fully (possibly reverting your patch for a nicer more complex solution) in 1.2.5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/02/2014 12:18 AM, Eric Blake wrote:
I have run out of time to come up with anything more elegant that doesn't feel like it is violating release candidate freeze, so ACK to your patch for 1.2.4. We can then tackle the problem more fully (possibly reverting your patch for a nicer more complex solution) in 1.2.5.
OK - I have pushed this change. John
participants (3)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan