From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48121) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agFgs-00038g-RF for qemu-devel@nongnu.org; Wed, 16 Mar 2016 13:57:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agFgr-00057p-LJ for qemu-devel@nongnu.org; Wed, 16 Mar 2016 13:57:02 -0400 Date: Wed, 16 Mar 2016 17:56:52 +0000 From: Stefan Hajnoczi Message-ID: <20160316175652.GB2012@stefanha-x1.localdomain> References: <1455645388-32401-1-git-send-email-pbonzini@redhat.com> <1455645388-32401-9-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="St7VIuEGZ6dlpu13" Content-Disposition: inline In-Reply-To: <1455645388-32401-9-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 08/16] blockjob: introduce .drain callback for jobs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org --St7VIuEGZ6dlpu13 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 16, 2016 at 06:56:20PM +0100, Paolo Bonzini wrote: > This is required to decouple block jobs from running in an > AioContext. With multiqueue block devices, a BlockDriverState > does not really belong to a single AioContext. >=20 > The solution is to first wait until all I/O operations are > complete; then loop in the main thread for the block job to > complete entirely. >=20 > Signed-off-by: Paolo Bonzini > --- > block/backup.c | 7 +++++++ > block/mirror.c | 12 ++++++++++-- > blockjob.c | 16 +++++++++++++--- > include/block/blockjob.h | 7 +++++++ > 4 files changed, 37 insertions(+), 5 deletions(-) >=20 > diff --git a/block/backup.c b/block/backup.c > index 00cafdb..2edb895 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -251,6 +251,12 @@ static void backup_abort(BlockJob *job) > } > } > =20 > +static void backup_drain(BlockJob *job) > +{ > + BackupBlockJob *s =3D container_of(job, BackupBlockJob, common); > + bdrv_drain(s->target); > +} > + > static const BlockJobDriver backup_job_driver =3D { > .instance_size =3D sizeof(BackupBlockJob), > .job_type =3D BLOCK_JOB_TYPE_BACKUP, > @@ -258,6 +264,7 @@ static const BlockJobDriver backup_job_driver =3D { > .iostatus_reset =3D backup_iostatus_reset, > .commit =3D backup_commit, > .abort =3D backup_abort, > + .drain =3D backup_drain, > }; > =20 > static BlockErrorAction backup_error_action(BackupBlockJob *job, > diff --git a/block/mirror.c b/block/mirror.c > index 3f163b8..b473a1b 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -358,7 +358,7 @@ static void mirror_free_init(MirrorBlockJob *s) > } > } > =20 > -static void mirror_drain(MirrorBlockJob *s) > +static void mirror_wait_for_completion(MirrorBlockJob *s) > { > while (s->in_flight > 0) { > s->waiting_for_io =3D true; > @@ -627,7 +627,7 @@ immediate_exit: > * the target is a copy of the source. > */ > assert(ret < 0 || (!s->synced && block_job_is_cancelled(&s->comm= on))); > - mirror_drain(s); > + mirror_wait_for_completion(s); > } > =20 > assert(s->in_flight =3D=3D 0); > @@ -708,12 +708,19 @@ static void mirror_complete(BlockJob *job, Error **= errp) > block_job_enter(&s->common); > } > =20 > +static void mirror_drain(BlockJob *job) > +{ > + MirrorBlockJob *s =3D container_of(job, MirrorBlockJob, common); > + bdrv_drain(s->target); > +} > + > static const BlockJobDriver mirror_job_driver =3D { > .instance_size =3D sizeof(MirrorBlockJob), > .job_type =3D BLOCK_JOB_TYPE_MIRROR, > .set_speed =3D mirror_set_speed, > .iostatus_reset=3D mirror_iostatus_reset, > .complete =3D mirror_complete, > + .drain =3D mirror_drain, > }; > =20 > static const BlockJobDriver commit_active_job_driver =3D { > @@ -723,6 +730,7 @@ static const BlockJobDriver commit_active_job_driver = =3D { > .iostatus_reset > =3D mirror_iostatus_reset, > .complete =3D mirror_complete, > + .drain =3D mirror_drain, > }; > =20 > static void mirror_start_job(BlockDriverState *bs, BlockDriverState *tar= get, > diff --git a/blockjob.c b/blockjob.c > index 9fc37ca..5bb6f9b 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -289,16 +289,26 @@ static int block_job_finish_sync(BlockJob *job, > assert(bs->job =3D=3D job); > =20 > block_job_ref(job); > + > + /* finish will call block_job_enter (see e.g. block_job_cancel, > + * or mirror_complete in block/mirror.c). Barring bugs in the > + * job coroutine, bdrv_drain should be enough to induce progress > + * until the job completes or moves to the main thread. > + */ > finish(job, &local_err); > if (local_err) { > error_propagate(errp, local_err); > block_job_unref(job); > return -EBUSY; > } > + while (!job->deferred_to_main_loop && !job->completed) { > + bdrv_drain(bs); > + if (job->driver->drain) { > + job->driver->drain(job); > + } > + } > while (!job->completed) { > - aio_poll(job->deferred_to_main_loop ? qemu_get_aio_context() : > - bdrv_get_aio_context(bs), > - true); > + aio_poll(qemu_get_aio_context(), true); > } This adds the assumption that block_job_defer_to_main_loop() is only used to complete the block job. If we really need to make this assumption then we could rename it complete_in_main_loop() or similar so that is clear. > ret =3D (job->cancelled && job->ret =3D=3D 0) ? -ECANCELED : job->re= t; > block_job_unref(job); > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index 8bedc49..8e564df 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -70,6 +70,13 @@ typedef struct BlockJobDriver { > * never both. > */ > void (*abort)(BlockJob *job); > + > + /** > + * If the callback is not NULL, it will be invoked when the job has = to be > + * synchronously cancelled or completed; it should drain BlockDriver= States > + * as required to ensure progress. > + */ > + void (*drain)(BlockJob *job); > } BlockJobDriver; > =20 > /** > --=20 > 2.5.0 >=20 >=20 --St7VIuEGZ6dlpu13 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJW6Z5kAAoJEJykq7OBq3PIZCMH/2LU6IxfzDDDBg+H7BdO7mFS 80+HfOVzcujy5XVoOt0/aRGZcKl3wMZVWDJJl9fM0cjW0WqIMSB5JMi30KRyqJZ1 VW/sDLnHTXNDo8O4H7M+9COjF4eML1I3eImC6+7z4Cb7In05FkLoDPu2+HCjj9oC IDpwMDX2O4zZr4w53OzVCJ2vjNSsuiGeUjvz1cS4VmrUF/WKSv6uN7jt54yXq0oG VbEK7QvXZeeZQluCbfoQBPbbiDtAv01MwwI1QYq8GnjkKXG5Epf+3kyHLAglvMh5 U7CIoAbHYLgiTz2L8iVZOHwQBfngKEpJL6ERo5WaTlaCaI+YDcKkC5ux3fpo7aY= =r5/g -----END PGP SIGNATURE----- --St7VIuEGZ6dlpu13--