Re: [libvirt] [PATCH 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.

2015-06-25 Thread Prerna Saxena 
Hi Jan,
Thanks for the review comments.
On Tuesday 23 June 2015 06:21 PM, Ján Tomko wrote:
 On Mon, Jun 22, 2015 at 05:07:26PM +0530, Prerna Saxena wrote:
 From: Prerna Saxena pre...@linux.vnet.ibm.com
 Date: Thu, 18 Jun 2015 05:05:09 -0500

 Libvirt periodically refreshes all volumes in a storage pool, including
 the volumes being cloned.
 While cloning a storage volume from parent, we drop pool locks. Subsequent
 volume refresh sometimes changes allocation for an ongoing copy, and leads
 to corrupt images.
 Fix: Introduce a shadow volume that isolates the volume object under refresh
 from the base which has a copy ongoing.

 Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
 ---
  src/storage/storage_driver.c | 15 +--
  1 file changed, 13 insertions(+), 2 deletions(-)

 diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
 index 57060ab..4980546 100644
 --- a/src/storage/storage_driver.c
 +++ b/src/storage/storage_driver.c
 @@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
  {
  virStoragePoolObjPtr pool, origpool = NULL;
  virStorageBackendPtr backend;
 -virStorageVolDefPtr origvol = NULL, newvol = NULL;
 +virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
  virStorageVolPtr ret = NULL, volobj = NULL;
  unsigned long long allocation;
  int buildret;
 @@ -2010,6 +2010,17 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
  if (backend-createVol(obj-conn, pool, newvol)  0)
  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
 + */
 +if (VIR_ALLOC(shadowvol)  0) {
 +newvol = NULL;
 newvol has not been added to pool-volumes.objs yet, so we should free
 it on the cleanup path. shadowvol should also be VIR_FREE'd.

Thanks, I'd missed this. Will be addressed in subsequent patch.
 +goto cleanup;
 +}
 +
 +memcpy(shadowvol, newvol, sizeof(*newvol));
 +
  pool-volumes.objs[pool-volumes.count++] = newvol;
  volobj = virGetStorageVol(obj-conn, pool-def-name, newvol-name,
newvol-key, NULL, NULL);
 @@ -2029,7 +2040,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
  virStoragePoolObjUnlock(origpool);
  }
  
 -buildret = backend-buildVolFrom(obj-conn, pool, newvol, origvol, 
 flags);
 +buildret = backend-buildVolFrom(obj-conn, pool, shadowvol, origvol, 
 flags);
  
 A few lines below, there is one more usage of newvol after the pool has
 been unlocked: newvol-target.allocation

 If the parallel volume refresh happened when the volume was not fully
 allocated, this might not contain the intended allocation.

 Using shadowvol-target.allocation will behave the same regardless of a
 parallel refresh (even though the buildVolFrom function might not honor
 the allocation exactly).


Right. The buildVolFrom() call completes an fsync and then returns. In my 
experimental runs, a parallel refresh would always happen by the time I got to 
this point; so this had missed me. But ofcourse
we can never say that for sure. Will fix in the next patch.

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.

2015-06-23 Thread Ján Tomko 
On Mon, Jun 22, 2015 at 05:07:26PM +0530, Prerna Saxena wrote:
 From: Prerna Saxena pre...@linux.vnet.ibm.com
 Date: Thu, 18 Jun 2015 05:05:09 -0500
 
 Libvirt periodically refreshes all volumes in a storage pool, including
 the volumes being cloned.
 While cloning a storage volume from parent, we drop pool locks. Subsequent
 volume refresh sometimes changes allocation for an ongoing copy, and leads
 to corrupt images.
 Fix: Introduce a shadow volume that isolates the volume object under refresh
 from the base which has a copy ongoing.
 
 Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
 ---
  src/storage/storage_driver.c | 15 +--
  1 file changed, 13 insertions(+), 2 deletions(-)
 
 diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
 index 57060ab..4980546 100644
 --- a/src/storage/storage_driver.c
 +++ b/src/storage/storage_driver.c
 @@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
  {
  virStoragePoolObjPtr pool, origpool = NULL;
  virStorageBackendPtr backend;
 -virStorageVolDefPtr origvol = NULL, newvol = NULL;
 +virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
  virStorageVolPtr ret = NULL, volobj = NULL;
  unsigned long long allocation;
  int buildret;
 @@ -2010,6 +2010,17 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
  if (backend-createVol(obj-conn, pool, newvol)  0)
  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
 + */
 +if (VIR_ALLOC(shadowvol)  0) {
 +newvol = NULL;

newvol has not been added to pool-volumes.objs yet, so we should free
it on the cleanup path. shadowvol should also be VIR_FREE'd.

 +goto cleanup;
 +}
 +
 +memcpy(shadowvol, newvol, sizeof(*newvol));
 +
  pool-volumes.objs[pool-volumes.count++] = newvol;
  volobj = virGetStorageVol(obj-conn, pool-def-name, newvol-name,
newvol-key, NULL, NULL);
 @@ -2029,7 +2040,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
  virStoragePoolObjUnlock(origpool);
  }
  
 -buildret = backend-buildVolFrom(obj-conn, pool, newvol, origvol, 
 flags);
 +buildret = backend-buildVolFrom(obj-conn, pool, shadowvol, origvol, 
 flags);
  

A few lines below, there is one more usage of newvol after the pool has
been unlocked: newvol-target.allocation

If the parallel volume refresh happened when the volume was not fully
allocated, this might not contain the intended allocation.

Using shadowvol-target.allocation will behave the same regardless of a
parallel refresh (even though the buildVolFrom function might not honor
the allocation exactly).

Jan


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.

2015-06-22 Thread Prerna Saxena 
From: Prerna Saxena pre...@linux.vnet.ibm.com
Date: Thu, 18 Jun 2015 05:05:09 -0500

Libvirt periodically refreshes all volumes in a storage pool, including
the volumes being cloned.
While cloning a storage volume from parent, we drop pool locks. Subsequent
volume refresh sometimes changes allocation for an ongoing copy, and leads
to corrupt images.
Fix: Introduce a shadow volume that isolates the volume object under refresh
from the base which has a copy ongoing.

Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
---
 src/storage/storage_driver.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 57060ab..4980546 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 {
 virStoragePoolObjPtr pool, origpool = NULL;
 virStorageBackendPtr backend;
-virStorageVolDefPtr origvol = NULL, newvol = NULL;
+virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
 virStorageVolPtr ret = NULL, volobj = NULL;
 unsigned long long allocation;
 int buildret;
@@ -2010,6 +2010,17 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 if (backend-createVol(obj-conn, pool, newvol)  0)
 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
+ */
+if (VIR_ALLOC(shadowvol)  0) {
+newvol = NULL;
+goto cleanup;
+}
+
+memcpy(shadowvol, newvol, sizeof(*newvol));
+
 pool-volumes.objs[pool-volumes.count++] = newvol;
 volobj = virGetStorageVol(obj-conn, pool-def-name, newvol-name,
   newvol-key, NULL, NULL);
@@ -2029,7 +2040,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 virStoragePoolObjUnlock(origpool);
 }
 
-buildret = backend-buildVolFrom(obj-conn, pool, newvol, origvol, flags);
+buildret = backend-buildVolFrom(obj-conn, pool, shadowvol, origvol, 
flags);
 
 storageDriverLock();
 virStoragePoolObjLock(pool);
-- 
1.8.3.1

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list