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

[Vladimir Sementsov-Ogievskiy]

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
> > [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.

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.
>
> Max


--
Best regards,
Vladimir