Current comment is not clear enough: which sparseness is meant, coming
from sparse image format or from sparse file system?

For example, if we have qcow2 above raw file on non-sparse file system,
this function will say nothing about unallocated (by qcow2 layer)
clusters.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 50ba264143..ba22fc0dfb 100644
--- a/block.c
+++ b/block.c
@@ -3388,8 +3388,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
 }
 
 /**
- * Length of a allocated file in bytes. Sparse files are counted by actual
- * allocated space. Return < 0 if error or unknown.
+ * Size of allocated in underlying file system area. Sparseness is taken into
+ * account for sparse file systems. Return < 0 if error or unknown.
  */
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
 {
-- 
2.11.1

On 05/25/2017 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> Current comment is not clear enough: which sparseness is meant, coming
> from sparse image format or from sparse file system?
> 
> For example, if we have qcow2 above raw file on non-sparse file system,
> this function will say nothing about unallocated (by qcow2 layer)
> clusters.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 50ba264143..ba22fc0dfb 100644
> --- a/block.c
> +++ b/block.c
> @@ -3388,8 +3388,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
>  }
>  
>  /**
> - * Length of a allocated file in bytes. Sparse files are counted by actual

Yay for getting rid of bad grammar (s/a /an /)

> - * allocated space. Return < 0 if error or unknown.
> + * Size of allocated in underlying file system area. Sparseness is taken into

Doesn't read well.  Maybe: s/Size of allocated/Allocation size/ ?

> + * account for sparse file systems. Return < 0 if error or unknown.

I still don't get what we are trying to present.

If we have the following 6 qcow2 file clusters backed by the underlying
lseek(SEEK_DATA/HOLE) file system contents:

BDRV_BLOCK_UNALLOCATED   N/A
BDRV_BLOCK_ZERO_PLAIN    N/A
BDRV_BLOCK_ZERO_ALLOC    hole
BDRV_BLOCK_ZERO_ALLOC    data
BDRV_BLOCK_DATA          hole
BDRV_BLOCK_DATA          data

Then is our answer the size of all qcow2 allocations regardless of
underlying status (4, due to clusters 3-6), or the size of only the
clusters that read from the backing file (2, due to clusters 5-6), or
the size of only the clusters that currently occupy space in the file
system (2, due to clusters 4 and 6), or the size of clusters that are
not provably read-as-zero (1, due to cluster 6)?

Does the answer change if you can have underlying holes happen at a
smaller granularity than clusters?

What happens for compressed clusters?

I think we still need to do a better job at writing a precise comment.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 05/25/2017 11:32 AM, Eric Blake wrote:
> If we have the following 6 qcow2 file clusters backed by the underlying
> lseek(SEEK_DATA/HOLE) file system contents:
> 
> BDRV_BLOCK_UNALLOCATED   N/A
> BDRV_BLOCK_ZERO_PLAIN    N/A
> BDRV_BLOCK_ZERO_ALLOC    hole
> BDRV_BLOCK_ZERO_ALLOC    data
> BDRV_BLOCK_DATA          hole
> BDRV_BLOCK_DATA          data

Sorry, these should all be s/BDRV_BLOCK/QCOW2_CLUSTER/ and
s/DATA/NORMAL/ - as I was referring to the qcow2 cluster status given by
qcow2_get_cluster_type() (as of commit fdfab37 or later).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

25.05.2017 19:32, Eric Blake wrote:
> On 05/25/2017 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Current comment is not clear enough: which sparseness is meant, coming
>> from sparse image format or from sparse file system?
>>
>> For example, if we have qcow2 above raw file on non-sparse file system,
>> this function will say nothing about unallocated (by qcow2 layer)
>> clusters.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 50ba264143..ba22fc0dfb 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3388,8 +3388,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
>>   }
>>   
>>   /**
>> - * Length of a allocated file in bytes. Sparse files are counted by actual
> Yay for getting rid of bad grammar (s/a /an /)
>
>> - * allocated space. Return < 0 if error or unknown.
>> + * Size of allocated in underlying file system area. Sparseness is taken into
> Doesn't read well.  Maybe: s/Size of allocated/Allocation size/ ?
>
>> + * account for sparse file systems. Return < 0 if error or unknown.
> I still don't get what we are trying to present.
>
> If we have the following 6 qcow2 file clusters backed by the underlying
> lseek(SEEK_DATA/HOLE) file system contents:
>
> BDRV_BLOCK_UNALLOCATED   N/A
> BDRV_BLOCK_ZERO_PLAIN    N/A
> BDRV_BLOCK_ZERO_ALLOC    hole
> BDRV_BLOCK_ZERO_ALLOC    data
> BDRV_BLOCK_DATA          hole
> BDRV_BLOCK_DATA          data
>
> Then is our answer the size of all qcow2 allocations regardless of
> underlying status (4, due to clusters 3-6), or the size of only the
> clusters that read from the backing file (2, due to clusters 5-6), or
> the size of only the clusters that currently occupy space in the file
> system (2, due to clusters 4 and 6), or the size of clusters that are
> not provably read-as-zero (1, due to cluster 6)?
>
> Does the answer change if you can have underlying holes happen at a
> smaller granularity than clusters?
>
> What happens for compressed clusters?
>
> I think we still need to do a better job at writing a precise comment.
>

bdrv_get_allocated_file_size is not related to qcow2, as qcow2 doesn't 
support it. So, it is finally just raw_get_allocated_file_size, which 
returns st.st_blocks * 512 after fstat(s->fd).

It will correspond to qcow2 sparseness if qcow2 discarded corresponding 
clusters in bs->file and if underlying fs is sparse.



-- 
Best regards,
Vladimir

On 05/25/2017 12:03 PM, Vladimir Sementsov-Ogievskiy wrote:
> 25.05.2017 19:32, Eric Blake wrote:
>> On 05/25/2017 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Current comment is not clear enough: which sparseness is meant, coming
>>> from sparse image format or from sparse file system?
>>>
>>> For example, if we have qcow2 above raw file on non-sparse file system,
>>> this function will say nothing about unallocated (by qcow2 layer)
>>> clusters.
>>>

>>> + * Size of allocated in underlying file system area. Sparseness is
>>> taken into
>> Doesn't read well.  Maybe: s/Size of allocated/Allocation size/ ?
>>
>>> + * account for sparse file systems. Return < 0 if error or unknown.
>> I still don't get what we are trying to present.
>>

>>
>> I think we still need to do a better job at writing a precise comment.
>>
> 
> bdrv_get_allocated_file_size is not related to qcow2, as qcow2 doesn't
> support it. So, it is finally just raw_get_allocated_file_size, which
> returns st.st_blocks * 512 after fstat(s->fd).
> 
> It will correspond to qcow2 sparseness if qcow2 discarded corresponding
> clusters in bs->file and if underlying fs is sparse.

Okay, that helps.  How about this wording:

Allocation size of the underlying file system area.  Sparseness is taken
into account (holes do not contribute to this size).  Return < 0 if
error or unknown.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

25.05.2017 20:46, Eric Blake wrote:
> On 05/25/2017 12:03 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 25.05.2017 19:32, Eric Blake wrote:
>>> On 05/25/2017 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Current comment is not clear enough: which sparseness is meant, coming
>>>> from sparse image format or from sparse file system?
>>>>
>>>> For example, if we have qcow2 above raw file on non-sparse file system,
>>>> this function will say nothing about unallocated (by qcow2 layer)
>>>> clusters.
>>>>
>>>> + * Size of allocated in underlying file system area. Sparseness is
>>>> taken into
>>> Doesn't read well.  Maybe: s/Size of allocated/Allocation size/ ?
>>>
>>>> + * account for sparse file systems. Return < 0 if error or unknown.
>>> I still don't get what we are trying to present.
>>>
>>> I think we still need to do a better job at writing a precise comment.
>>>
>> bdrv_get_allocated_file_size is not related to qcow2, as qcow2 doesn't
>> support it. So, it is finally just raw_get_allocated_file_size, which
>> returns st.st_blocks * 512 after fstat(s->fd).
>>
>> It will correspond to qcow2 sparseness if qcow2 discarded corresponding
>> clusters in bs->file and if underlying fs is sparse.
> Okay, that helps.  How about this wording:
>
> Allocation size of the underlying file system area.  Sparseness is taken
> into account (holes do not contribute to this size).  Return < 0 if
> error or unknown.
>
OK for me, thank you. Will use your wording if no other comments on this.



-- 
Best regards,
Vladimir