On 18.08.20 12:27, Kevin Wolf wrote:
> Am 25.06.2020 um 17:21 hat Max Reitz geschrieben:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/mirror.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index e8e8844afc..469acf4600 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -1480,6 +1480,15 @@ static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
>>                                      NULL, 0);
>>  }
>>  
>> +static int coroutine_fn bdrv_mirror_top_pwritev_compressed(BlockDriverState *bs,
>> +                                                           uint64_t offset,
>> +                                                           uint64_t bytes,
>> +                                                           QEMUIOVector *qiov)
>> +{
>> +    return bdrv_mirror_top_pwritev(bs, offset, bytes, qiov,
>> +                                   BDRV_REQ_WRITE_COMPRESSED);
>> +}
> 
> Hm, not sure if it's a problem, but bdrv_supports_compressed_writes()
> will now return true for mirror-top. However, with an active mirror to a
> target that doesn't support compression, trying to actually do a
> compressed write will always return -ENOTSUP.

Right.

> So I guess the set of nodes patch 7 looks at still isn't quite complete.
> However, it's not obvious how to make it more complete without
> delegating to the driver.
> 
> Maybe we need to use bs->supported_write_flags, which is set by the
> driver, instead of looking at the presence of callbacks.

Hm, yes, that would work better.  Not sure if it’s worth it for this series.

The only problem we’d have is late failure when trying to do a
compressed backup to a target that’s running an active mirror.  (Late as
in “first write fails without explanation”, as opposed to “job fails
during set-up”.)

Which I hope is not a case anyone would ever encounter, and even if they
do, the failure doesn’t seem catastrophic to me.  So I don’t think it’s
really a problem.

Max