From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52374) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGBGn-0005j3-CM for qemu-devel@nongnu.org; Mon, 04 Jan 2016 14:58:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGBGm-0008Cm-7J for qemu-devel@nongnu.org; Mon, 04 Jan 2016 14:58:21 -0500 References: <1450850347-5291-1-git-send-email-famz@redhat.com> <1450850347-5291-5-git-send-email-famz@redhat.com> <567B4207.8030300@redhat.com> <20151224032538.GC23372@ad.usersys.redhat.com> From: Max Reitz Message-ID: <568ACED0.3090806@redhat.com> Date: Mon, 4 Jan 2016 20:58:08 +0100 MIME-Version: 1.0 In-Reply-To: <20151224032538.GC23372@ad.usersys.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="1P6sGhEXQjf43p59d4C3cLEEkhKVe9r77" Subject: Re: [Qemu-devel] [PATCH v3 4/5] qmp: Add blockdev-mirror command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , qemu-block@nongnu.org, Jeff Cody , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --1P6sGhEXQjf43p59d4C3cLEEkhKVe9r77 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 24.12.2015 04:25, Fam Zheng wrote: > On Thu, 12/24 01:53, Max Reitz wrote: >> On 23.12.2015 06:59, Fam Zheng wrote: >>> This will start a mirror job from a named device to another named >>> device, its relation with drive-mirror is similar with blockdev-backu= p >>> to drive-backup. >>> >>> In blockdev-mirror, the target node should be prepared by blockdev-ad= d, >>> which will be responsible for assigning a name to the new node, so >>> we don't have 'node-name' parameter. >>> >>> Signed-off-by: Fam Zheng >>> --- >>> blockdev.c | 62 ++++++++++++++++++++++++++++++++++++++++++= ++++++++++ >>> qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++ >>> qmp-commands.hx | 48 ++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 157 insertions(+) >> >> It appears you haven't addressed the comments for v2. I only had a >> single one (regarding documentation), but Markus had a couple ones, so= >> those may be worth addressing. >=20 > Will look into that. >=20 >> >>> >>> diff --git a/blockdev.c b/blockdev.c >>> index f42e171..2df0c6d 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -3345,6 +3345,10 @@ static void blockdev_mirror_common(BlockDriver= State *bs, >>> if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp= )) { >>> return; >>> } >>> + if (target->blk) { >>> + error_setg(errp, "Cannot mirror to an attached block device"= ); >>> + return; >>> + } >>> =20 >>> if (!bs->backing && sync =3D=3D MIRROR_SYNC_MODE_TOP) { >>> sync =3D MIRROR_SYNC_MODE_FULL; >>> @@ -3518,6 +3522,64 @@ out: >>> aio_context_release(aio_context); >>> } >>> =20 >>> +void qmp_blockdev_mirror(const char *device, const char *target, >>> + bool has_replaces, const char *replaces, >>> + MirrorSyncMode sync, >>> + bool has_speed, int64_t speed, >>> + bool has_granularity, uint32_t granularity,= >>> + bool has_buf_size, int64_t buf_size, >>> + bool has_on_source_error, >>> + BlockdevOnError on_source_error, >>> + bool has_on_target_error, >>> + BlockdevOnError on_target_error, >>> + Error **errp) >>> +{ >>> + BlockDriverState *bs; >>> + BlockBackend *blk; >>> + BlockDriverState *target_bs; >>> + AioContext *aio_context; >>> + Error *local_err =3D NULL; >>> + >>> + blk =3D blk_by_name(device); >>> + if (!blk) { >>> + error_setg(errp, "Device '%s' not found", device); >>> + return; >>> + } >>> + bs =3D blk_bs(blk); >>> + >>> + if (!bs) { >>> + error_setg(errp, "Device '%s' has no media", device); >>> + return; >>> + } >>> + >>> + target_bs =3D bdrv_lookup_bs(target, target, errp); >>> + if (!target_bs) { >>> + return; >>> + } >>> + >>> + aio_context =3D bdrv_get_aio_context(bs); >>> + aio_context_acquire(aio_context); >>> + >>> + bdrv_ref(target_bs); >>> + bdrv_set_aio_context(target_bs, aio_context); >>> + >>> + blockdev_mirror_common(bs, target_bs, >>> + has_replaces, replaces, sync, >>> + has_speed, speed, >>> + has_granularity, granularity, >>> + has_buf_size, buf_size, >>> + has_on_source_error, on_source_error, >>> + has_on_target_error, on_target_error, >>> + true, true, >> >> Shouldn't this be "false, false,", or, ideally, set by the user? >=20 > I think true is correct here because then it will be effectively contro= lled by > open flags of target. I.e. mirror.c always sets BDRV_REQ_MAY_UNMAP, and= > bdrv_co_write_zeroes has: >=20 > if (!(bs->open_flags & BDRV_O_UNMAP)) { > flags &=3D ~BDRV_REQ_MAY_UNMAP; > } I was asking because it differs from what drive-mirror does - but that is probably a good thing (drive-mirror takes this flag from the user (defaulting to false, which is why I was asking), but it takes the open_flags for the new image from the mirror source, which is... Interesting. So it's probably better this way, right. Max --1P6sGhEXQjf43p59d4C3cLEEkhKVe9r77 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 iQEcBAEBCAAGBQJWis7QAAoJEDuxQgLoOKytYnUH/2UL/IX4rTQ4xtWCN+/IH377 Z9zuhesPly6FCDxSF4WAnyv54Ik2WB0lcIa0MxVC7wZ3tKLhsyoLFPNdKhpYSs8t GdXuqNprOgl1Tq0iHjQV/h6LVqv7kMtLEncsODWbJd/h8flvmpSLRq+F5B6H8+4W Wa/g/2BlcNJXNRnITYoNJ98Oo0sajHTUPT8xcZdz32geaWXPFX3f+DjQ5G3Ny763 dIDhkpIfLRB4AM/M/3meZ2b2nJPVf4SyT2agGus2MEhmjzAS4PxZnwjnnpI/YaGh x1w1qoEkD/YZevLOif8u7Evn91yDU98ztrXUC2ek4HvMQyKe/CVx27tyMb8lpvo= =PWBv -----END PGP SIGNATURE----- --1P6sGhEXQjf43p59d4C3cLEEkhKVe9r77--