[Top][All Lists]

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

Re: [PATCH 04/17] block: Improve documentation of .bdrv_has_zero_init

From: Eric Blake
Subject: Re: [PATCH 04/17] block: Improve documentation of .bdrv_has_zero_init
Date: Tue, 4 Feb 2020 09:16:20 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

On 2/4/20 9:03 AM, Vladimir Sementsov-Ogievskiy wrote:
31.01.2020 20:44, Eric Blake wrote:
Several drivers supply .bdrv_has_zero_init that returns 1, but lack
the .bdrv_has_zero_init_truncate callback (parallels and qed outright,
vdi in some scenarios).  A literal reading of the existing
documentation says such drivers are broken, because
bdrv_has_zero_init_truncate() defaults to zero if the callback is
missing; but in practice, the tie between the two functions is only
relevant when truncate is supported.  Clarify the documentation to
make it obvious that this is okay.

Fixes: 1dcaf527
Signed-off-by: Eric Blake <address@hidden>
  include/block/block_int.h | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 640fb82c789e..77ab45dc87cf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -444,7 +444,8 @@ struct BlockDriver {
       * Returns 1 if newly created images are guaranteed to contain only
       * zeros, 0 otherwise.
-     * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
+     * Must return 0 if .bdrv_co_truncate is set and
+     * .bdrv_has_zero_init_truncate() returns 0.
      int (*bdrv_has_zero_init)(BlockDriverState *bs);

I doubt, shouldn't 1dcaf527 be fixed by adding all needed bdrv_has_zero_init_truncate functions?

That was my original thought. But looking at callers of bdrv_has_zero_init_truncate() shows that they all plan to perform bdrv_co_truncate(PREALLOC_MODE_OFF) and want to know if that will leak non-zero data; if you can't truncate, it doesn't matter what init_truncate() returns, but since init_truncate() defaults to 0, it shouldn't invalidate init() returning 1 - fixing the docs was easier than adding useless callbacks to parallels, qed, and vdi just to rip them back out again in patch 9.

As you noted later, sheepdog was the only driver that violated this rule (and it is fixed in patch 8). I could reorder the series to get the bug fix in before the documentation change, if that would help.

(sorry, I started to dig in the code and patches around bdrv_has_zero_init_truncate and tired :(.,
  hope Max will comment this).

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

reply via email to

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