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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1779 bytes --]

On 13.06.19 15:29, Vladimir Sementsov-Ogievskiy wrote:
> 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.

Well, bdrv_driver_pwritev_compressed() doesn’t really care, it will find
out sooner or later anyway (while being passed down the chain).  This is
only really important for the backup job, which will use this function
as of patch 26.  (It isn’t important before 26, because using filters
with backup generally is a gamble before that patch.)

> (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.

We probably could, yes.  I just felt like this wasn’t the time to do it.
O:-)

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

Thanks for reviewing!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]