Patchwork [V9,5/8] qcow2: full rollback on fail in qcow2_write_snapshots()

login
register
mail settings
Submitter Wayne Xia
Date Jan. 5, 2014, 7:43 p.m.
Message ID <1388950991-30105-6-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/307090/
State New
Headers show

Comments

Wayne Xia - Jan. 5, 2014, 7:43 p.m.
Header restore step is added, and old code section "fail" is
renamed to "dealloc_sn_table" to tip better, now "fail" section
does not rollback anything on disk. When one step in rollback
fails, following steps will be skipped, to make sure no dangling
pointer is left.

A new parameter "*errp_rollback" is added to tell caller the result
of rollback procedure.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |   63 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 47 insertions(+), 16 deletions(-)
Max Reitz - Jan. 11, 2014, 11:35 p.m.
On 05.01.2014 20:43, Wenchao Xia wrote:
> Header restore step is added, and old code section "fail" is
> renamed to "dealloc_sn_table" to tip better, now "fail" section
> does not rollback anything on disk. When one step in rollback
> fails, following steps will be skipped, to make sure no dangling
> pointer is left.

How about:

A header restore step is added and the old label "fail" is
renamed to the more verbose "dealloc_sn_table", whereas the new "fail" 
section
does not rollback anything on disk. If any step during the rollback
fails, all remaining will be skipped to prevent dangling pointers.

> A new parameter "*errp_rollback" is added to tell caller the result

"the caller"

> of rollback procedure.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>   block/qcow2-snapshot.c |   63 +++++++++++++++++++++++++++++++++++------------
>   1 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 5619fc3..5cac714 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -152,7 +152,9 @@ fail:
>   }
>   
>   /* add at the end of the file a new list of snapshots */
> -static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
> +static int qcow2_write_snapshots(BlockDriverState *bs,
> +                                 Error **errp,
> +                                 Error **errp_rollback)
>   {
>       BDRVQcowState *s = bs->opaque;
>       QCowSnapshot *sn;
> @@ -162,9 +164,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>       struct {
>           uint32_t nb_snapshots;
>           uint64_t snapshots_offset;
> -    } QEMU_PACKED header_data;
> +    } QEMU_PACKED header_data, header_data_old;
>       int64_t offset, snapshots_offset;
> -    int ret;
> +    int ret, ret0;
>   
>       /* compute the size of the snapshots */
>       offset = 0;
> @@ -192,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>           error_setg_errno(errp, -ret,
>                            "Failed in flush after snapshot list cluster "
>                            "allocation");
> -        goto fail;
> +        goto dealloc_sn_table;
>       }
>   
>       /* The snapshot list position has not yet been updated, so these clusters
> @@ -203,7 +205,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                            "Failed in overlap check for snapshot list cluster "
>                            "at %" PRIi64 " with size %d",
>                            offset, snapshots_size);
> -        goto fail;
> +        goto dealloc_sn_table;
>       }
>   
>   
> @@ -240,7 +242,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of snapshot header at %"
>                                PRIi64 " with size %zu",
>                                offset, sizeof(h));
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += sizeof(h);
>   
> @@ -250,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of extra snapshot data at %"
>                                PRIi64 " with size %zu",
>                                offset, sizeof(extra));
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += sizeof(extra);
>   
> @@ -260,7 +262,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of snapshot id string at %"
>                                PRIi64 " with size %d",
>                                offset, id_str_size);
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += id_str_size;
>   
> @@ -270,7 +272,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of snapshot name string at %"
>                                PRIi64 " with size %d",
>                                offset, name_size);
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += name_size;
>       }
> @@ -283,7 +285,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>       if (ret < 0) {
>           error_setg_errno(errp, -ret,
>                            "Failed in flush after snapshot list update");
> -        goto fail;
> +        goto dealloc_sn_table;
> +    }
> +
> +    /* Start the qcow2 header update */
> +    ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
> +                     &header_data_old, sizeof(header_data_old));
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "Failed in read of image header at %zu with size %zu",

I'd prefer "Failed to read image header at %zu with size %zu".

> +                         offsetof(QCowHeader, nb_snapshots),
> +                         sizeof(header_data_old));
> +        goto dealloc_sn_table;
>       }
>   
>       QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
> @@ -299,7 +312,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                            "Failed in update of image header at %zu with size %zu",
>                            offsetof(QCowHeader, nb_snapshots),
>                            sizeof(header_data));
> -        goto fail;
> +        goto restore_header;
>       }
>   
>       /* free the old snapshot table */
> @@ -309,11 +322,29 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>       s->snapshots_size = snapshots_size;
>       return 0;
>   
> -fail:
> +restore_header:
> +    ret0 = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
> +                            &header_data_old, sizeof(header_data_old));
> +    if (ret0 < 0) {
> +        error_setg_errno(errp_rollback, -ret0,
> +                         "In rollback failed to restore image header at %zu "
> +                         "with size %zu",

I think changing the order (and "in" to "during") to "Failed to restore 
image header at %zu with size %zu during rollback" sounds better.

> +                         offsetof(QCowHeader, nb_snapshots),
> +                         sizeof(header_data));
> +        goto fail;
> +    }
> +
> +dealloc_sn_table:
>       if (snapshots_offset > 0) {
> -        qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
> -                            QCOW2_DISCARD_ALWAYS);
> +        ret0 = qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
> +                                   QCOW2_DISCARD_ALWAYS);
> +        if (ret0 < 0) {
> +            error_setg_errno(errp_rollback, -ret0,
> +                             "In rollback failed to free snapshot table");

Again, I'd prefer the order "Failed to free snapshot table in/during 
rollback".

> +        }
>       }
> +
> +fail:
>       if (errp) {
>           g_assert(error_is_set(errp));
>       }
> @@ -481,7 +512,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>       s->snapshots = new_snapshot_list;
>       s->snapshots[s->nb_snapshots++] = *sn;
>   
> -    ret = qcow2_write_snapshots(bs, errp);
> +    ret = qcow2_write_snapshots(bs, errp, NULL);
>       if (ret < 0) {
>           g_free(s->snapshots);
>           s->snapshots = old_snapshot_list;
> @@ -656,7 +687,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
>               s->snapshots + snapshot_index + 1,
>               (s->nb_snapshots - snapshot_index - 1) * sizeof(sn));
>       s->nb_snapshots--;
> -    ret = qcow2_write_snapshots(bs, errp);
> +    ret = qcow2_write_snapshots(bs, errp, NULL);
>       if (ret < 0) {
>           return ret;
>       }


Aside from these:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Patch

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5619fc3..5cac714 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -152,7 +152,9 @@  fail:
 }
 
 /* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
+static int qcow2_write_snapshots(BlockDriverState *bs,
+                                 Error **errp,
+                                 Error **errp_rollback)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *sn;
@@ -162,9 +164,9 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     struct {
         uint32_t nb_snapshots;
         uint64_t snapshots_offset;
-    } QEMU_PACKED header_data;
+    } QEMU_PACKED header_data, header_data_old;
     int64_t offset, snapshots_offset;
-    int ret;
+    int ret, ret0;
 
     /* compute the size of the snapshots */
     offset = 0;
@@ -192,7 +194,7 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret,
                          "Failed in flush after snapshot list cluster "
                          "allocation");
-        goto fail;
+        goto dealloc_sn_table;
     }
 
     /* The snapshot list position has not yet been updated, so these clusters
@@ -203,7 +205,7 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                          "Failed in overlap check for snapshot list cluster "
                          "at %" PRIi64 " with size %d",
                          offset, snapshots_size);
-        goto fail;
+        goto dealloc_sn_table;
     }
 
 
@@ -240,7 +242,7 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot header at %"
                              PRIi64 " with size %zu",
                              offset, sizeof(h));
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += sizeof(h);
 
@@ -250,7 +252,7 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of extra snapshot data at %"
                              PRIi64 " with size %zu",
                              offset, sizeof(extra));
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += sizeof(extra);
 
@@ -260,7 +262,7 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot id string at %"
                              PRIi64 " with size %d",
                              offset, id_str_size);
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += id_str_size;
 
@@ -270,7 +272,7 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                              "Failed in write of snapshot name string at %"
                              PRIi64 " with size %d",
                              offset, name_size);
-            goto fail;
+            goto dealloc_sn_table;
         }
         offset += name_size;
     }
@@ -283,7 +285,18 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     if (ret < 0) {
         error_setg_errno(errp, -ret,
                          "Failed in flush after snapshot list update");
-        goto fail;
+        goto dealloc_sn_table;
+    }
+
+    /* Start the qcow2 header update */
+    ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
+                     &header_data_old, sizeof(header_data_old));
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed in read of image header at %zu with size %zu",
+                         offsetof(QCowHeader, nb_snapshots),
+                         sizeof(header_data_old));
+        goto dealloc_sn_table;
     }
 
     QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
@@ -299,7 +312,7 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
                          "Failed in update of image header at %zu with size %zu",
                          offsetof(QCowHeader, nb_snapshots),
                          sizeof(header_data));
-        goto fail;
+        goto restore_header;
     }
 
     /* free the old snapshot table */
@@ -309,11 +322,29 @@  static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
     s->snapshots_size = snapshots_size;
     return 0;
 
-fail:
+restore_header:
+    ret0 = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
+                            &header_data_old, sizeof(header_data_old));
+    if (ret0 < 0) {
+        error_setg_errno(errp_rollback, -ret0,
+                         "In rollback failed to restore image header at %zu "
+                         "with size %zu",
+                         offsetof(QCowHeader, nb_snapshots),
+                         sizeof(header_data));
+        goto fail;
+    }
+
+dealloc_sn_table:
     if (snapshots_offset > 0) {
-        qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
-                            QCOW2_DISCARD_ALWAYS);
+        ret0 = qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
+                                   QCOW2_DISCARD_ALWAYS);
+        if (ret0 < 0) {
+            error_setg_errno(errp_rollback, -ret0,
+                             "In rollback failed to free snapshot table");
+        }
     }
+
+fail:
     if (errp) {
         g_assert(error_is_set(errp));
     }
@@ -481,7 +512,7 @@  void qcow2_snapshot_create(BlockDriverState *bs,
     s->snapshots = new_snapshot_list;
     s->snapshots[s->nb_snapshots++] = *sn;
 
-    ret = qcow2_write_snapshots(bs, errp);
+    ret = qcow2_write_snapshots(bs, errp, NULL);
     if (ret < 0) {
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
@@ -656,7 +687,7 @@  int qcow2_snapshot_delete(BlockDriverState *bs,
             s->snapshots + snapshot_index + 1,
             (s->nb_snapshots - snapshot_index - 1) * sizeof(sn));
     s->nb_snapshots--;
-    ret = qcow2_write_snapshots(bs, errp);
+    ret = qcow2_write_snapshots(bs, errp, NULL);
     if (ret < 0) {
         return ret;
     }