Re: [PATCH 1/3] block: add BDRV_BLOCK_COMPRESSED flag for bdrv_block_status()

[Andrey Drobyshev]

On 6/21/23 20:46, Denis V. Lunev wrote:
> On 6/21/23 19:08, Denis V. Lunev wrote:
>> On 6/7/23 17:26, Andrey Drobyshev wrote:
>>> Functions qcow2_get_host_offset(), get_cluster_offset() explicitly
>>> report compressed cluster types when data is compressed. However, this
>>> information is never passed further.  Let's make use of it by adding new
>>> BDRV_BLOCK_COMPRESSED flag for bdrv_block_status(), so that caller may
>>> know that the data range is compressed.  In particular, we're going to
>>> use this flag to tweak "qemu-img map" output.
>>>
>>> This new flag is only being utilized by qcow and qcow2 formats, as only
>>> these two support compression.
>>>
>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>> ---
>>>   block/qcow.c                 | 5 ++++-
>>>   block/qcow2.c                | 3 +++
>>>   include/block/block-common.h | 3 +++
>>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/qcow.c b/block/qcow.c
>>> index 3644bbf5cb..8416bcc2c3 100644
>>> --- a/block/qcow.c
>>> +++ b/block/qcow.c
>>> @@ -549,7 +549,10 @@ qcow_co_block_status(BlockDriverState *bs, bool
>>> want_zero,
>>>       if (!cluster_offset) {
>>>           return 0;
>>>       }
>>> -    if ((cluster_offset & QCOW_OFLAG_COMPRESSED) || s->crypto) {
>>> +    if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
>>> +        return BDRV_BLOCK_DATA | BDRV_BLOCK_COMPRESSED;
>>> +    }
>>> +    if (s->crypto) {
>>>           return BDRV_BLOCK_DATA;
>>>       }
>>>       *map = cluster_offset | index_in_cluster;
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index e23edd48c2..8e01adc610 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2162,6 +2162,9 @@ qcow2_co_block_status(BlockDriverState *bs,
>>> bool want_zero, int64_t offset,
>>>       {
>>>           status |= BDRV_BLOCK_RECURSE;
>>>       }
>>> +    if (type == QCOW2_SUBCLUSTER_COMPRESSED) {
>>> +        status |= BDRV_BLOCK_COMPRESSED;
>>> +    }
>>>       return status;
>>>   }
>>>   diff --git a/include/block/block-common.h
>>> b/include/block/block-common.h
>>> index e15395f2cb..f7a4e7d4db 100644
>>> --- a/include/block/block-common.h
>>> +++ b/include/block/block-common.h
>>> @@ -282,6 +282,8 @@ typedef enum {
>>>    *                       layer rather than any backing, set by
>>> block layer
>>>    * BDRV_BLOCK_EOF: the returned pnum covers through end of file for
>>> this
>>>    *                 layer, set by block layer
>>> + * BDRV_BLOCK_COMPRESSED: the underlying data is compressed; only
>>> valid for
>>> + *                        the formats supporting compression: qcow,
>>> qcow2
>>>    *
>>>    * Internal flags:
>>>    * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to
>>> request
>>> @@ -317,6 +319,7 @@ typedef enum {
>>>   #define BDRV_BLOCK_ALLOCATED    0x10
>>>   #define BDRV_BLOCK_EOF          0x20
>>>   #define BDRV_BLOCK_RECURSE      0x40
>>> +#define BDRV_BLOCK_COMPRESSED   0x80
>>>     typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry)
>>> BlockReopenQueue;
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
> Looking into the second patch I have found that I was a too fast here :)
> 
> The comment is misleading and the patch is incomplete.
> 
> static inline bool TSA_NO_TSA block_driver_can_compress(BlockDriver *drv)
> {
>     return drv->bdrv_co_pwritev_compressed ||
>            drv->bdrv_co_pwritev_compressed_part;
> }
> 
> which means that
> 
>    1    257  block/copy-on-read.c <<GLOBAL>>
>              .bdrv_co_pwritev_compressed = cor_co_pwritev_compressed,
>    2   1199  block/qcow.c <<GLOBAL>>
>              .bdrv_co_pwritev_compressed = qcow_co_pwritev_compressed,
>    3    255  block/throttle.c <<GLOBAL>>
>              .bdrv_co_pwritev_compressed = throttle_co_pwritev_compressed,
>    4   3108  block/vmdk.c <<GLOBAL>>
>              .bdrv_co_pwritev_compressed = vmdk_co_pwritev_compressed,
>   1   6121  block/qcow2.c <<GLOBAL>>
> .bdrv_co_pwritev_compressed_part...cow2_co_pwritev_compressed_part,
> 
> We have missed at least VMDK images.

Thanks, my bad I didn't check that more thoroughly, I was mainly looking
in the docs when making this conclusion.

man qemu-img:
> Only the formats qcow and qcow2 support  compression.  The  com‐
> pression  is  read-only. It means that if a compressed sector is
> rewritten, then it is rewritten as uncompressed data.

Apparently man pages got a bit outdated on this.