Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Kevin Wolf]

Am 28.06.2016 um 14:32 hat Pavel Butsykin geschrieben:
> On 28.06.2016 14:47, Kevin Wolf wrote:
> >Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
> >>From: Pavel Butsykin <address@hidden>
> >>
> >>For bdrv_pwrite_compressed() it looks like most of the code creating 
> >>coroutine
> >>is duplicated in blk_prw(). So we can just add a 
> >>flag(BDRV_REQ_WRITE_COMPRESSED)
> >>and use the blk_prw() as a generic one.
> >>
> >>Signed-off-by: Pavel Butsykin <address@hidden>
> >>Signed-off-by: Denis V. Lunev <address@hidden>
> >>CC: Jeff Cody <address@hidden>
> >>CC: Markus Armbruster <address@hidden>
> >>CC: Eric Blake <address@hidden>
> >>CC: John Snow <address@hidden>
> >>CC: Stefan Hajnoczi <address@hidden>
> >>CC: Kevin Wolf <address@hidden>
> >
> >Oh, so you already do use a flag. Nice. :-)
> >
> >>diff --git a/block/block-backend.c b/block/block-backend.c
> >>index 3c1fc50..9e1c793 100644
> >>--- a/block/block-backend.c
> >>+++ b/block/block-backend.c
> >>@@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, 
> >>int64_t offset,
> >>          flags |= BDRV_REQ_FUA;
> >>      }
> >>
> >>-    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> >>+    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> >>+        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, 
> >>qiov);
> >>+    } else {
> >>+        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> >>+    }
> >>  }
> >
> >If you move the processing of the flag inside bdrv_co_pwritev(), where I
> >think it belongs anyway, you could use the flag from the start (by going
> >through bdrv_prwv_co()) instead of temporarily introducing your own
> >coroutine wrapper. I think that would make the initial conversion
> >patches quite a bit simpler.
> 
> You propose to add bdrv_driver_compressed and call it from
> bdrv_aligned_pwritev ?

I'm not sure if we can easily move it that much down the caller chain.
If we can, great. But here I was just thinking of making the switch at
the start of bdrv_co_pwritev() so that you can reuse the existing
wrappers like bdrv_prwv_co().

The long-term advantage of putting it into bdrv_aligned_pwritev() could
be that we would ge the common alignment handling. But I think qcow2
still errors out if you rewrite an already allocated cluster with
qcow2_pwritev_compressed(), so currently doing sub-cluster (or even
sub-sector) writes doesn't make much sense anyway. So I doubt it's worth
the hassle now.

Kevin