Re: [Qemu-devel] [PATCH v5 11/42] block: Add bdrv_supports_compressed_writes()

[Vladimir Sementsov-Ogievskiy]

13.06.2019 1:09, Max Reitz wrote:
> Filters cannot compress data themselves but they have to implement
> .bdrv_co_pwritev_compressed() still (or they cannot forward compressed
> writes).  Therefore, checking whether
> bs->drv->bdrv_co_pwritev_compressed is non-NULL is not sufficient to
> know whether the node can actually handle compressed writes.  This
> function looks down the filter chain to see whether there is a
> non-filter that can actually convert the compressed writes into
> compressed data (and thus normal writes).

Why not to use this function in (as I remember only 2-3 cases) when
we check for bs->drv->bdrv_co_pwritev_compressed? It would be a complete fix
for described problem.
(hmm, ok, other new APIs are added separately too, for some reason they don't
confuse me and this confuses)

On the other hand, (the second time I think about it during review), could
we handle compression through flags completely?
We have supported_write_flags feature, which should be used for all these 
checks..
And may be, we may drop .bdrv_co_pwritev_compressed at all.

But if you want to keep it as is, it's OK too:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   include/block/block.h |  1 +
>   block.c               | 22 ++++++++++++++++++++++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 687c03b275..7835c5b370 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -487,6 +487,7 @@ void bdrv_next_cleanup(BdrvNextIterator *it);
>   
>   BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
>   bool bdrv_is_encrypted(BlockDriverState *bs);
> +bool bdrv_supports_compressed_writes(BlockDriverState *bs);
>   void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>                            void *opaque, bool read_only);
>   const char *bdrv_get_node_name(const BlockDriverState *bs);
> diff --git a/block.c b/block.c
> index 567a0f82c8..97774b7b06 100644
> --- a/block.c
> +++ b/block.c
> @@ -4584,6 +4584,28 @@ bool bdrv_is_encrypted(BlockDriverState *bs)
>       return false;
>   }
>   
> +/**
> + * Return whether the given node supports compressed writes.
> + */
> +bool bdrv_supports_compressed_writes(BlockDriverState *bs)
> +{
> +    BlockDriverState *filtered = bdrv_filtered_rw_bs(bs);
> +
> +    if (!bs->drv || !bs->drv->bdrv_co_pwritev_compressed) {
> +        return false;
> +    }
> +
> +    if (filtered) {
> +        /*
> +         * Filters can only forward compressed writes, so we have to
> +         * check the child.
> +         */
> +        return bdrv_supports_compressed_writes(filtered);
> +    }
> +
> +    return true;
> +}
> +
>   const char *bdrv_get_format_name(BlockDriverState *bs)
>   {
>       return bs->drv ? bs->drv->format_name : NULL;
> 


-- 
Best regards,
Vladimir