From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZExZi-0000jH-46 for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:36:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZExZe-0006Pk-B6 for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:36:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54526) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZExZe-0006Pg-6F for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:36:30 -0400 Date: Tue, 14 Jul 2015 18:36:26 +0800 From: Fam Zheng Message-ID: <20150714103626.GG27873@ad.nay.redhat.com> References: <1436500012-32593-1-git-send-email-famz@redhat.com> <1436500012-32593-10-git-send-email-famz@redhat.com> <20150714100311.GE17927@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150714100311.GE17927@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 09/15] blockjob: Move BlockJobDeferToMainLoopData into BlockJob List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, John Snow On Tue, 07/14 11:03, Stefan Hajnoczi wrote: > On Fri, Jul 10, 2015 at 11:46:46AM +0800, Fam Zheng wrote: > > diff --git a/blockjob.c b/blockjob.c > > index 11b48f5..e057dd5 100644 > > --- a/blockjob.c > > +++ b/blockjob.c > > @@ -92,6 +92,10 @@ void block_job_completed(BlockJob *job, int ret) > > assert(!job->completed); > > job->completed = true; > > job->ret = ret; > > + if (job->defer_to_main_loop_data.bh) { > > + qemu_bh_delete(job->defer_to_main_loop_data.bh); > > + job->defer_to_main_loop_data.bh = NULL; > > + } > > job->cb(job->opaque, ret); > > block_job_release(bs); > > } > > This doesn't make sense to me: > > 1. This function is called from a defer_to_main_loop BH so > job->defer_to_main_loop_data.bh should already be running here. > > In fact, we've already called qemu_bh_delete() in > block_job_defer_to_main_loop_bh(). This call is pointless but you > could change it to an assertion and assign bh = NULL in > block_job_defer_to_main_loop_bh(). > > 2. If there was a BH scheduled, why is it safe to silently delete it? > Wouldn't the caller rely on the BH code executing to clean up, for > example? > > If you drop this if statement, is it necessary to move > BlockJobDeferToMainLoopData into BlockJob at all? Maybe you can just > drop this patch. You're right, I agree this patch is superfluous and can be dropped. Fam > > > @@ -344,44 +348,36 @@ 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; > > + BlockJob *job = opaque; > > + /* Copy the struct in case job get released in data.fn() */ > > + BlockJobDeferToMainLoopData data = job->defer_to_main_loop_data; > > A comment is warranted since this access is only safe due to the > following: > > The job may still be executing in another thread (the AioContext hasn't > been acquired by us yet), but job->defer_to_main_loop_data isn't > modified by the other thread after the qemu_bh_schedule() call. > > Additionally, the atomic_xchg memory barriers in qemu_bh_schedule() and > aio_bh_poll() to ensure that this thread sees the latest > job->defer_to_main_loop_data data. > > Access to other job fields would probably not be safe here!