Re: [PATCH v7 10/47] mirror-top: Support compressed writes

[Kevin Wolf]

Am 19.08.2020 um 17:35 hat Max Reitz geschrieben:
> 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.

This patch looks like a feature addition that is only marginally related
to the goal of the series anyway. Maybe it should be a separate small
series on top?

The other compression related patches in the series don't seem to have
this problem, so they could stay there anyway.

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

Yeah, it's just a bit unfortunate to add a new function that we know
doesn't do what it promises.

Kevin

signature.asc
Description: PGP signature