On Tuesday 30 June 2015 06:20 PM, Ján Tomko wrote:

On Fri, Jun 26, 2015 at 05:05:11PM +0530, Prerna Saxena wrote:

...

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 <prerna@linux.vnet.ibm.com> --- src/storage/storage_driver.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

ACK

...

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 57060ab..41de8df 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,15 @@ 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) + 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 +2038,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjUnlock(origpool); }

- buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags); + buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol, flags);

newvol->target.allocation should also use the value from shadowvol. I have made the adjustment and pushed the patch.

Jan

Hi Jan, Thanks. Could you also take a look at the second patch in this series ? Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India

On Fri, Jun 26, 2015 at 05:13:26PM +0530, Prerna Saxena wrote:

When virsh vol-clone is attempted on a raw file where capacity > allocation, the resulting cloned volume has a size that matches the virtual-size of the parent; in place of matching its actual, disk size. This patch fixes the cloned disk to have same _allocated_size_ as the parent file from which it was cloned.

Ref: http://www.redhat.com/archives/libvir-list/2015-May/msg00050.html

Also fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1130739

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/storage/storage_backend.c | 23 ++++++++++------------- src/storage/storage_driver.c | 5 ----- 2 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ce59f63..492b344 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -342,7 +342,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; }

- remain = vol->target.allocation; + remain = vol->target.capacity;

if (inputvol) { int res = virStorageBackendCopyToFD(vol, inputvol, @@ -397,7 +397,8 @@ createRawFile(int fd, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, bool reflink_copy) { - bool need_alloc = true; + bool want_sparse = (inputvol && + (inputvol->target.capacity > vol->target.allocation)); int ret = 0; unsigned long long remain;

@@ -420,10 +421,9 @@ createRawFile(int fd, virStorageVolDefPtr vol, * to writing zeroes block by block in case fallocate isn't * available, and since we're going to copy data from another * file it doesn't make sense to write the file twice. */ - if (vol->target.allocation) { - if (fallocate(fd, 0, 0, vol->target.allocation) == 0) { - need_alloc = false; - } else if (errno != ENOSYS && errno != EOPNOTSUPP) { + if (vol->target.allocation && !want_sparse) { + if ((fallocate(fd, 0, 0, vol->target.allocation) != 0) &&

If fallocate succeeds here, there's no need to copy zero bytes from the source.

+ (errno != ENOSYS && errno != EOPNOTSUPP)) { ret = -errno; virReportSystemError(errno, _("cannot allocate %llu bytes in file '%s'"), @@ -433,22 +433,19 @@ createRawFile(int fd, virStorageVolDefPtr vol, } #endif

- remain = vol->target.allocation; + remain = vol->target.capacity;

This breaks sparse file creation on systems without fallocate.

if (inputvol) { /* allow zero blocks to be skipped if we've requested sparse - * allocation (allocation < capacity) or we have already - * been able to allocate the required space. */ - bool want_sparse = !need_alloc || - (vol->target.allocation < inputvol->target.capacity); - + * allocation (allocation < capacity) + */ ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain, want_sparse, reflink_copy);

I'd rahter use !need_alloc instead of want_sparse here, because sparseness only makes sense for cloning volumes, but the bool affects creating new volumes as well.

if (ret < 0) goto cleanup; }

- if (remain && need_alloc) { + if (remain && !want_sparse) { if (safezero(fd, vol->target.allocation - remain, remain) < 0) {

This calculation assumes that 'remain' contains the bytes remaining to the target allocation, but after this patch it contains the bytes remaining to the target capacity.

ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"),

I'll send a v4 shortly. Jan