[libvirt] [PATCH 2/3] virStorageFileResize: fallocate the whole capacity

[Ján Tomko]

On Tue, Sep 26, 2017 at 06:44:55PM -0400, John Ferlan wrote:
>
>
>On 09/25/2017 11:46 AM, Ján Tomko wrote:
>> We have been trying to implement the ALLOCATE flag to mean
>
>Is this the colloquial "we"? ;-)
>

We believe so.

>> "the volume should be fully allocated after the resize".
>
>commit id 'aa2a4cff' added support for ALLOCATE, but it seems that code
>was wrong, then?
>
>The docs are somewhat vague regarding the ALLOCATE flag:
>
>virStorageVolResize:
>...
> * Normally, the operation will attempt to affect capacity with a minimum
> * impact on allocation (that is, the default operation favors a sparse
> * resize).  If @flags contains VIR_STORAGE_VOL_RESIZE_ALLOCATE, then the
> * operation will ensure that allocation is sufficient for the new
> * capacity; this may make the operation take noticeably longer.
>
>and:
>
>VIR_STORAGE_VOL_RESIZE_ALLOCATE = 1 << 0, /* force allocation of new size */
>
>
>What really isn't clear is whether the new allocation wants to preserve
>some sort of sparseness factor as well. IOW: If you have a sparse file
>before you start, then shouldn't you have a sparse file when you're done?
>

Mixing sparseness in the original part and allocation in the new part
does not seem that useful and it does not "ensure that the allocation is
sufficient for the new capacity".

>>
>> Since commit b0579ed9 we do not allocate from the existing
>> capacity, but from the existing allocation value.
>> However this value is a total of all the allocated bytes,
>> not an offset.
>>
>> For a sparsely allocated file:
>> $ perl -e 'print "x"x8192;' > vol1
>> $ fallocate -p -o 0 -l 4096 vol1
>>
>> Treating allocation as an offset would result in an incompletely
>> allocated file:
>> $ virsh vol-resize vol1 --pool default 16384 --allocate
>> Capacity:       16.00 KiB
>> Allocation:     12.00 KiB
>
>Yep, would have expected an allocation of 16KiB here...
>
>>
>> Call fallocate from zero on the whole requested capacity to fully
>> allocate the file.
>
>FWIW: complete output/example because I'm a visual learner...
>
>Before:
>
>$ perl -e 'print "x"x8192;' > /path/to/default/vol1
>$ virsh pool-refresh default
>$ virsh vol-info default vol1
>Capacity:       8.00 KiB
>Allocation:     8.00 KiB
>
>$ fallocate -p -o 0 -l 4096 /path/to/default/vol1
>$ virsh vol-info vol1 default
>Capacity:       8.00 KiB
>Allocation:     4.00 KiB
>
>$ virsh vol-resize vol1 --pool default 16384 --allocate
>$ virsh vol-info vol1 default
>Capacity:       16.00 KiB
>Allocation:     12.00 KiB
>
>After the patch:
>Only the vol-resize output changes to:
>
>Capacity:       16.00 KiB
>Allocation:     16.00 KiB
>
>Which would seem to be correct. Still I wonder if the initial changes
>got things "wrong" by trying to be cute and adding even though DELTA is
>dealt with much earlier.
>
>The question being - should the original code adjusted both capacity and
>allocation getting the difference between the two as the additional
>capacity in order to preserve whatever sparseness may have existed
>previously or in this case a capacity=24KiB and Allocation=20KiB.  Oh
>well we may never know...
>
>
>> ---
>>  src/storage/storage_util.c | 3 +--
>>  src/util/virstoragefile.c  | 8 +-------
>>  src/util/virstoragefile.h  | 1 -
>>  3 files changed, 2 insertions(+), 10 deletions(-)
>>
>
>I guess it's a long way of me giving:
>
>Reviewed-by: John Ferlan <jferlan at redhat.com>
>
>But I needed to convince myself first...  and if you feel the need to
>alter the commit message a bit to pull in more details that's even
>better (for me), but I suppose the current one works too.
>

I have added some more examples to the commit message and pushed the
series

Thanks!

Jan

>John
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170927/c9bb94e6/attachment-0001.sig>