[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: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 09/17] block: Refactor bdrv_has_zero_init{,_truncate}
Date: Wed, 5 Feb 2020 10:51:01 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

04.02.2020 20:42, Max Reitz wrote:
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
     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
file resizing? I agree that it's normal for these flags to have the same
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.

This means, that if truncation bit is set, than create bit should be set.. But
here we say that if truncation is clear, than create bit must be clear.

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.

ah yes, due to qcow2 zero clusters.

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.


Best regards,

reply via email to

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