[Top][All Lists]

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

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

From: Max Reitz
Subject: Re: [PATCH 09/17] block: Refactor bdrv_has_zero_init{,_truncate}
Date: Tue, 4 Feb 2020 18:42:27 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

On 04.02.20 16:35, 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.
>> @@ -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?..

If areas added by truncation (or growth, rather) are always zero, then
the file can always be created with size 0 and grown from there.  Thus,
images where truncation adds zeroed areas will generally always be zero
after creation.

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

For preallocated qcow2 images, it depends on the storage whether they
are actually 0 after creation.  Hence qcow2_has_zero_init() then defers
to bdrv_has_zero_init() of s->data_file->bs.

But when you truncate them (with PREALLOC_MODE_OFF, as
BlockDriver.bdrv_has_zero_init_truncate()’s comment explains), the new
area is always going to be 0, regardless of initial preallocation.

I just noticed a bug there, though: Encrypted qcow2 images will not see
areas added through growth as 0.  Hence, qcow2’s
bdrv_has_zero_init_truncate() implementation should not return true
unconditionally, but only for unencrypted images.


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]