[Qemu-devel] [PATCH 0/3] qcow2: Small error path fixes for l2_allocate

[Qemu-devel] [PATCH 0/3] qcow2: Small error path fixes for l2_allocate

[Max Reitz]

Errors in l2_allocate should always go down the error path. If this path
is taken, the newly allocated L2 cluster is abandoned and should thus be
freed. The L2 table on the other hand should only be put back into the
cache if it was taken from it before.

Max Reitz (3):
  qcow2: Don't put invalid L2 table into cache
  qcow2: Free allocated L2 cluster on error
  qcow2: Always use error path in l2_allocate

 block/qcow2-cluster.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] qcow2: Don't put invalid L2 table into cache

[Max Reitz]

In l2_allocate, the fail path is executed if qcow2_cache_flush fails.
However, the L2 table has not yet been fetched from the L2 table cache.
The qcow2_cache_put in the fail path therefore basically gives an
undefined argument as the L2 table address (in this case).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 738ff73..f6d47c9 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -188,7 +188,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t old_l2_offset;
-    uint64_t *l2_table;
+    uint64_t *l2_table = NULL;
     int64_t l2_offset;
     int ret;
 
@@ -265,7 +265,9 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
 
 fail:
     trace_qcow2_l2_allocate_done(bs, l1_index, ret);
-    qcow2_cache_put(bs, s->l2_table_cache, (void**) table);
+    if (l2_table != NULL) {
+        qcow2_cache_put(bs, s->l2_table_cache, (void**) table);
+    }
     s->l1_table[l1_index] = old_l2_offset;
     return ret;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/3] qcow2: Don't put invalid L2 table into cache

[Benoît Canet]

Le Wednesday 25 Sep 2013 à 16:37:18 (+0200), Max Reitz a écrit :
> In l2_allocate, the fail path is executed if qcow2_cache_flush fails.
> However, the L2 table has not yet been fetched from the L2 table cache.
> The qcow2_cache_put in the fail path therefore basically gives an
> undefined argument as the L2 table address (in this case).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 738ff73..f6d47c9 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -188,7 +188,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t old_l2_offset;
> -    uint64_t *l2_table;
> +    uint64_t *l2_table = NULL;
>      int64_t l2_offset;
>      int ret;
>  
> @@ -265,7 +265,9 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
>  
>  fail:
>      trace_qcow2_l2_allocate_done(bs, l1_index, ret);
> -    qcow2_cache_put(bs, s->l2_table_cache, (void**) table);
> +    if (l2_table != NULL) {
> +        qcow2_cache_put(bs, s->l2_table_cache, (void**) table);
> +    }
>      s->l1_table[l1_index] = old_l2_offset;
>      return ret;
>  }
> -- 
> 1.8.3.1
> 
> 

Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH 2/3] qcow2: Free allocated L2 cluster on error

[Max Reitz]

If an error occurs in l2_allocate, the allocated (but unused) L2 cluster
should be freed.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f6d47c9..1c3d3fc 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -269,6 +269,10 @@ fail:
         qcow2_cache_put(bs, s->l2_table_cache, (void**) table);
     }
     s->l1_table[l1_index] = old_l2_offset;
+    if (l2_offset > 0) {
+        qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t),
+                            QCOW2_DISCARD_ALWAYS);
+    }
     return ret;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/3] qcow2: Free allocated L2 cluster on error

[Benoît Canet]

Le Wednesday 25 Sep 2013 à 16:37:19 (+0200), Max Reitz a écrit :
> If an error occurs in l2_allocate, the allocated (but unused) L2 cluster
> should be freed.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f6d47c9..1c3d3fc 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -269,6 +269,10 @@ fail:
>          qcow2_cache_put(bs, s->l2_table_cache, (void**) table);
>      }
>      s->l1_table[l1_index] = old_l2_offset;
> +    if (l2_offset > 0) {
> +        qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t),
> +                            QCOW2_DISCARD_ALWAYS);
> +    }
>      return ret;
>  }
>  
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 2/3] qcow2: Free allocated L2 cluster on error

[Kevin Wolf]

Am 25.09.2013 um 16:37 hat Max Reitz geschrieben:
> If an error occurs in l2_allocate, the allocated (but unused) L2 cluster
> should be freed.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 4 ++++
>  1 file changed, 4 insertions(+)

This needs an update of the reference output for test case 026 (both for
-nocache and writethrough).

Most of the changes look expected and good, like cluster leaks
disappearing. With -nocache, however, there are a few cases that failed
previously and result in successful writes now. It would be interesting
to see the explanation for these before we merge the patch.

Kevin

Re: [Qemu-devel] [PATCH 2/3] qcow2: Free allocated L2 cluster on error

[Max Reitz]

On 2013-09-27 16:54, Kevin Wolf wrote:
> Am 25.09.2013 um 16:37 hat Max Reitz geschrieben:
>> If an error occurs in l2_allocate, the allocated (but unused) L2 cluster
>> should be freed.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-cluster.c | 4 ++++
>>   1 file changed, 4 insertions(+)
> This needs an update of the reference output for test case 026 (both for
> -nocache and writethrough).
Yes, right.

> Most of the changes look expected and good, like cluster leaks
> disappearing. With -nocache, however, there are a few cases that failed
> previously and result in successful writes now. It would be interesting
> to see the explanation for these before we merge the patch.
I personally don't see this cases. Could you give an example?

Max

Re: [Qemu-devel] [PATCH 2/3] qcow2: Free allocated L2 cluster on error

[Kevin Wolf]

Am 30.09.2013 um 11:48 hat Max Reitz geschrieben:
> On 2013-09-27 16:54, Kevin Wolf wrote:
> >Am 25.09.2013 um 16:37 hat Max Reitz geschrieben:
> >>If an error occurs in l2_allocate, the allocated (but unused) L2 cluster
> >>should be freed.
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>  block/qcow2-cluster.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >This needs an update of the reference output for test case 026 (both for
> >-nocache and writethrough).
> Yes, right.
> 
> >Most of the changes look expected and good, like cluster leaks
> >disappearing. With -nocache, however, there are a few cases that failed
> >previously and result in successful writes now. It would be interesting
> >to see the explanation for these before we merge the patch.
> I personally don't see this cases. Could you give an example?

Weird, I can't reproduce it any more. I guess it was a problem on my side
then.

Kevin

[Qemu-devel] [PATCH 3/3] qcow2: Always use error path in l2_allocate

[Max Reitz]

Just returning -errno in some cases prevents
trace_qcow2_l2_allocate_done from being executed (and, in one case, also
the unused allocated L2 table from being freed). Always going down the
error path fixes this.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1c3d3fc..c442f6c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -200,7 +200,8 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
 
     l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
     if (l2_offset < 0) {
-        return l2_offset;
+        ret = l2_offset;
+        goto fail;
     }
 
     ret = qcow2_cache_flush(bs, s->refcount_block_cache);
@@ -213,7 +214,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
     trace_qcow2_l2_allocate_get_empty(bs, l1_index);
     ret = qcow2_cache_get_empty(bs, s->l2_table_cache, l2_offset, (void**) table);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
 
     l2_table = *table;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/3] qcow2: Always use error path in l2_allocate

[Benoît Canet]

Le Wednesday 25 Sep 2013 à 16:37:20 (+0200), Max Reitz a écrit :
> Just returning -errno in some cases prevents
> trace_qcow2_l2_allocate_done from being executed (and, in one case, also
> the unused allocated L2 table from being freed). Always going down the
> error path fixes this.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 1c3d3fc..c442f6c 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -200,7 +200,8 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
>  
>      l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
>      if (l2_offset < 0) {
> -        return l2_offset;
> +        ret = l2_offset;
> +        goto fail;
>      }
>  
>      ret = qcow2_cache_flush(bs, s->refcount_block_cache);
> @@ -213,7 +214,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
>      trace_qcow2_l2_allocate_get_empty(bs, l1_index);
>      ret = qcow2_cache_get_empty(bs, s->l2_table_cache, l2_offset, (void**) table);
>      if (ret < 0) {
> -        return ret;
> +        goto fail;
>      }
>  
>      l2_table = *table;
> -- 
> 1.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 0/3] qcow2: Small error path fixes for l2_allocate

[Kevin Wolf]

Am 25.09.2013 um 16:37 hat Max Reitz geschrieben:
> Errors in l2_allocate should always go down the error path. If this path
> is taken, the newly allocated L2 cluster is abandoned and should thus be
> freed. The L2 table on the other hand should only be put back into the
> cache if it was taken from it before.
> 
> Max Reitz (3):
>   qcow2: Don't put invalid L2 table into cache
>   qcow2: Free allocated L2 cluster on error
>   qcow2: Always use error path in l2_allocate
> 
>  block/qcow2-cluster.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

Thanks, applied all to the block branch.

Kevin