On 02/07/2012 03:17 PM, Eric Blake wrote:
> On 02/07/2012 12:55 PM, Cole Robinson wrote:
>> Input to the volume cloning code is a source volume and an XML
>> descriptor for the new volume. It is possible for the new volume
>> to have a greater size than source volume, at which point libvirt
>> will just stick 0s on the end of the new image (for raw format
>> anyways).
>>
>> Unfortunately a logic error messed up our tracking of the of the
>> excess amount that needed to be written: end result is that sparse
>> clones were made very much non-sparse, and cloning regular disk
>> images could end up excessively sized (though data unaltered).
>>
>> Drop the 'remain' variable entriely here since it's redundant, and
>
> s/entriely/entirely/
>
>> track actual allocation directly against the desired 'total'.
>> ---
>> src/storage/storage_backend.c | 11 +++--------
>> 1 files changed, 3 insertions(+), 8 deletions(-)
>
> Hmm - given that we just added virStorageVolResize, with a flag
> VIR_STORAGE_VOL_RESIZE_ALLOCATE, where the default operation is a sparse
> resize but the flag requests a non-sparse allocation, I'm wondering if
> virStorageVolCreateXMLFrom should also be taught to use a flag that
> controls whether or not we attempt to sparsify the output (the default
> of sparse is nicer, as it is likely to be faster, but there are some
> cases where a sparse file is undesirable because you want to know up
> front if you are going to hit ENOSPC errors, rather than overcommitting
> due to sparse files that later grow).
>
>>
>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> index 1c22112..caac2f8 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -127,7 +127,6 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>> int inputfd = -1;
>> int amtread = -1;
>> int ret = 0;
>> - unsigned long long remain;
>> size_t rbytes = READ_BLOCK_SIZE_DEFAULT;
>> size_t wbytes = 0;
>> int interval;
>> @@ -165,13 +164,11 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>> goto cleanup;
>> }
>>
>> - remain = *total;
>> -
>> while (amtread != 0) {
>> int amtleft;
>>
>> - if (remain < rbytes)
>> - rbytes = remain;
>> + if (*total < rbytes)
>> + rbytes = *total;
>>
>> if ((amtread = saferead(inputfd, buf, rbytes)) < 0) {
>> ret = -errno;
>> @@ -180,7 +177,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>> inputvol->target.path);
>> goto cleanup;
>> }
>> - remain -= amtread;
>> + *total -= amtread;
>>
>> /* Loop over amt read in 512 byte increments, looking for sparse
>> * blocks */
>> @@ -225,8 +222,6 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
>> }
>> inputfd = -1;
>>
>> - *total -= remain;
>
> If I'm understanding you correctly, this line was the logic error: it
> should have been '*total = remain' instead of using -=.
>
> The only reason for using remain instead of total would have then been
> to avoid pointer dereferencing in the loop, but I don't think it's on a
> hot enough path to make a difference, so I'm okay with your patch as-is
> (although we should still be thinking about a flag that controls whether
> the user can request non-sparse clones).
>
As you OK'd this on IRC, I've pushed it now:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=0ed86cfb514fea6d6c6182fec86a632ff0fa3062
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list