[libvirt] [PATCH] storage: Update pool metadata after adding/removing/resizing volume
RHEL6.5: https://bugzilla.redhat.com/show_bug.cgi?id=965442 One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done. v1: https://www.redhat.com/archives/libvir-list/2013-May/msg01957.html --- src/storage/storage_driver.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 72786dd..7908ba6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1507,6 +1507,7 @@ storageVolCreateXML(virStoragePoolPtr obj, virStorageBackendPtr backend; virStorageVolDefPtr voldef = NULL; virStorageVolPtr ret = NULL, volobj = NULL; + virStorageVolDefPtr buildvoldef = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -1565,20 +1566,19 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; } - if (backend->buildVol) { - int buildret; - virStorageVolDefPtr buildvoldef = NULL; + if (VIR_ALLOC(buildvoldef) < 0) { + voldef = NULL; + goto cleanup; + } - if (VIR_ALLOC(buildvoldef) < 0) { - voldef = NULL; - goto cleanup; - } + /* Make a shallow copy of the 'defined' volume definition, since the + * original allocation value will change as the user polls 'info', + * but we only need the initial requested values + */ + memcpy(buildvoldef, voldef, sizeof(*voldef)); - /* Make a shallow copy of the 'defined' volume definition, since the - * original allocation value will change as the user polls 'info', - * but we only need the initial requested values - */ - memcpy(buildvoldef, voldef, sizeof(*voldef)); + if (backend->buildVol) { + int buildret; /* Drop the pool lock during volume allocation */ pool->asyncjobs++; @@ -1595,7 +1595,6 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--; voldef = NULL; - VIR_FREE(buildvoldef); if (buildret < 0) { virStoragePoolObjUnlock(pool); @@ -1606,6 +1605,10 @@ storageVolCreateXML(virStoragePoolPtr obj, } + /* Update pool metadata */ + pool->def->allocation += buildvoldef->allocation; + pool->def->available -= buildvoldef->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -1615,6 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj, cleanup: virObjectUnref(volobj); virStorageVolDefFree(voldef); + virStorageVolDefFree(buildvoldef); if (pool) virStoragePoolObjUnlock(pool); return ret; @@ -1770,6 +1774,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } + /* Updating pool metadata */ + pool->def->allocation += newvol->allocation; + pool->def->available -= newvol->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -2013,6 +2021,11 @@ storageVolResize(virStorageVolPtr obj, goto out; vol->capacity = abs_capacity; + + /* Update pool metadata */ + pool->def->allocation += (abs_capacity - vol->capacity); + pool->def->available -= (abs_capacity - vol->capacity); + ret = 0; out: @@ -2356,6 +2369,10 @@ storageVolDelete(virStorageVolPtr obj, if (backend->deleteVol(obj->conn, pool, vol, flags) < 0) goto cleanup; + /* Update pool metadata */ + pool->def->allocation -= vol->allocation; + pool->def->available += vol->allocation; + for (i = 0; i < pool->volumes.count; i++) { if (pool->volumes.objs[i] == vol) { VIR_INFO("Deleting volume '%s' from storage pool '%s'", -- 1.8.1.4
On 08/16/2013 06:08 AM, Osier Yang wrote:
Links to a private bug aren't considered very nice on a public list or git repo.
One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done.
But at least this is a fair summary of its contents.
v1: https://www.redhat.com/archives/libvir-list/2013-May/msg01957.html
This is helpful for review, but is generally placed...
---
after the separator, so that it appears only in the mail but is not incorporated by 'git am' (no one cares 6 months from now how many revisions of the patch went to the mailing list before it was polished). ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 17/08/13 03:57, Eric Blake wrote:
On 08/16/2013 06:08 AM, Osier Yang wrote:
RHEL6.5: https://bugzilla.redhat.com/show_bug.cgi?id=965442 Links to a private bug aren't considered very nice on a public list or git repo.
Oh, I didn't notice it.
One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done. But at least this is a fair summary of its contents.
v1: https://www.redhat.com/archives/libvir-list/2013-May/msg01957.html This is helpful for review, but is generally placed...
--- after the separator, so that it appears only in the mail but is not incorporated by 'git am' (no one cares 6 months from now how many revisions of the patch went to the mailing list before it was polished).
I pushed with the bug and v2 uris removed. Thanks. Osier
On 08/16/2013 08:08 AM, Osier Yang wrote:
RHEL6.5: https://bugzilla.redhat.com/show_bug.cgi?id=965442
One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done.
v1: https://www.redhat.com/archives/libvir-list/2013-May/msg01957.html --- src/storage/storage_driver.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 72786dd..7908ba6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1507,6 +1507,7 @@ storageVolCreateXML(virStoragePoolPtr obj, virStorageBackendPtr backend; virStorageVolDefPtr voldef = NULL; virStorageVolPtr ret = NULL, volobj = NULL; + virStorageVolDefPtr buildvoldef = NULL;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1565,20 +1566,19 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; }
- if (backend->buildVol) { - int buildret; - virStorageVolDefPtr buildvoldef = NULL; + if (VIR_ALLOC(buildvoldef) < 0) { + voldef = NULL; + goto cleanup; + }
- if (VIR_ALLOC(buildvoldef) < 0) { - voldef = NULL; - goto cleanup; - } + /* Make a shallow copy of the 'defined' volume definition, since the + * original allocation value will change as the user polls 'info', + * but we only need the initial requested values + */ + memcpy(buildvoldef, voldef, sizeof(*voldef));
- /* Make a shallow copy of the 'defined' volume definition, since the - * original allocation value will change as the user polls 'info', - * but we only need the initial requested values - */ - memcpy(buildvoldef, voldef, sizeof(*voldef)); + if (backend->buildVol) { + int buildret;
/* Drop the pool lock during volume allocation */ pool->asyncjobs++; @@ -1595,7 +1595,6 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--;
voldef = NULL; - VIR_FREE(buildvoldef);
if (buildret < 0) { virStoragePoolObjUnlock(pool); @@ -1606,6 +1605,10 @@ storageVolCreateXML(virStoragePoolPtr obj,
}
+ /* Update pool metadata */ + pool->def->allocation += buildvoldef->allocation; + pool->def->available -= buildvoldef->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -1615,6 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj, cleanup: virObjectUnref(volobj); virStorageVolDefFree(voldef); + virStorageVolDefFree(buildvoldef); if (pool) virStoragePoolObjUnlock(pool); return ret; @@ -1770,6 +1774,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; }
+ /* Updating pool metadata */ + pool->def->allocation += newvol->allocation; + pool->def->available -= newvol->allocation; +
Coverity found a FORWARD_NULL issue - at line 1761 there's a : (36) Event assign_zero: Assigning: "newvol" = "NULL". Also see events: [var_deref_op] 1761 newvol = NULL; 1762 pool->asyncjobs--; 1763 which will lead to problems at these lines. John
VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -2013,6 +2021,11 @@ storageVolResize(virStorageVolPtr obj, goto out;
vol->capacity = abs_capacity; + + /* Update pool metadata */ + pool->def->allocation += (abs_capacity - vol->capacity); + pool->def->available -= (abs_capacity - vol->capacity); + ret = 0;
out: @@ -2356,6 +2369,10 @@ storageVolDelete(virStorageVolPtr obj, if (backend->deleteVol(obj->conn, pool, vol, flags) < 0) goto cleanup;
+ /* Update pool metadata */ + pool->def->allocation -= vol->allocation; + pool->def->available += vol->allocation; + for (i = 0; i < pool->volumes.count; i++) { if (pool->volumes.objs[i] == vol) { VIR_INFO("Deleting volume '%s' from storage pool '%s'",
On 08/16/2013 02:08 PM, Osier Yang wrote:
RHEL6.5: https://bugzilla.redhat.com/show_bug.cgi?id=965442
One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done.
v1: https://www.redhat.com/archives/libvir-list/2013-May/msg01957.html --- src/storage/storage_driver.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 72786dd..7908ba6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1507,6 +1507,7 @@ storageVolCreateXML(virStoragePoolPtr obj, virStorageBackendPtr backend; virStorageVolDefPtr voldef = NULL; virStorageVolPtr ret = NULL, volobj = NULL; + virStorageVolDefPtr buildvoldef = NULL;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1565,20 +1566,19 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; }
- if (backend->buildVol) { - int buildret; - virStorageVolDefPtr buildvoldef = NULL; + if (VIR_ALLOC(buildvoldef) < 0) { + voldef = NULL; + goto cleanup; + }
- if (VIR_ALLOC(buildvoldef) < 0) { - voldef = NULL; - goto cleanup; - } + /* Make a shallow copy of the 'defined' volume definition, since the + * original allocation value will change as the user polls 'info', + * but we only need the initial requested values + */ + memcpy(buildvoldef, voldef, sizeof(*voldef));
- /* Make a shallow copy of the 'defined' volume definition, since the - * original allocation value will change as the user polls 'info', - * but we only need the initial requested values - */ - memcpy(buildvoldef, voldef, sizeof(*voldef)); + if (backend->buildVol) { + int buildret;
/* Drop the pool lock during volume allocation */ pool->asyncjobs++; @@ -1595,7 +1595,6 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--;
voldef = NULL; - VIR_FREE(buildvoldef);
You changed this VIR_FREE...
if (buildret < 0) { virStoragePoolObjUnlock(pool); @@ -1606,6 +1605,10 @@ storageVolCreateXML(virStoragePoolPtr obj,
}
+ /* Update pool metadata */ + pool->def->allocation += buildvoldef->allocation; + pool->def->available -= buildvoldef->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -1615,6 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj, cleanup: virObjectUnref(volobj); virStorageVolDefFree(voldef); + virStorageVolDefFree(buildvoldef);
...into virStorageVolDefFree that frees the pointers that are now present in pool->volumes.objs[i].
if (pool) virStoragePoolObjUnlock(pool); return ret;
Jan
On Fri, Aug 16, 2013 at 08:08:07PM +0800, Osier Yang wrote:
RHEL6.5: https://bugzilla.redhat.com/show_bug.cgi?id=965442
One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done.
I didn't look more closely but it seems this broke libvirt-tck: http://honk.sigxcpu.org:8001/job/libvirt-tck-debian-wheezy-qemu-session/1351... If you look at the libvirtd.log http://honk.sigxcpu.org:8001/job/libvirt-tck-debian-wheezy-qemu-session/1351... the volume name looks rather weird: info : storageVolDelete:2379 : Deleting volume '8Ü!¹àÂ,¹@' from storage pool 'tck' after this patch got applied. Cheers, -- Guido
v1: https://www.redhat.com/archives/libvir-list/2013-May/msg01957.html --- src/storage/storage_driver.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 72786dd..7908ba6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1507,6 +1507,7 @@ storageVolCreateXML(virStoragePoolPtr obj, virStorageBackendPtr backend; virStorageVolDefPtr voldef = NULL; virStorageVolPtr ret = NULL, volobj = NULL; + virStorageVolDefPtr buildvoldef = NULL;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1565,20 +1566,19 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; }
- if (backend->buildVol) { - int buildret; - virStorageVolDefPtr buildvoldef = NULL; + if (VIR_ALLOC(buildvoldef) < 0) { + voldef = NULL; + goto cleanup; + }
- if (VIR_ALLOC(buildvoldef) < 0) { - voldef = NULL; - goto cleanup; - } + /* Make a shallow copy of the 'defined' volume definition, since the + * original allocation value will change as the user polls 'info', + * but we only need the initial requested values + */ + memcpy(buildvoldef, voldef, sizeof(*voldef));
- /* Make a shallow copy of the 'defined' volume definition, since the - * original allocation value will change as the user polls 'info', - * but we only need the initial requested values - */ - memcpy(buildvoldef, voldef, sizeof(*voldef)); + if (backend->buildVol) { + int buildret;
/* Drop the pool lock during volume allocation */ pool->asyncjobs++; @@ -1595,7 +1595,6 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--;
voldef = NULL; - VIR_FREE(buildvoldef);
if (buildret < 0) { virStoragePoolObjUnlock(pool); @@ -1606,6 +1605,10 @@ storageVolCreateXML(virStoragePoolPtr obj,
}
+ /* Update pool metadata */ + pool->def->allocation += buildvoldef->allocation; + pool->def->available -= buildvoldef->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -1615,6 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj, cleanup: virObjectUnref(volobj); virStorageVolDefFree(voldef); + virStorageVolDefFree(buildvoldef); if (pool) virStoragePoolObjUnlock(pool); return ret; @@ -1770,6 +1774,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; }
+ /* Updating pool metadata */ + pool->def->allocation += newvol->allocation; + pool->def->available -= newvol->allocation; + VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); ret = volobj; @@ -2013,6 +2021,11 @@ storageVolResize(virStorageVolPtr obj, goto out;
vol->capacity = abs_capacity; + + /* Update pool metadata */ + pool->def->allocation += (abs_capacity - vol->capacity); + pool->def->available -= (abs_capacity - vol->capacity); + ret = 0;
out: @@ -2356,6 +2369,10 @@ storageVolDelete(virStorageVolPtr obj, if (backend->deleteVol(obj->conn, pool, vol, flags) < 0) goto cleanup;
+ /* Update pool metadata */ + pool->def->allocation -= vol->allocation; + pool->def->available += vol->allocation; + for (i = 0; i < pool->volumes.count; i++) { if (pool->volumes.objs[i] == vol) { VIR_INFO("Deleting volume '%s' from storage pool '%s'", -- 1.8.1.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 19, 2013 at 06:12:00PM +0200, Guido Günther wrote:
On Fri, Aug 16, 2013 at 08:08:07PM +0800, Osier Yang wrote:
RHEL6.5: https://bugzilla.redhat.com/show_bug.cgi?id=965442
One has to refresh the pool to get the correct pool info after adding/removing/resizing a volume, this updates the pool metadata (allocation, available) after those operation are done.
I didn't look more closely but it seems this broke libvirt-tck:
http://honk.sigxcpu.org:8001/job/libvirt-tck-debian-wheezy-qemu-session/1351...
If you look at the libvirtd.log
http://honk.sigxcpu.org:8001/job/libvirt-tck-debian-wheezy-qemu-session/1351...
the volume name looks rather weird:
info : storageVolDelete:2379 : Deleting volume '8Ü!¹àÂ,¹@' from storage pool 'tck'
after this patch got applied.
Yep, this is caused by the issue Jan pointed out in his reply. We're free'ing data that is still in use. 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 :|
participants (6)
-
Daniel P. Berrange -
Eric Blake -
Guido Günther -
John Ferlan -
Ján Tomko -
Osier Yang