From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55418) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaIfQ-0005D3-A8 for qemu-devel@nongnu.org; Mon, 29 Feb 2016 02:54:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaIfN-0001R2-1Y for qemu-devel@nongnu.org; Mon, 29 Feb 2016 02:54:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43832) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaIfM-0001Qs-QI for qemu-devel@nongnu.org; Mon, 29 Feb 2016 02:54:52 -0500 Date: Mon, 29 Feb 2016 08:54:47 +0100 From: Kevin Wolf Message-ID: <20160229075447.GA5004@noname.redhat.com> References: <20160216100208.GA4920@noname.str.redhat.com> <000a01d168ac$09929500$1cb7bf00$@ru> <20160216125453.GC4920@noname.str.redhat.com> <000601d16bad$e93f9eb0$bbbedc10$@ru> <20160222110644.GD5387@noname.str.redhat.com> <002101d16efa$c74b3850$55e1a8f0$@ru> <20160224131407.GF4485@noname.redhat.com> <000c01d16fab$d85e3b40$891ab1c0$@ru> <20160226090108.GB5668@noname.redhat.com> <001201d172bf$3aa580e0$aff082a0$@ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <001201d172bf$3aa580e0$aff082a0$@ru> Subject: Re: [Qemu-devel] [PATCH 3/3] replay: introduce block devices record/replay List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgalyuk Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org, igor.rubinov@gmail.com, mark.burton@greensocs.com, real@ispras.ru, hines@cert.org, qemu-devel@nongnu.org, maria.klimushenkova@ispras.ru, stefanha@redhat.com, pbonzini@redhat.com, batuzovk@ispras.ru, alex.bennee@linaro.org, fred.konrad@greensocs.com Am 29.02.2016 um 08:03 hat Pavel Dovgalyuk geschrieben: > > From: Kevin Wolf [mailto:kwolf@redhat.com] > > Am 25.02.2016 um 10:06 hat Pavel Dovgalyuk geschrieben: > > > There is one problem with flush event - callbacks for flush are called for > > > all layers and I couldn't synchronize them correctly yet. > > > I'll probably have to add new callback to block driver, which handles > > > flush request for the whole stack of the drivers. > > > > Flushes should be treated more or less the same a writes, I think. > > But bdrv_co_flush has different structure and does not allow synchronization > of the top layer. Here is the patch for fixing this: > > diff --git a/block/io.c b/block/io.c > index a69bfc4..9e05dfe 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2369,6 +2369,12 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs) > } > > tracked_request_begin(&req, bs, 0, 0, BDRV_TRACKED_FLUSH); > + /* Write back all layers by calling one driver function */ > + if (bs->drv->bdrv_co_flush) { > + ret = bs->drv->bdrv_co_flush(bs); > + goto out; > + } > + > /* Write back cached data to the OS even with cache=unsafe */ > BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_OS); > if (bs->drv->bdrv_co_flush_to_os) { > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 9ef823a..9cc2c58 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -176,6 +176,13 @@ struct BlockDriver { > int (*bdrv_inactivate)(BlockDriverState *bs); > > /* > + * Flushes all data for all layers by calling bdrv_co_flush for underlying > + * layers, if needed. This function is needed for deterministic > + * synchronization of the flush finishing callback. > + */ > + int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs); > + > + /* > * Flushes all data that was already written to the OS all the way down to > * the disk (for example raw-posix calls fsync()). > */ > > > Then I'll have just to implement bdrv_co_flush for blkreplay layer. > This callback will manually invoke bdrv_co_flush for underlying layer > allowing synchronization of finishing callback. The real problem is that the lower layer (bs->file) is automatically flushed, introducing non-determinism outside of the protecting blkreplay driver, correct? Hm... My first thought was maybe introduce a bool BlockDriver.no_flush_parent which avoids the automatic flush for the parent node. Then you could use .bdrv_co_flush_to_os() like all other drivers do. But you're right that your callback wouldn't just flush to the OS, but you would have to implement the flush on the lower layer and the handling of BDRV_O_NO_FLUSH in the blkreplay driver, so introducing a new callback for a "whole" flush might indeed be cleaner. I don't particularly like it, but I don't see a better option, so it's okay with me. Kevin