From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsVQg-0000hK-9p for qemu-devel@nongnu.org; Wed, 13 May 2015 08:06:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsVQc-0004xr-2J for qemu-devel@nongnu.org; Wed, 13 May 2015 08:06:26 -0400 Message-ID: <55533E37.5060200@redhat.com> Date: Wed, 13 May 2015 14:06:15 +0200 From: Max Reitz MIME-Version: 1.0 References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-20-git-send-email-kwolf@redhat.com> In-Reply-To: <1431105726-3682-20-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 19/34] qcow2: Make qcow2_update_options() suitable for transactions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: armbru@redhat.com, qemu-devel@nongnu.org On 08.05.2015 19:21, Kevin Wolf wrote: > Before we can allow updating options at runtime with bdrv_reopen(), we > need to split the function into prepare/commit/abort parts. > > Signed-off-by: Kevin Wolf > --- > block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 67 insertions(+), 34 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 84d6e0f..fc9375e 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -538,17 +538,24 @@ static void read_cache_sizes(QemuOpts *opts, uint64_t *l2_cache_size, > } > } > > -static int qcow2_update_options(BlockDriverState *bs, QDict *options, > - int flags, Error **errp) > +typedef struct Qcow2ReopenState { > + Qcow2Cache* l2_table_cache; > + Qcow2Cache* refcount_block_cache; *cough* > + bool use_lazy_refcounts; > + int overlap_check; > + bool discard_passthrough[QCOW2_DISCARD_MAX]; > +} Qcow2ReopenState; > + > +static int qcow2_update_options_prepare(BlockDriverState *bs, > + Qcow2ReopenState *r, > + QDict *options, int flags, > + Error **errp) > { > BDRVQcowState *s = bs->opaque; > QemuOpts *opts = NULL; > const char *opt_overlap_check, *opt_overlap_check_template; > int overlap_check_template = 0; > uint64_t l2_cache_size, refcount_cache_size; > - Qcow2Cache* l2_table_cache = NULL; > - Qcow2Cache* refcount_block_cache = NULL; > - bool use_lazy_refcounts; > int i; > Error *local_err = NULL; > int ret; > @@ -590,18 +597,18 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options, > } > > /* alloc L2 table/refcount block cache */ > - l2_table_cache = qcow2_cache_create(bs, l2_cache_size); > - refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size); > - if (l2_table_cache == NULL || refcount_block_cache == NULL) { > + r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size); > + r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size); > + if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) { > error_setg(errp, "Could not allocate metadata caches"); > ret = -ENOMEM; > goto fail; > } > > /* Enable lazy_refcounts according to image and command line options */ > - use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS, > + r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS, > (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS)); > - if (use_lazy_refcounts && s->qcow_version < 3) { > + if (r->use_lazy_refcounts && s->qcow_version < 3) { > error_setg(errp, "Lazy refcounts require a qcow2 image with at least " > "qemu 1.1 compatibility level"); > ret = -EINVAL; > @@ -640,46 +647,72 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options, > goto fail; > } > > - /* > - * Start updating fields in BDRVQcowState. > - * After this point no failure is allowed any more. > - */ > - s->overlap_check = 0; > + r->overlap_check = 0; > for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) { > /* overlap-check defines a template bitmask, but every flag may be > * overwritten through the associated boolean option */ > - s->overlap_check |= > + r->overlap_check |= > qemu_opt_get_bool(opts, overlap_bool_option_names[i], > overlap_check_template & (1 << i)) << i; > } > > - s->l2_table_cache = l2_table_cache; > - s->refcount_block_cache = refcount_block_cache; > - > - s->use_lazy_refcounts = use_lazy_refcounts; > - > - s->discard_passthrough[QCOW2_DISCARD_NEVER] = false; > - s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true; > - s->discard_passthrough[QCOW2_DISCARD_REQUEST] = > + r->discard_passthrough[QCOW2_DISCARD_NEVER] = false; > + r->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true; > + r->discard_passthrough[QCOW2_DISCARD_REQUEST] = > qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST, > flags & BDRV_O_UNMAP); > - s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] = > + r->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] = > qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true); > - s->discard_passthrough[QCOW2_DISCARD_OTHER] = > + r->discard_passthrough[QCOW2_DISCARD_OTHER] = > qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false); > > ret = 0; > fail: > - if (ret < 0) { > - if (l2_table_cache) { > - qcow2_cache_destroy(bs, l2_table_cache); > - } > - if (refcount_block_cache) { > - qcow2_cache_destroy(bs, refcount_block_cache); > - } > - } > qemu_opts_del(opts); > opts = NULL; > + return ret; > +} > + > +static void qcow2_update_options_commit(BlockDriverState *bs, > + Qcow2ReopenState *r) > +{ > + BDRVQcowState *s = bs->opaque; > + int i; > + > + s->l2_table_cache = r->l2_table_cache; > + s->refcount_block_cache = r->refcount_block_cache; > + > + s->overlap_check = r->overlap_check; > + s->use_lazy_refcounts = r->use_lazy_refcounts; > + > + for (i = 0; i < QCOW2_DISCARD_MAX; i++) { > + s->discard_passthrough[i] = r->discard_passthrough[i]; > + } > +} > + > +static void qcow2_update_options_abort(BlockDriverState *bs, > + Qcow2ReopenState *r) > +{ > + if (r->l2_table_cache) { > + qcow2_cache_destroy(bs, r->l2_table_cache); > + } > + if (r->refcount_block_cache) { > + qcow2_cache_destroy(bs, r->refcount_block_cache); > + } > +} Okay, now it makes sense not to set {l2_table,refcount_block}_cache to NULL in patch 18 after they've been moved to *s. Reviewed-by: Max Reitz > + > +static int qcow2_update_options(BlockDriverState *bs, QDict *options, > + int flags, Error **errp) > +{ > + Qcow2ReopenState r = {}; > + int ret; > + > + ret = qcow2_update_options_prepare(bs, &r, options, flags, errp); > + if (ret >= 0) { > + qcow2_update_options_commit(bs, &r); > + } else { > + qcow2_update_options_abort(bs, &r); > + } > > return ret; > }