From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:44764)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1S77y4-0005CB-RE
	for qemu-devel@nongnu.org; Mon, 12 Mar 2012 12:19:34 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1S77xg-0007RM-Ds
	for qemu-devel@nongnu.org; Mon, 12 Mar 2012 12:19:28 -0400
Received: from mx1.redhat.com ([209.132.183.28]:20646)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1S77xg-0007Pg-5v
	for qemu-devel@nongnu.org; Mon, 12 Mar 2012 12:19:04 -0400
Message-ID: <4F5E22C9.1000701@redhat.com>
Date: Mon, 12 Mar 2012 17:22:33 +0100
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
References: <1331568108-13188-1-git-send-email-pbonzini@redhat.com>
In-Reply-To: <1331568108-13188-1-git-send-email-pbonzini@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] block: push recursive flushing up from
	drivers
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com

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