[Qemu-devel] [PATCH V2 0/2] qcow2: try cancel the modification on fail in snapshot creation

[Qemu-devel] [PATCH V2 0/2] qcow2: try cancel the modification on fail in snapshot creation

[Wenchao Xia]

V2:
  1/2: all fail case will goto fail section.
  2/2: add the goto code.

Wenchao Xia (2):
  1 qcow2: free allocated cluster on fail in qcow2_write_snapshots()
  2 qcow2: cancel the modification on fail in qcow2_snapshot_create()

 block/qcow2-snapshot.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

[Qemu-devel] [PATCH V2 1/2] qcow2: free allocated cluster on fail in qcow2_write_snapshots()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 992a5c8..45da32d 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -180,13 +180,13 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
     /* Allocate space for the new snapshot list */
     snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
-    offset = snapshots_offset;
-    if (offset < 0) {
-        return offset;
+    ret = offset = snapshots_offset;
+    if (ret < 0) {
+        goto fail;
     }
     ret = bdrv_flush(bs);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
 
     /* Write all snapshots to the new list */
@@ -268,6 +268,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     return 0;
 
 fail:
+    /* free the new snapshot table */
+    qcow2_free_clusters(bs, snapshots_offset, snapshots_size);
     return ret;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH V2 2/2] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 45da32d..033f705 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -367,7 +367,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
                       s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
-        goto fail;
+        goto dealloc_cluster;
     }
 
     g_free(l1_table);
@@ -380,7 +380,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
     if (ret < 0) {
-        goto fail;
+        goto dealloc_cluster;
     }
 
     /* Append the new snapshot to the snapshot list */
@@ -397,7 +397,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     if (ret < 0) {
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
-        goto fail;
+        goto restore_refcount;
     }
 
     g_free(old_snapshot_list);
@@ -410,6 +410,17 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 #endif
     return 0;
 
+restore_refcount:
+    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
+                                         s->l1_size, -1);
+    if (ret < 0) {
+        /* Nothing can be done none now, need image check later */
+        error_report("qcow2: Error in restoring refcount in snapshot");
+    }
+
+dealloc_cluster:
+    qcow2_free_clusters(bs, sn->l1_table_offset, sn->l1_size);
+
 fail:
     g_free(sn->id_str);
     g_free(sn->name);
-- 
1.7.1

Re: [Qemu-devel] [PATCH V2 1/2] qcow2: free allocated cluster on fail in qcow2_write_snapshots()

[Stefan Hajnoczi]

On Fri, May 17, 2013 at 03:56:44PM +0800, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 992a5c8..45da32d 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -180,13 +180,13 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>  
>      /* Allocate space for the new snapshot list */
>      snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
> -    offset = snapshots_offset;
> -    if (offset < 0) {
> -        return offset;
> +    ret = offset = snapshots_offset;
> +    if (ret < 0) {
> +        goto fail;
>      }

If qcow2_alloc_clusters() failed then we must not call
qcow2_free_clusters().

Re: [Qemu-devel] [PATCH V2 2/2] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Stefan Hajnoczi]

On Fri, May 17, 2013 at 03:56:45PM +0800, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 45da32d..033f705 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -367,7 +367,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>      ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
>                        s->l1_size * sizeof(uint64_t));
>      if (ret < 0) {
> -        goto fail;
> +        goto dealloc_cluster;
>      }
>  
>      g_free(l1_table);
> @@ -380,7 +380,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>       */
>      ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
>      if (ret < 0) {
> -        goto fail;
> +        goto dealloc_cluster;
>      }
>  
>      /* Append the new snapshot to the snapshot list */
> @@ -397,7 +397,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>      if (ret < 0) {
>          g_free(s->snapshots);
>          s->snapshots = old_snapshot_list;
> -        goto fail;
> +        goto restore_refcount;
>      }
>  
>      g_free(old_snapshot_list);
> @@ -410,6 +410,17 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>  #endif
>      return 0;
>  
> +restore_refcount:
> +    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> +                                         s->l1_size, -1);
> +    if (ret < 0) {
> +        /* Nothing can be done none now, need image check later */
> +        error_report("qcow2: Error in restoring refcount in snapshot");
> +    }
> +
> +dealloc_cluster:
> +    qcow2_free_clusters(bs, sn->l1_table_offset, sn->l1_size);

sn->l1_size is in uint64_t units, not in bytes.

Do you have test cases to exercise these code paths?  Use blkdebug to
inject errors at the right point so that the error code path is taken.

Re: [Qemu-devel] [PATCH V2 2/2] qcow2: cancel the modification on fail in qcow2_snapshot_create()

[Wenchao Xia]

于 2013-5-17 17:27, Stefan Hajnoczi 写道:
> On Fri, May 17, 2013 at 03:56:45PM +0800, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qcow2-snapshot.c |   17 ++++++++++++++---
>>   1 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 45da32d..033f705 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -367,7 +367,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>       ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
>>                         s->l1_size * sizeof(uint64_t));
>>       if (ret < 0) {
>> -        goto fail;
>> +        goto dealloc_cluster;
>>       }
>>
>>       g_free(l1_table);
>> @@ -380,7 +380,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>        */
>>       ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
>>       if (ret < 0) {
>> -        goto fail;
>> +        goto dealloc_cluster;
>>       }
>>
>>       /* Append the new snapshot to the snapshot list */
>> @@ -397,7 +397,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>       if (ret < 0) {
>>           g_free(s->snapshots);
>>           s->snapshots = old_snapshot_list;
>> -        goto fail;
>> +        goto restore_refcount;
>>       }
>>
>>       g_free(old_snapshot_list);
>> @@ -410,6 +410,17 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>   #endif
>>       return 0;
>>
>> +restore_refcount:
>> +    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
>> +                                         s->l1_size, -1);
>> +    if (ret < 0) {
>> +        /* Nothing can be done none now, need image check later */
>> +        error_report("qcow2: Error in restoring refcount in snapshot");
>> +    }
>> +
>> +dealloc_cluster:
>> +    qcow2_free_clusters(bs, sn->l1_table_offset, sn->l1_size);
>
> sn->l1_size is in uint64_t units, not in bytes.
>
> Do you have test cases to exercise these code paths?  Use blkdebug to
> inject errors at the right point so that the error code path is taken.
>
   OK, I'll try it.

-- 
Best Regards

Wenchao Xia