Re: [Qemu-devel] [PATCH] block: push recursive flushing up from drivers

[Kevin Wolf]

Am 12.03.2012 17:01, schrieb Paolo Bonzini:
> In most cases, bdrv_co_flush_to_disk just needs to flush the underlying
> file for protocols.  Do this implicitly in the block layer.
> 
> The backing file is also flushed, because it may still be open read-write
> in the case of live snapshots.

Is this an independent change? I'm also not convinced that it's the
right thing to do because even though it is still opened read-write, we
don't write to it any more. Once bdrv_reopen() is ready, we'll want to
change it to read-only after taking the snapshot.

> @@ -3516,10 +3514,13 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>  {
>      int ret;
>  
> -    if (!bs->drv) {
> +    if (!bs || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
>          return 0;
>      }
>  
> +    bdrv_co_flush(bs->file);
> +    bdrv_co_flush(bs->backing_hd);

Error handling is missing here.

> +
>      /* Write back cached data to the OS even with cache=unsafe */
>      if (bs->drv->bdrv_co_flush_to_os) {
>          ret = bs->drv->bdrv_co_flush_to_os(bs);

Now you first flush bs->file and then write out the internal caches.
This doesn't look quite right. Probably the recursion should be at the
very end of the function.

Kevin