From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHsha-0006wt-RP for qemu-devel@nongnu.org; Tue, 28 Jun 2016 09:05:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHshV-0002kH-KB for qemu-devel@nongnu.org; Tue, 28 Jun 2016 09:05:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46778) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHshV-0002k1-Cv for qemu-devel@nongnu.org; Tue, 28 Jun 2016 09:05:13 -0400 Date: Tue, 28 Jun 2016 15:05:10 +0200 From: Kevin Wolf Message-ID: <20160628130510.GG6800@noname.redhat.com> References: <1464686130-12265-1-git-send-email-den@openvz.org> <1464686130-12265-8-git-send-email-den@openvz.org> <20160628114707.GE6800@noname.redhat.com> <57726E4D.4080501@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57726E4D.4080501@virtuozzo.com> Subject: Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Butsykin Cc: "Denis V. Lunev" , qemu-devel@nongnu.org, Jeff Cody , Markus Armbruster , Eric Blake , John Snow , Stefan Hajnoczi 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 > >> > >>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 > >>Signed-off-by: Denis V. Lunev > >>CC: Jeff Cody > >>CC: Markus Armbruster > >>CC: Eric Blake > >>CC: John Snow > >>CC: Stefan Hajnoczi > >>CC: Kevin Wolf > > > >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