From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33898) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEx3Z-0004b4-U5 for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:03:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEx3S-0006Br-PY for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:03:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50570) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEx3S-0006Aj-IN for qemu-devel@nongnu.org; Tue, 14 Jul 2015 06:03:14 -0400 Date: Tue, 14 Jul 2015 11:03:11 +0100 From: Stefan Hajnoczi Message-ID: <20150714100311.GE17927@stefanha-thinkpad.redhat.com> References: <1436500012-32593-1-git-send-email-famz@redhat.com> <1436500012-32593-10-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="WK3l2KTTmXPVedZ6" Content-Disposition: inline In-Reply-To: <1436500012-32593-10-git-send-email-famz@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: Fam Zheng Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, John Snow --WK3l2KTTmXPVedZ6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D true; > job->ret =3D 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 =3D 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 =3D 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. > @@ -344,44 +348,36 @@ BlockErrorAction block_job_error_action(BlockJob *j= ob, BlockDriverState *bs, > return action; > } > =20 > -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 =3D opaque; > + BlockJob *job =3D opaque; > + /* Copy the struct in case job get released in data.fn() */ > + BlockJobDeferToMainLoopData data =3D 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! --WK3l2KTTmXPVedZ6 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVpN5fAAoJEJykq7OBq3PIXnoIAJi2gDKaYLIvSVYwBpoe4sce ua1rHicytJFPWRJeDz7lvaWCUX5S8/dhtyV1lC8VIqAkziwopKTeOYtAcdWJepm2 roZm2RR6nRaIwilQm33e3e3Uz9mvvNIyUESj6i/RkDbDaCIPjMc+ysy0sgE1JfOp xTjc6R0RJ09eSniCZHj5PRvwkSa0KdL1nHWQPIav2qV0NsSNMCyPsp+uh/WS6+j/ 2vlj8KzUg7rpiuK59VSeALteFjqzxGMhls9rLuUlzb6Me/QhRk38cwK5L/8nWxje QPFqb0nVUTOsxChBemkza276B8lNAHGWDoqHm0qEnkr7QNE2I68xUuKgzKCqLc0= =1eDo -----END PGP SIGNATURE----- --WK3l2KTTmXPVedZ6--