From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41571) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avP9V-0002Cj-BY for qemu-devel@nongnu.org; Wed, 27 Apr 2016 09:05:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avP9P-00059u-6A for qemu-devel@nongnu.org; Wed, 27 Apr 2016 09:05:13 -0400 References: <3b38569c5caeb92ae2fd6b3c6c7272bcce2dcf85.1459776815.git.berto@igalia.com> From: Max Reitz Message-ID: <5720B8F9.8080504@redhat.com> Date: Wed, 27 Apr 2016 15:04:57 +0200 MIME-Version: 1.0 In-Reply-To: <3b38569c5caeb92ae2fd6b3c6c7272bcce2dcf85.1459776815.git.berto@igalia.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="bEFJVX45ftVlDVxbG17k9xJVrABKfDrU3" Subject: Re: [Qemu-devel] [PATCH v9 06/11] block: Support streaming to an intermediate layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, Kevin Wolf , Eric Blake , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --bEFJVX45ftVlDVxbG17k9xJVrABKfDrU3 Content-Type: multipart/mixed; boundary="S98V7wqx5GNPSRocgNgOxEBwHRsHTR6vx" From: Max Reitz To: Alberto Garcia , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, Kevin Wolf , Eric Blake , Stefan Hajnoczi Message-ID: <5720B8F9.8080504@redhat.com> Subject: Re: [PATCH v9 06/11] block: Support streaming to an intermediate layer References: <3b38569c5caeb92ae2fd6b3c6c7272bcce2dcf85.1459776815.git.berto@igalia.com> In-Reply-To: <3b38569c5caeb92ae2fd6b3c6c7272bcce2dcf85.1459776815.git.berto@igalia.com> --S98V7wqx5GNPSRocgNgOxEBwHRsHTR6vx Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 04.04.2016 15:43, Alberto Garcia wrote: > This makes sure that the image we are steaming into is open in > read-write mode during the operation. >=20 > The block job is created on the destination image, but operation > blockers are also set on the active layer. We do this in order to > prevent other block jobs from running in parallel in the same chain. > See here for details on why that is currently not supported: >=20 > [Qemu-block] RFC: Status of the intermediate block streaming work > https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.html >=20 > Finally, this also unblocks the stream operation in backing files. >=20 > Signed-off-by: Alberto Garcia > --- > block.c | 4 +++- > block/stream.c | 39 ++++++++++++++++++++++++++++++++++++++-= > blockdev.c | 2 +- > include/block/block_int.h | 5 ++++- > 4 files changed, 46 insertions(+), 4 deletions(-) >=20 > diff --git a/block.c b/block.c > index 48638c9..f7698a1 100644 > --- a/block.c > +++ b/block.c > @@ -1252,9 +1252,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, B= lockDriverState *backing_hd) > backing_hd->drv ? backing_hd->drv->format_name : ""); > =20 > bdrv_op_block_all(backing_hd, bs->backing_blocker); > - /* Otherwise we won't be able to commit due to check in bdrv_commi= t */ > + /* Otherwise we won't be able to commit or stream */ > bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET, > bs->backing_blocker); > + bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM, > + bs->backing_blocker); > out: > bdrv_refresh_limits(bs, NULL); > } > diff --git a/block/stream.c b/block/stream.c > index 332b9a1..ac02ddf 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -35,8 +35,10 @@ typedef struct StreamBlockJob { > BlockJob common; > RateLimit limit; > BlockDriverState *base; > + BlockDriverState *active; > BlockdevOnError on_error; > char *backing_file_str; > + int bs_flags; > } StreamBlockJob; > =20 > static int coroutine_fn stream_populate(BlockDriverState *bs, > @@ -66,6 +68,11 @@ static void stream_complete(BlockJob *job, void *opa= que) > StreamCompleteData *data =3D opaque; > BlockDriverState *base =3D s->base; > =20 > + if (s->active) { > + bdrv_op_unblock_all(s->active, s->common.blocker); > + bdrv_unref(s->active); > + } > + > if (!block_job_is_cancelled(&s->common) && data->reached_end && > data->ret =3D=3D 0) { > const char *base_id =3D NULL, *base_fmt =3D NULL; > @@ -79,6 +86,11 @@ static void stream_complete(BlockJob *job, void *opa= que) > bdrv_set_backing_hd(job->bs, base); > } > =20 > + /* Reopen the image back in read-only mode if necessary */ > + if (s->bs_flags !=3D bdrv_get_flags(job->bs)) { > + bdrv_reopen(job->bs, s->bs_flags, NULL); > + } > + > g_free(s->backing_file_str); > block_job_completed(&s->common, data->ret); > g_free(data); > @@ -216,13 +228,15 @@ static const BlockJobDriver stream_job_driver =3D= { > .set_speed =3D stream_set_speed, > }; > =20 > -void stream_start(BlockDriverState *bs, BlockDriverState *base, > +void stream_start(BlockDriverState *bs, BlockDriverState *active, > + BlockDriverState *base, > const char *backing_file_str, int64_t speed, > BlockdevOnError on_error, > BlockCompletionFunc *cb, > void *opaque, Error **errp) > { > StreamBlockJob *s; > + int orig_bs_flags; > =20 > if ((on_error =3D=3D BLOCKDEV_ON_ERROR_STOP || > on_error =3D=3D BLOCKDEV_ON_ERROR_ENOSPC) && > @@ -231,13 +245,36 @@ void stream_start(BlockDriverState *bs, BlockDriv= erState *base, > return; > } > =20 > + /* Make sure that the image is opened in read-write mode */ > + orig_bs_flags =3D bdrv_get_flags(bs); > + if (!(orig_bs_flags & BDRV_O_RDWR)) { > + if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) !=3D 0)= { > + return; > + } > + } > + > s =3D block_job_create(&stream_job_driver, bs, speed, cb, opaque, = errp); > if (!s) { > return; > } > =20 > + /* If we are streaming to an intermediate image, we need to block > + * the active layer. Due to a race condition, having several block= > + * jobs running in the same chain is broken and we currently don't= > + * support it. See here for details: > + * https://lists.gnu.org/archive/html/qemu-block/2015-12/msg00180.= html > + */ > + if (active) { > + bdrv_op_block_all(active, s->common.blocker); block_job_create() unblocks BLOCK_OP_TYPE_DATAPLANE. Maybe this should do the same? No other objections from me. Max > + /* Hold a reference to the active layer, otherwise > + * bdrv_close_all() will destroy it if we shut down the VM */ > + bdrv_ref(active); > + } > + > s->base =3D base; > + s->active =3D active; > s->backing_file_str =3D g_strdup(backing_file_str); > + s->bs_flags =3D orig_bs_flags; > =20 > s->on_error =3D on_error; > s->common.co =3D qemu_coroutine_create(stream_run); > diff --git a/blockdev.c b/blockdev.c > index d1f5dfb..2e7712e 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3038,7 +3038,7 @@ void qmp_block_stream(const char *device, > /* backing_file string overrides base bs filename */ > base_name =3D has_backing_file ? backing_file : base_name; > =20 > - stream_start(bs, base_bs, base_name, has_speed ? speed : 0, > + stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0, > on_error, block_job_cb, bs, &local_err); > if (local_err) { > error_propagate(errp, local_err); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 10d8759..0a53d5f 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -597,6 +597,8 @@ int is_windows_drive(const char *filename); > /** > * stream_start: > * @bs: Block device to operate on. > + * @active: The active layer of the chain where @bs belongs, or %NULL > + * if @bs is already the topmost device. > * @base: Block device that will become the new base, or %NULL to > * flatten the whole backing file chain onto @bs. > * @base_id: The file name that will be written to @bs as the new > @@ -613,7 +615,8 @@ int is_windows_drive(const char *filename); > * streaming job, the backing file of @bs will be changed to > * @base_id in the written image and to @base in the live BlockDriverS= tate. > */ > -void stream_start(BlockDriverState *bs, BlockDriverState *base, > +void stream_start(BlockDriverState *bs, BlockDriverState *active, > + BlockDriverState *base, > const char *base_id, int64_t speed, BlockdevOnError = on_error, > BlockCompletionFunc *cb, > void *opaque, Error **errp); >=20 --S98V7wqx5GNPSRocgNgOxEBwHRsHTR6vx-- --bEFJVX45ftVlDVxbG17k9xJVrABKfDrU3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXILj5AAoJEDuxQgLoOKytV1sH/3C6B2IVe6KwrkSIftr0p6br FVTPOeqPpmfrFsfaIuUfTOBzHbVQ3lVL1vsC1lbQcVNfv5/CTiTjhgyN+7uuNHFK HJbwjGlHVCs0SDMLaWSIQVoUoBi2dI9IR66oHWSXNK+bggIdJHAiSw2pMVbBs1Wq TCdk8ByIJu3NHNUtDgL7q5vqumB8oFIHEO2mAbMol1S/aLPw6nFaHB8zdhJqfkE+ GCg6hSB4pLqpXfnGMAOKhk8S25e47p90CznPGRK2cOMOm/u2gEj8JVD2XdsPT2yr QC66xdTheD9gIwy3dqGo0dpfXFo7y2Ei3BALvGAVoiSR2hlEeKxM3l2pQDx3rbg= =B8/I -----END PGP SIGNATURE----- --bEFJVX45ftVlDVxbG17k9xJVrABKfDrU3--