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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <berto@igalia.com>
>>> ---
>>>    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