diff mbox

[20/34] qcow2: Support updating driver-specific options in reopen

Message ID 1431105726-3682-21-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf May 8, 2015, 5:21 p.m. UTC 
For updating the cache sizes or disabling lazy refcounts there is a bit
more to do than just changing the variables, but otherwise we're all set
for changing options during bdrv_reopen().

Just implement the missing pieces and hook the functions up in
bdrv_reopen().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 6 deletions(-)

Comments

Eric Blake May 12, 2015, 9:47 p.m. UTC | #1 
On 05/08/2015 11:21 AM, Kevin Wolf wrote:
> For updating the cache sizes or disabling lazy refcounts there is a bit
> more to do than just changing the variables, but otherwise we're all set
> for changing options during bdrv_reopen().
> 
> Just implement the missing pieces and hook the functions up in
> bdrv_reopen().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 64 insertions(+), 6 deletions(-)
> 

> -/* We have no actual commit/abort logic for qcow2, but we need to write out any
> - * unwritten data if we reopen read-only. */
>  static int qcow2_reopen_prepare(BDRVReopenState *state,
>                                  BlockReopenQueue *queue, Error **errp)
>  {
> +    Qcow2ReopenState *r;
>      int ret;
>  
> +    r = g_new0(Qcow2ReopenState, 1);
> +    state->opaque = r;
> +
> +    ret = qcow2_update_options_prepare(state->bs, r, state->options,
> +                                       state->flags, errp);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    /* We need to write out any unwritten data if we reopen read-only. */
>      if ((state->flags & BDRV_O_RDWR) == 0) {
>          ret = bdrv_flush(state->bs);
>          if (ret < 0) {
> -            return ret;
> +            goto fail;
>          }
>  
>          ret = qcow2_mark_clean(state->bs);
>          if (ret < 0) {
> -            return ret;
> +            goto fail;
>          }
>      }
>  
>      return 0;
> +
> +fail:
> +    qcow2_update_options_abort(state->bs, r);
> +    return ret;

Doesn't this leak r?  That is, you only free r if _commit or _abort is
reached, but my understanding of transaction semantics is that we only
guarantee that one of those is reached if _prepare succeeded.
Kevin Wolf May 13, 2015, 9:26 a.m. UTC | #2 
Am 12.05.2015 um 23:47 hat Eric Blake geschrieben:
> On 05/08/2015 11:21 AM, Kevin Wolf wrote:
> > For updating the cache sizes or disabling lazy refcounts there is a bit
> > more to do than just changing the variables, but otherwise we're all set
> > for changing options during bdrv_reopen().
> > 
> > Just implement the missing pieces and hook the functions up in
> > bdrv_reopen().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 64 insertions(+), 6 deletions(-)
> > 
> 
> > -/* We have no actual commit/abort logic for qcow2, but we need to write out any
> > - * unwritten data if we reopen read-only. */
> >  static int qcow2_reopen_prepare(BDRVReopenState *state,
> >                                  BlockReopenQueue *queue, Error **errp)
> >  {
> > +    Qcow2ReopenState *r;
> >      int ret;
> >  
> > +    r = g_new0(Qcow2ReopenState, 1);
> > +    state->opaque = r;
> > +
> > +    ret = qcow2_update_options_prepare(state->bs, r, state->options,
> > +                                       state->flags, errp);
> > +    if (ret < 0) {
> > +        goto fail;
> > +    }
> > +
> > +    /* We need to write out any unwritten data if we reopen read-only. */
> >      if ((state->flags & BDRV_O_RDWR) == 0) {
> >          ret = bdrv_flush(state->bs);
> >          if (ret < 0) {
> > -            return ret;
> > +            goto fail;
> >          }
> >  
> >          ret = qcow2_mark_clean(state->bs);
> >          if (ret < 0) {
> > -            return ret;
> > +            goto fail;
> >          }
> >      }
> >  
> >      return 0;
> > +
> > +fail:
> > +    qcow2_update_options_abort(state->bs, r);
> > +    return ret;
> 
> Doesn't this leak r?  That is, you only free r if _commit or _abort is
> reached, but my understanding of transaction semantics is that we only
> guarantee that one of those is reached if _prepare succeeded.

Yes, it does. Thanks for catching that.

Kevin
diff mbox

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index fc9375e..940447b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -596,7 +596,24 @@  static int qcow2_update_options_prepare(BlockDriverState *bs,
         goto fail;
     }
 
-    /* alloc L2 table/refcount block cache */
+    /* alloc new L2 table/refcount block cache, flush old one */
+    if (s->l2_table_cache) {
+        ret = qcow2_cache_flush(bs, s->l2_table_cache);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to flush the L2 table cache");
+            goto fail;
+        }
+    }
+
+    if (s->refcount_block_cache) {
+        ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+        if (ret) {
+            error_setg_errno(errp, -ret,
+                             "Failed to flush the refcount block cache");
+            goto fail;
+        }
+    }
+
     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) {
@@ -605,7 +622,7 @@  static int qcow2_update_options_prepare(BlockDriverState *bs,
         goto fail;
     }
 
-    /* Enable lazy_refcounts according to image and command line options */
+    /* lazy-refcounts; flush if going from enabled to disabled */
     r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
         (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
     if (r->use_lazy_refcounts && s->qcow_version < 3) {
@@ -615,6 +632,14 @@  static int qcow2_update_options_prepare(BlockDriverState *bs,
         goto fail;
     }
 
+    if (s->use_lazy_refcounts && !r->use_lazy_refcounts) {
+        ret = qcow2_mark_clean(bs);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to disable lazy refcounts");
+            goto fail;
+        }
+    }
+
     /* Overlap check options */
     opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
     opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
@@ -679,6 +704,12 @@  static void qcow2_update_options_commit(BlockDriverState *bs,
     BDRVQcowState *s = bs->opaque;
     int i;
 
+    if (s->l2_table_cache) {
+        qcow2_cache_destroy(bs, s->l2_table_cache);
+    }
+    if (s->refcount_block_cache) {
+        qcow2_cache_destroy(bs, s->refcount_block_cache);
+    }
     s->l2_table_cache = r->l2_table_cache;
     s->refcount_block_cache = r->refcount_block_cache;
 
@@ -1135,26 +1166,51 @@  static int qcow2_set_key(BlockDriverState *bs, const char *key)
     return 0;
 }
 
-/* We have no actual commit/abort logic for qcow2, but we need to write out any
- * unwritten data if we reopen read-only. */
 static int qcow2_reopen_prepare(BDRVReopenState *state,
                                 BlockReopenQueue *queue, Error **errp)
 {
+    Qcow2ReopenState *r;
     int ret;
 
+    r = g_new0(Qcow2ReopenState, 1);
+    state->opaque = r;
+
+    ret = qcow2_update_options_prepare(state->bs, r, state->options,
+                                       state->flags, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* We need to write out any unwritten data if we reopen read-only. */
     if ((state->flags & BDRV_O_RDWR) == 0) {
         ret = bdrv_flush(state->bs);
         if (ret < 0) {
-            return ret;
+            goto fail;
         }
 
         ret = qcow2_mark_clean(state->bs);
         if (ret < 0) {
-            return ret;
+            goto fail;
         }
     }
 
     return 0;
+
+fail:
+    qcow2_update_options_abort(state->bs, r);
+    return ret;
+}
+
+static void qcow2_reopen_commit(BDRVReopenState *state)
+{
+    qcow2_update_options_commit(state->bs, state->opaque);
+    g_free(state->opaque);
+}
+
+static void qcow2_reopen_abort(BDRVReopenState *state)
+{
+    qcow2_update_options_abort(state->bs, state->opaque);
+    g_free(state->opaque);
 }
 
 static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
@@ -3000,6 +3056,8 @@  BlockDriver bdrv_qcow2 = {
     .bdrv_open          = qcow2_open,
     .bdrv_close         = qcow2_close,
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
+    .bdrv_reopen_commit   = qcow2_reopen_commit,
+    .bdrv_reopen_abort    = qcow2_reopen_abort,
     .bdrv_create        = qcow2_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_get_block_status = qcow2_co_get_block_status,