From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52244) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvbNs-0008S4-Bs for qemu-devel@nongnu.org; Fri, 13 Jun 2014 19:59:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvbNm-0002Ud-WD for qemu-devel@nongnu.org; Fri, 13 Jun 2014 19:59:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60504) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvbNm-0002UZ-L8 for qemu-devel@nongnu.org; Fri, 13 Jun 2014 19:59:42 -0400 Message-ID: <539B9064.3090000@redhat.com> Date: Sat, 14 Jun 2014 01:59:32 +0200 From: Max Reitz MIME-Version: 1.0 References: <1402493053-13787-1-git-send-email-benoit.canet@irqsave.net> <1402493053-13787-4-git-send-email-benoit.canet@irqsave.net> In-Reply-To: <1402493053-13787-4-git-send-email-benoit.canet@irqsave.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 3/4] block: Add replaces argument to drive-mirror List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Beno=EEt_Canet?= , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Benoit Canet , stefanha@redhat.com On 11.06.2014 15:24, Beno=EEt Canet wrote: > drive-mirror will bdrv_swap the new BDS named node-name with the one > pointed by replaces when the mirroring is finished. > > Signed-off-by: Benoit Canet > --- > block.c | 17 ++++++++++++++ > block/mirror.c | 60 +++++++++++++++++++++++++++++++++++++-= --------- > blockdev.c | 30 +++++++++++++++++++++++- > hmp.c | 2 +- > include/block/block.h | 4 ++++ > include/block/block_int.h | 4 +++- > qapi/block-core.json | 6 ++++- > qmp-commands.hx | 4 +++- > 8 files changed, 109 insertions(+), 18 deletions(-) > > diff --git a/block.c b/block.c > index 17f763d..318f1e6 100644 > --- a/block.c > +++ b/block.c > @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState *= candidate) > =20 > return false; > } > + > +BlockDriverState *check_to_replace_node(const char *node_name, Error *= *errp) > +{ > + BlockDriverState *to_replace_bs =3D bdrv_find_node(node_name); > + if (!to_replace_bs) { > + error_setg(errp, "Node name '%s' not found", > + node_name); > + return NULL; > + } > + > + if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)= ) { > + return NULL; > + } > + > + return to_replace_bs; > +} > + > diff --git a/block/mirror.c b/block/mirror.c > index 94c8661..b00365f 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob { > RateLimit limit; > BlockDriverState *target; > BlockDriverState *base; > + /* The name of the graph node to replace */ > + char *replaces; > + /* The block BDS to replace */ Maybe s/block // > + BlockDriverState *to_replace; > + /* Used to block operations on the drive-mirror-replace target */ > + Error *replace_blocker; > bool is_none_mode; > BlockdevOnError on_source_error, on_target_error; > bool synced; > @@ -490,10 +496,14 @@ immediate_exit: > bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > bdrv_iostatus_disable(s->target); > if (s->should_complete && ret =3D=3D 0) { > - if (bdrv_get_flags(s->target) !=3D bdrv_get_flags(s->common.bs= )) { > - bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL)= ; > + BlockDriverState *to_replace =3D s->common.bs; > + if (s->to_replace) { > + to_replace =3D s->to_replace; > } > - bdrv_swap(s->target, s->common.bs); > + if (bdrv_get_flags(s->target) !=3D bdrv_get_flags(to_replace))= { > + bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); > + } > + bdrv_swap(s->target, to_replace); > if (s->common.driver->job_type =3D=3D BLOCK_JOB_TYPE_COMMIT) = { > /* drop the bs loop chain formed by the swap: break the l= oop then > * trigger the unref from the top one */ > @@ -502,6 +512,12 @@ immediate_exit: > bdrv_unref(p); > } > } > + if (s->to_replace) { > + bdrv_op_unblock_all(s->to_replace, s->replace_blocker); > + error_free(s->replace_blocker); > + bdrv_unref(s->to_replace); > + g_free(s->replaces); If we get here without mirror_complete() having been invoked (which is=20 possible, as far as I know), s->to_replace will still be NULL while=20 s->replaces may be not. In that case, s->replaces will be leaked. > + } > bdrv_unref(s->target); > block_job_completed(&s->common, ret); > } > @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error *= *errp) > return; > } > =20 > + /* check the target bs is not block and block all operations on it= */ s/block/blocked/ > + if (s->replaces) { > + s->to_replace =3D check_to_replace_node(s->replaces, errp); > + > + if (!s->to_replace) { > + return; > + } > + > + error_setg(&s->replace_blocker, > + "block device is in use by block-job-complete"); > + bdrv_op_block_all(s->to_replace, s->replace_blocker); > + bdrv_ref(s->to_replace); > + } > + > s->should_complete =3D true; > block_job_resume(job); > } > @@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_dri= ver =3D { > }; > =20 > static void mirror_start_job(BlockDriverState *bs, BlockDriverState *= target, > - int64_t speed, int64_t granularity, > - int64_t buf_size, > - BlockdevOnError on_source_error, > - BlockdevOnError on_target_error, > - BlockDriverCompletionFunc *cb, > - void *opaque, Error **errp, > - const BlockJobDriver *driver, > - bool is_none_mode, BlockDriverState *base) > + const char *replaces, > + int64_t speed, int64_t granularity, > + int64_t buf_size, > + BlockdevOnError on_source_error, > + BlockdevOnError on_target_error, > + BlockDriverCompletionFunc *cb, > + void *opaque, Error **errp, > + const BlockJobDriver *driver, > + bool is_none_mode, BlockDriverState *base= ) > { > MirrorBlockJob *s; > =20 > @@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs, = BlockDriverState *target, > return; > } > =20 > + s->replaces =3D g_strdup(replaces); > s->on_source_error =3D on_source_error; > s->on_target_error =3D on_target_error; > s->target =3D target; > @@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs, = BlockDriverState *target, > } > =20 > void mirror_start(BlockDriverState *bs, BlockDriverState *target, > + const char *replaces, > int64_t speed, int64_t granularity, int64_t buf_siz= e, > MirrorSyncMode mode, BlockdevOnError on_source_erro= r, > BlockdevOnError on_target_error, > @@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriver= State *target, > =20 > is_none_mode =3D mode =3D=3D MIRROR_SYNC_MODE_NONE; > base =3D mode =3D=3D MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL= ; > - mirror_start_job(bs, target, speed, granularity, buf_size, > + mirror_start_job(bs, target, replaces, > + speed, granularity, buf_size, > on_source_error, on_target_error, cb, opaque, er= rp, > &mirror_job_driver, is_none_mode, base); > } > @@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, Bloc= kDriverState *base, > } > =20 > bdrv_ref(base); > - mirror_start_job(bs, base, speed, 0, 0, > + mirror_start_job(bs, base, NULL, speed, 0, 0, > on_error, on_error, cb, opaque, &local_err, > &commit_active_job_driver, false, base); > if (local_err) { > diff --git a/blockdev.c b/blockdev.c > index 06b14f2..237a548 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2107,6 +2107,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(= Error **errp) > void qmp_drive_mirror(const char *device, const char *target, > bool has_format, const char *format, > bool has_node_name, const char *node_name, > + bool has_replaces, const char *replaces, > enum MirrorSyncMode sync, > bool has_mode, enum NewImageMode mode, > bool has_speed, int64_t speed, > @@ -2194,6 +2195,28 @@ void qmp_drive_mirror(const char *device, const = char *target, > return; > } > =20 > + if (has_replaces) { > + BlockDriverState *to_replace_bs; > + > + if (!has_node_name) { > + error_setg(errp, "a node-name must be provided when replac= ing a" > + " named node of the graph"); > + return; > + } > + > + to_replace_bs =3D check_to_replace_node(replaces, errp); > + > + if (!to_replace_bs) { > + return; > + } > + > + if (size !=3D bdrv_getlength(to_replace_bs)) { > + error_setg(errp, "cannot replace image with a mirror image= of " > + "different size"); > + return; > + } > + } > + > if ((sync =3D=3D MIRROR_SYNC_MODE_FULL || !source) > && mode !=3D NEW_IMAGE_MODE_EXISTING) > { > @@ -2238,7 +2261,12 @@ void qmp_drive_mirror(const char *device, const = char *target, > return; > } > =20 > - mirror_start(bs, target_bs, speed, granularity, buf_size, sync, > + /* pass the node name to replace to mirror start since it's loose = coupling > + * and will allow to check whether the node still exist at mirror = completion > + */ > + mirror_start(bs, target_bs, > + has_replaces ? replaces : NULL, > + speed, granularity, buf_size, sync, > on_source_error, on_target_error, > block_job_cb, bs, &local_err); > if (local_err !=3D NULL) { > diff --git a/hmp.c b/hmp.c > index ef0d583..9a4b8da 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -930,7 +930,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qd= ict) > } > =20 > qmp_drive_mirror(device, filename, !!format, format, > - false, NULL, > + false, NULL, false, NULL, > full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_= TOP, > true, mode, false, 0, false, 0, false, 0, > false, 0, false, 0, &err); > diff --git a/include/block/block.h b/include/block/block.h > index 7d86e29..b0ed701 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -179,6 +179,7 @@ typedef enum BlockOpType { > BLOCK_OP_TYPE_MIRROR, > BLOCK_OP_TYPE_RESIZE, > BLOCK_OP_TYPE_STREAM, > + BLOCK_OP_TYPE_REPLACE, > BLOCK_OP_TYPE_MAX, > } BlockOpType; > =20 > @@ -319,6 +320,9 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverSt= ate *bs, > BlockDriverState *candidate); > bool bdrv_is_first_non_filter(BlockDriverState *candidate); > =20 > +/* check if a named node can be replaced when doing drive-mirror */ > +BlockDriverState *check_to_replace_node(const char *node_name, Error *= *errp); > + > /* async block I/O */ > typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t se= ctor, > int sector_num); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 8d58334..fdf3ed2 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -86,7 +86,6 @@ struct BlockDriver { > */ > bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs, > BlockDriverState *candid= ate); > - > int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *f= ilename); > int (*bdrv_probe_device)(const char *filename); > =20 This hunk doesn't seem intended...? Other than these, the patch looks fine to me. If you fix the leakage of=20 s->replaces (or explain why mirror_complete() is always invoked before=20 immediate_exit): Reviewed-by: Max Reitz