Re: [PATCH 09/17] block: Refactor bdrv_has_zero_init{,_truncate}

[Eric Blake <eblake@redhat.com>]

On 2/4/20 9:35 AM, Vladimir Sementsov-Ogievskiy wrote:
> 31.01.2020 20:44, Eric Blake wrote:
>> Having two slightly-different function names for related purposes is
>> unwieldy, especially since I envision adding yet another notion of
>> zero support in an upcoming patch.  It doesn't help that
>> bdrv_has_zero_init() is a misleading name (I originally thought that a
>> driver could only return 1 when opening an already-existing image
>> known to be all zeroes; but in reality many drivers always return 1
>> because it only applies to a just-created image).  Refactor all uses
>> to instead have a single function that returns multiple bits of
>> information, with better naming and documentation.
> 
> Sounds good
> 
>>
>> No semantic change, although some of the changes (such as to qcow2.c)
>> require a careful reading to see how it remains the same.
>>
> 
> ...
> 
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 6cd566324d95..a6a227f50678 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
> 
> Hmm, header file in the middle of the patch, possibly you don't use
> [diff]
>      orderFile = scripts/git.orderfile
> 
> in git config.. Or it is broken.

I do have it set up, so I'm not sure why it didn't work as planned. 
I'll make sure v2 follows the order I intended.

> 
>> @@ -85,6 +85,28 @@ typedef enum {
>>       BDRV_REQ_MASK               = 0x3ff,
>>   } BdrvRequestFlags;
>>
>> +typedef enum {
>> +    /*
>> +     * bdrv_known_zeroes() should include this bit if the contents of
>> +     * a freshly-created image with no backing file reads as all
>> +     * zeroes without any additional effort.  If .bdrv_co_truncate is
>> +     * set, then this must be clear if BDRV_ZERO_TRUNCATE is clear.
> 
> I understand that this is preexisting logic, but could I ask: why? 
> What's wrong
> if driver can guarantee that created file is all-zero, but is not sure 
> about
> file resizing? I agree that it's normal for these flags to have the same 
> value,
> but what is the reason for this restriction?..

For _this_ patch, my goal is to preserve pre-existing practice. Where we 
think pre-existing practice is wrong, we can then improve it in other 
patches (see patch 6, for example).

I _think_ the reason for this original limitation is as follows: If an 
image can be resized, we could choose to perform 'create(size=0), 
truncate(size=final)' instead of 'create(size=final)', and we want to 
guarantee the same behavior. If truncation can't guarantee a zero read, 
then why is creation doing so?

But as I did not write the original patch, I would welcome Max's input 
with regards to the thought behind commit ceaca56f.

> 
> So, the only possible combination of flags, when they differs, is 
> create=0 and
> truncate=1.. How is it possible?

qcow2 had that mode, at least before patch 5.

> 
>> +     * Since this bit is only reliable at image creation, a driver may
>> +     * return this bit even for existing images that do not currently
>> +     * read as zero.
>> +     */
>> +    BDRV_ZERO_CREATE        = 0x1,
>> +
>> +    /*
>> +     * bdrv_known_zeroes() should include this bit if growing an image
>> +     * with PREALLOC_MODE_OFF (either with no backing file, or beyond
>> +     * the size of the backing file) will read the new data as all
>> +     * zeroes without any additional effort.  This bit only matters
>> +     * for drivers that set .bdrv_co_truncate.
>> +     */
>> +    BDRV_ZERO_TRUNCATE      = 0x2,
>> +} BdrvZeroFlags;
>> +
> 
> ...
> 
> 

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