[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] storage: Don't unsparsify images when cloning



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]