Re: [PATCH v4 20/30] qcow2: Add subcluster support to discard_in_l2_slice()

[Vladimir Sementsov-Ogievskiy]

22.04.2020 20:42, Alberto Garcia wrote:
> On Wed 22 Apr 2020 01:35:25 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> > 17.03.2020 21:16, Alberto Garcia wrote:
> > > Two changes are needed in this function:
> > >
> > > 1) A full discard deallocates a cluster so we can skip the operation if
> > >      it is already unallocated. With extended L2 entries however if any
> > >      of the subclusters has the 'all zeroes' bit set then we have to
> > >      clear it.
> > >
> > > 2) Setting the QCOW_OFLAG_ZERO bit of the L2 entry is forbidden if an
> > >      image has extended L2 entries. Instead, the individual 'all zeroes'
> > >      bits must be used.
> > >
> > > Signed-off-by: Alberto Garcia <address@hidden>
> > > ---
> > >    block/qcow2-cluster.c | 18 +++++++++++++++---
> > >    1 file changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > index 746006a117..824c710760 100644
> > > --- a/block/qcow2-cluster.c
> > > +++ b/block/qcow2-cluster.c
> > > @@ -1790,12 +1790,20 @@ static int discard_in_l2_slice(BlockDriverState *bs, 
> > > uint64_t offset,
> > >             * TODO We might want to use bdrv_block_status(bs) here, but we're
> > >             * holding s->lock, so that doesn't work today.
> > >             *
> > > -         * If full_discard is true, the sector should not read back as zeroes,
> > > +         * If full_discard is true, the cluster should not read back as zeroes,
> > >             * but rather fall through to the backing file.
> > >             */
> > >            switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
> > >            case QCOW2_CLUSTER_UNALLOCATED:
> > > -            if (full_discard || !bs->backing) {
> > > +            if (full_discard) {
> > > +                /* If the image has extended L2 entries we can only
> > > +                 * skip this operation if the L2 bitmap is zero. */
> > > +                uint64_t bitmap = has_subclusters(s) ?
> > > +                    get_l2_bitmap(s, l2_slice, l2_index + i) : 0;
> > > +                if (bitmap == 0) {
> > > +                    continue;
> > > +                }
> > > +            } else if (!bs->backing) {
> > >                    continue;
> > >                }
> >
> > Hmm, so you do continue if full_discard is false AND bitmap != 0 &
> > !bs->backing,
>
> > but you do not continue if full_discard is true AND bitmap != 0 &
> > !bs->backing (as you will not go to "else") branch.
>
> 1. If full_discard is true it means that the entry and the bitmap should
>     always be set to 0, regardless of whether there's a backing file or
>     any other consideration.
>
>     This is used e.g when shrinking an image, or by qcow2_make_empty().
>
>     We can only skip this operation if both the entry and the bitmap are
>     already 0 (the former we know because of QCOW2_CLUSTER_UNALLOCATED).

Ah, understand, sorry. I thought that behavior was changed accidentally, but it 
is for purpose. With old code cluster is already unallocated, but with 
subclusters we may have some ZERO_PLAIN subclusters.

> 2. If full_discard is false it means that we must ensure that the
>     cluster reads back as zeroes, but there's no need to clear the bitmap
>     (in fact we must set QCOW_OFLAG_ZERO or QCOW_L2_BITMAP_ALL_ZEROES
>     depending on the type of image).
>
>     We can skip this operation if there's no backing file and the cluster
>     is already unallocated (because then we know that it already reads as
>     zeroes).
>
>     One optimization would be to skip the operation also if the image has
>     subclusters and the bitmap is QCOW_L2_BITMAP_ALL_ZEROES, I can do
>     that for the next version.
>
> > In case QCOW2_CLUSTER_ZERO_PLAIN, worth assert !has_subclusters(s) or
> > mark image corrupted ?
>
> I think that should be handled directly in qcow2_get_cluster_type().
>
> There's currently an inconsistency now that I think of it: if an image
> has subclusters and QCOW_OFLAG_ZERO set then qcow2_get_cluster_type()
> returns QCOW2_CLUSTER_ZERO_* but qcow2_get_subcluster_type() returns
> QCOW2_SUBCLUSTER_INVALID.
>
> Two alternatives:
>
>    - We add QCOW2_CLUSTER_INVALID so we get an error in both
>      cases. Problem: any function that calls qcow2_get_cluster_type()
>      should be modified to handle that.
>
>    - We ignore QCOW_OFLAG_ZERO. Simpler, and it would allow us to use
>      that bit in the future if we wanted.

Hmm. Actually we don't check other reserved bits. But ZERO bit is risky, we may 
miss data corruptions during transmission to the qcow2-subclusters world. So 
I'm for the first variant if it's not too huge.




--
Best regards,
Vladimir