Re: [PATCH 3/4] block/copy-before-write: reverse access bitmap

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

On 5/20/25 19:26, Eric Blake wrote:
> [?? ??????? ????????? ?????? ?? eblake@redhat.com. ???????, ?????? ??? ?????, ?? ?????? https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Tue, May 13, 2025 at 03:32:37AM +0200, Andrey Zhadchenko wrote:
>> HBitmaps allow us to search set bits pretty fast. On the contrary,
>> when searching zeroes, we may be forced to fully traverse the lower
>> level.
>> When we run blockdev-backup with mode=full on top of snapshot filter
>> + cbw filter, the job fills copy bitmap by calling block_status()
>> with range (X, virtual_size). The problem is that we check for zeroes
>> in this whole range. We also hit the worst case here, as access
>> bitmap is fully set and we need to scan the entire lowest level.
>> After scanning the full bitmap we actually ask the block status of
>> original image, which may return significantly lower amount of empty
>> clusters.
>> Beacuse of this, the backup job 'hangs' on block copy initializaiton
>> for a long time with 100% CPU.
>>
>> Example copy bitmap buildup time for image with clu_size=65536 and
>> preallocated metadata
>> size                 10T   11T
>> blockdev-backup      52s   57s
>> cbw + snap           325s  413s
>> cbw + snap + patch   55s   61s
>>
>> To fix it, reverse the access bitmap in cbw filter: rather set it
> 
> s/reverse/invert/
> 
>> when the user is not allowed to read the cluster.
>>
>> Update qemu-iotest 257: now access bitmap have count 0 instead of
>> the image size 67108864
>>
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>> ---
>> @@ -501,9 +501,12 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
>>           return -EINVAL;
>>       }
>>       bdrv_disable_dirty_bitmap(s->access_bitmap);
>> -    bdrv_dirty_bitmap_merge_internal(s->access_bitmap,
>> -                                     block_copy_dirty_bitmap(s->bcs), NULL,
>> -                                     true);
>> +    if (bitmap) {
>> +        bdrv_dirty_bitmap_merge_internal(s->access_bitmap,
>> +                                         block_copy_dirty_bitmap(s->bcs), NULL,
>> +                                         true);
>> +        bdrv_dirty_bitmap_reverse(s->access_bitmap);
> 
> Is this setting the bits correctly?  Inverting a bitmap is a
> reversible operation, but it looks odd that you are only reversing
> once around the merge.  Either the two sources of the merge have the
> same sense (whether that be 0 for dirty 1 for clean, or 0 for readable
> 1 for avoid) and no inverting is needed before or after the merge, or
> the two sources have opposite sense (in which case, I would have
> expected inverting one of the bitmaps before the merge to get them to
> agree on sense, then merging, then inverting back to the desired
> sense).  Am I missing something?

We have just created access bitmap a few lines above. So it is empty: 
everything is accessible.
  - If cbw did not have any bitmap, we don't need to to do anything
  - If someone set bitmap on cbw, we used it to init block-copy dirty 
bitmap. So to get the access bitmap, we inverse the bitmap that marks
to-be-copied blocks: access bitmap now has all of them as 0 (allowed) 
and not-to-be-copied as 1 (disallowed)

This case is also covered with iotests/image-fleecing with filter 
snapshot + cbw bitmap case: some not-in-a-bitmap blocks are accessed and 
we expect to get errors:

--- Sanity Check ---

read -P0x5d 0 64k
read -P0xd5 1M 64k
read -P0xdc 32M 64k
read -P0xcd 0x3ff0000 64k
read -P0 0x00f8000 32k
read failed: Invalid argument

read -P0 0x2010000 32k
read failed: Invalid argument

read -P0 0x3fe0000 64k
read failed: Invalid argument


> 
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization:  qemu.org | libguestfs.org
>