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

[Vladimir Sementsov-Ogievskiy]

04.02.2020 18:49, Eric Blake wrote:
> 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).

This is OK, of course, I'm just trying to understand existing logic.

> 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?

If we want to guarantee the same behavior, we should restrict any difference 
between these flags :)

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

yes, it reported even for encrypted images truncate=1...

> > > +     * 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;
> > > +
> >
> > ...


--
Best regards,
Vladimir