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).
>
Yeah not a bad idea for completeness sake, but IMO it's probably not worth the
effort unless someone is actually asking for it.
The default here should probably be better in that it only sparsifys the bits
of the image that are actually seen as sparse by the host filesystem. No idea
how to detect that though. Either way I don't forsee a tool like virt-manager
ever pushing this choice to the user, just deferring to libvirt's default
behavior (like we do now).
>>
>> 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 -=.
>
Yup
> 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).
>
Is that an ACK for commit? :)
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list