| Message ID | 1431105726-3682-20-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/08/2015 11:21 AM, 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 <kwolf@redhat.com> > --- > block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 67 insertions(+), 34 deletions(-) > In isolation, it looks like a valid conversion, so: Reviewed-by: Eric Blake <eblake@redhat.com> However, given that we are having a conversation on another thread about semantics for prepare vs. commit being the action that actually changes the in-memory representation, is this the correct split for however we end up resolving that bug?
Am 12.05.2015 um 23:40 hat Eric Blake geschrieben: > On 05/08/2015 11:21 AM, 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 <kwolf@redhat.com> > > --- > > block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++-------------------- > > 1 file changed, 67 insertions(+), 34 deletions(-) > > > > In isolation, it looks like a valid conversion, so: > > Reviewed-by: Eric Blake <eblake@redhat.com> > > However, given that we are having a conversation on another thread about > semantics for prepare vs. commit being the action that actually changes > the in-memory representation, is this the correct split for however we > end up resolving that bug? Reopen transactions are a different thing than QMP transactions, so I hope that they are not affected by that discussion. Generally, my assumption with reopen transactions is that the change only takes effect in commit. Kevin
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 <kwolf@redhat.com> > --- > 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 <mreitz@redhat.com> > + > +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; > }
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; + 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); + } +} + +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; }
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 <kwolf@redhat.com> --- block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 67 insertions(+), 34 deletions(-)