From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36466) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZPEe-0004do-DO for qemu-devel@nongnu.org; Wed, 01 Oct 2014 15:06:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZPEY-0003Ur-4J for qemu-devel@nongnu.org; Wed, 01 Oct 2014 15:06:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZPEX-0003Ui-RE for qemu-devel@nongnu.org; Wed, 01 Oct 2014 15:06:42 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s91J6eg6032719 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 1 Oct 2014 15:06:41 -0400 Message-ID: <542C50BC.8010305@redhat.com> Date: Wed, 01 Oct 2014 21:06:36 +0200 From: Max Reitz MIME-Version: 1.0 References: <1412182919-9550-1-git-send-email-stefanha@redhat.com> <1412182919-9550-6-git-send-email-stefanha@redhat.com> In-Reply-To: <1412182919-9550-6-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/11] blockjob: add block_job_defer_to_main_loop() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , qemu-devel@nongnu.org Cc: Kevin Wolf , Paolo Bonzini , Fam Zheng On 01.10.2014 19:01, Stefan Hajnoczi wrote: > Block jobs will run in the BlockDriverState's AioContext, which may not > always be the QEMU main loop. > > There are some block layer APIs that are either not thread-safe or risk > lock ordering problems. This includes bdrv_unref(), bdrv_close(), and > anything that calls bdrv_drain_all(). > > The block_job_defer_to_main_loop() API allows a block job to schedule a > function to run in the main loop with the BlockDriverState AioContext > held. > > This function will be used to perform cleanup and backing chain > manipulations in block jobs. > > Signed-off-by: Stefan Hajnoczi > --- > blockjob.c | 35 +++++++++++++++++++++++++++++++++++ > include/block/blockjob.h | 19 +++++++++++++++++++ > 2 files changed, 54 insertions(+) > > diff --git a/blockjob.c b/blockjob.c > index 0689fdd..24a64d8 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -313,3 +313,38 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, > } > return action; > } > + > +typedef struct { > + BlockJob *job; > + QEMUBH *bh; > + AioContext *aio_context; > + BlockJobDeferToMainLoopFn *fn; > + void *opaque; > +} BlockJobDeferToMainLoopData; > + > +static void block_job_defer_to_main_loop_bh(void *opaque) > +{ > + BlockJobDeferToMainLoopData *data = opaque; > + > + qemu_bh_delete(data->bh); > + > + aio_context_acquire(data->aio_context); > + data->fn(data->job, data->opaque); > + aio_context_release(data->aio_context); > + > + g_free(data); > +} > + > +void block_job_defer_to_main_loop(BlockJob *job, > + BlockJobDeferToMainLoopFn *fn, > + void *opaque) > +{ > + BlockJobDeferToMainLoopData *data = g_malloc(sizeof(*data)); > + data->job = job; > + data->bh = qemu_bh_new(block_job_defer_to_main_loop_bh, data); > + data->aio_context = bdrv_get_aio_context(job->bs); > + data->fn = fn; > + data->opaque = opaque; > + > + qemu_bh_schedule(data->bh); > +} I'm not so sure whether it'd be possible to change the BDS's AIO context (in another thread) after the call to bdrv_get_aio_context() in block_job_defer_to_main_loop() and before block_job_defer_to_main_loop_bh() is run. If so, this might create race conditions. Other than that, this patch looks good. Max