qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Kevin Wolf <kwolf@redhat.com>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Charlie Shepherd <charlie@ctshepherd.com>,
	gabriel@kerneis.info, qemu-devel@nongnu.org, pbonzini@redhat.com
Subject: Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers
Date: Fri, 19 Jul 2013 10:37:11 +0200	[thread overview]
Message-ID: <20130719083711.GC2992@dhcp-200-207.str.redhat.com> (raw)
In-Reply-To: <20130719052744.GF14330@stefanha-thinkpad.redhat.com>

Am 19.07.2013 um 07:27 hat Stefan Hajnoczi geschrieben:
> On Thu, Jul 18, 2013 at 11:21:42PM +0200, Charlie Shepherd wrote:
> > This patch makes bdrv_flush a synchronous function and updates any callers from
> > a coroutine context to use bdrv_co_flush instead.
> > 
> > The motivation for this patch comes from the GSoC Continuation-Passing C
> > project. When coroutines were introduced, synchronous functions in the block
> > layer were converted to use asynchronous methods by dynamically detecting if
> > they were being run from a coroutine context by calling qemu_in_coroutine(), and
> > yielding if so. If they were not, they would spawn a new coroutine and poll
> > until the asynchronous counterpart finished.
> > 
> > However this approach does not work with CPC as the CPC translator converts all
> > functions annotated coroutine_fn to a different (continuation based) calling
> > convention. This means that coroutine_fn annotated functions cannot be called
> > from a non-coroutine context.
> > 
> > This patch is a Request For Comments on the approach of splitting these
> > "dynamic" functions into synchronous and asynchronous versions. This is easy for
> > bdrv_flush as it already has an asynchronous counterpart - bdrv_co_flush. The
> > only caller of bdrv_flush from a coroutine context is mirror_drain in
> > block/mirror.c - this should be annotated as a coroutine_fn as it calls
> > qemu_coroutine_yield().
> > 
> > If this approach meets with approval I will develop a patchset splitting the
> > other "dynamic" functions in the block layer. This will allow all coroutine
> > functions to have a coroutine_fn annotation that can be statically checked (CPC
> > can be used to verify annotations).
> > 
> > I have audited the other callers of bdrv_flush, they are included below:
> > 
> > block.c: bdrv_reopen_prepare, bdrv_close, bdrv_commit, bdrv_pwrite_sync
> 
> bdrv_pwrite_sync() is a dynamic function.  If called from coroutine
> context it will yield (indirectly from bdrv_pwrite() or bdrv_flush()).
> 
> > block/qcow2-cache.c: qcow2_cache_entry_flush, qcow2_cache_flush
> > block/qcow2-refcount.c: qcow2_update_snapshot_refcount
> > block/qcow2-snapshot.c: qcow2_write_snapshots
> > block/qcow2.c: qcow2_mark_dirty, qcow2_mark_clean
> 
> qcow2 runs in coroutine context, the coroutine_fn annotations are just
> missing.  See block/qcow2.c:bdrv_qcow2 for the entry points like
> qcow2_co_readv.

Yes, you can't rely on coroutine_fn, it's missing in many places where
it should be there. But that was still the optimistic view.

The truth is that the greatest part of the qcow2 functions can be called
from eiher coroutine or non-coroutine context. You get coroutine context
for read/write/discard/flush, but anything else like doing snapshots,
resizing, preallocating the image, writing compressed data also accesses
the same metadata management functions outside coroutines.

It's only getting worse for function like bdrv_pwrite().

Kevin

  reply	other threads:[~2013-07-19  8:37 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-18 21:21 [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers Charlie Shepherd
2013-07-19  5:27 ` Stefan Hajnoczi
2013-07-19  8:37   ` Kevin Wolf [this message]
2013-07-23 12:05     ` Stefan Hajnoczi
2013-07-23 12:10       ` Gabriel Kerneis
2013-07-23 13:36         ` Stefan Hajnoczi
2013-07-23 14:00           ` Gabriel Kerneis
2013-07-25  3:47 ` Wenchao Xia

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130719083711.GC2992@dhcp-200-207.str.redhat.com \
    --to=kwolf@redhat.com \
    --cc=charlie@ctshepherd.com \
    --cc=gabriel@kerneis.info \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).