From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41713) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UcFVo-00055E-WC for qemu-devel@nongnu.org; Tue, 14 May 2013 09:43:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UcFVj-0008J1-FQ for qemu-devel@nongnu.org; Tue, 14 May 2013 09:43:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56936) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UcFVj-0008Is-6W for qemu-devel@nongnu.org; Tue, 14 May 2013 09:43:23 -0400 Date: Tue, 14 May 2013 15:43:00 +0200 From: Kevin Wolf Message-ID: <20130514134300.GC2556@dhcp-200-207.str.redhat.com> References: <1367221335-22777-1-git-send-email-stefanha@redhat.com> <1367221335-22777-2-git-send-email-stefanha@redhat.com> <20130508123925.GG3093@dhcp-200-207.str.redhat.com> <20130514132459.GA21771@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130514132459.GA21771@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 1/3] block: add basic backup support to block driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Fam Zheng , qemu-devel@nongnu.org, dietmar@proxmox.com, imain@redhat.com, Stefan Hajnoczi , pbonzini@redhat.com, Wenchao Xia Am 14.05.2013 um 15:24 hat Stefan Hajnoczi geschrieben: > On Wed, May 08, 2013 at 02:39:25PM +0200, Kevin Wolf wrote: > > Am 29.04.2013 um 09:42 hat Stefan Hajnoczi geschrieben: > > > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > > > index c290d07..6f42495 100644 > > > --- a/include/block/blockjob.h > > > +++ b/include/block/blockjob.h > > > @@ -50,6 +50,13 @@ typedef struct BlockJobType { > > > * manually. > > > */ > > > void (*complete)(BlockJob *job, Error **errp); > > > + > > > + /** tracked requests */ > > > + int coroutine_fn (*before_read)(BlockDriverState *bs, int64_t sector_num, > > > + int nb_sectors, QEMUIOVector *qiov); > > > + int coroutine_fn (*before_write)(BlockDriverState *bs, int64_t sector_num, > > > + int nb_sectors, QEMUIOVector *qiov); > > > + > > > } BlockJobType; > > > > This is actually a sign that a block job isn't the right tool. Jobs are > > something that runs in the background and doesn't have callbacks. You > > really want to have a filter here (that happens to be coupled to a job). > > Need the BlockBackend split before we can do this right. > > > > The second thing that this conflicts with is generalising block jobs to > > generic background jobs. > > > > Each hack like this that we accumulate makes it harder to get the real > > thing eventually. > > In v3 I added a slightly cleaner interface: > > bdrv_set_before_write_cb(bs, backup_before_write); > > This way the "before write" callback is not tied to block jobs and > could be reused in the future. > > The alternative is to create a BlockDriver that mostly delegates plus > uses bdrv_swap(). The boilerplate involved in that is ugly though, so > I'm using bdrv_set_before_write_cb() instead. The clean implementation of filters is putting a BlockDriver on top, it gives you flexibility to intercept anything and you can stack multiple of them instead of having just one callback per BDS. But I'm not sure if simply bdrv_swap is good enough. You would definitely have to disable snapshots while the filter is active and there may be more cases. This is the part that I think getting right is a bit more complex and might need the BlockBackend split. So you would vote to introduce bdrv_set_before_write_cb() now and later change how everything works? Kevin