Re: [Qemu-devel] [PATCH 3/3] replay: introduce block devices record/replay

[Kevin Wolf]

Am 29.02.2016 um 08:03 hat Pavel Dovgalyuk geschrieben:
> > From: Kevin Wolf [mailto:address@hidden
> > 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