From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40772) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V06Bs-0002cz-3K for qemu-devel@nongnu.org; Fri, 19 Jul 2013 04:37:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V06Bq-0006cM-Oh for qemu-devel@nongnu.org; Fri, 19 Jul 2013 04:37:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38083) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V06Bq-0006cE-Ht for qemu-devel@nongnu.org; Fri, 19 Jul 2013 04:37:26 -0400 Date: Fri, 19 Jul 2013 10:37:11 +0200 From: Kevin Wolf Message-ID: <20130719083711.GC2992@dhcp-200-207.str.redhat.com> References: <1374182502-10292-1-git-send-email-charlie@ctshepherd.com> <20130719052744.GF14330@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130719052744.GF14330@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Charlie Shepherd , gabriel@kerneis.info, qemu-devel@nongnu.org, pbonzini@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