[Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Anthony Liguori]

Revision: 5860
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
Author:   aliguori
Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)

Log Message:
-----------
Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

Move duplicated code into helper functions.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/block-qcow2.c

Modified: trunk/block-qcow2.c
===================================================================
--- trunk/block-qcow2.c	2008-12-02 20:10:14 UTC (rev 5859)
+++ trunk/block-qcow2.c	2008-12-02 20:11:27 UTC (rev 5860)
@@ -601,6 +601,34 @@
     return l2_table;
 }
 
+static int size_to_clusters(BDRVQcowState *s, int64_t size)
+{
+    return (size + (s->cluster_size - 1)) >> s->cluster_bits;
+}
+
+static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
+        uint64_t *l2_table, uint64_t mask)
+{
+    int i;
+    uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
+
+    for (i = 0; i < nb_clusters; i++)
+        if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) & ~mask))
+            break;
+
+	return i;
+}
+
+static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t *l2_table)
+{
+    int i = 0;
+
+    while(nb_clusters-- && l2_table[i] == 0)
+        i++;
+
+    return i;
+}
+
 /*
  * get_cluster_offset
  *
@@ -622,9 +650,9 @@
 {
     BDRVQcowState *s = bs->opaque;
     int l1_index, l2_index;
-    uint64_t l2_offset, *l2_table, cluster_offset, next;
-    int l1_bits;
-    int index_in_cluster, nb_available, nb_needed;
+    uint64_t l2_offset, *l2_table, cluster_offset;
+    int l1_bits, c;
+    int index_in_cluster, nb_available, nb_needed, nb_clusters;
 
     index_in_cluster = (offset >> 9) & (s->cluster_sectors - 1);
     nb_needed = *num + index_in_cluster;
@@ -632,7 +660,7 @@
     l1_bits = s->l2_bits + s->cluster_bits;
 
     /* compute how many bytes there are between the offset and
-     * and the end of the l1 entry
+     * the end of the l1 entry
      */
 
     nb_available = (1 << l1_bits) - (offset & ((1 << l1_bits) - 1));
@@ -667,38 +695,25 @@
 
     l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
     cluster_offset = be64_to_cpu(l2_table[l2_index]);
-    nb_available = s->cluster_sectors;
-    l2_index++;
+    nb_clusters = size_to_clusters(s, nb_needed << 9);
 
     if (!cluster_offset) {
-
-       /* how many empty clusters ? */
-
-       while (nb_available < nb_needed && !l2_table[l2_index]) {
-           l2_index++;
-           nb_available += s->cluster_sectors;
-       }
+        /* how many empty clusters ? */
+        c = count_contiguous_free_clusters(nb_clusters, &l2_table[l2_index]);
     } else {
+        /* how many allocated clusters ? */
+        c = count_contiguous_clusters(nb_clusters, s->cluster_size,
+                &l2_table[l2_index], QCOW_OFLAG_COPIED);
+    }
 
-       /* how many allocated clusters ? */
-
-       cluster_offset &= ~QCOW_OFLAG_COPIED;
-       while (nb_available < nb_needed) {
-           next = be64_to_cpu(l2_table[l2_index]) & ~QCOW_OFLAG_COPIED;
-           if (next != cluster_offset + (nb_available << 9))
-               break;
-           l2_index++;
-           nb_available += s->cluster_sectors;
-       }
-   }
-
+   nb_available = (c * s->cluster_sectors);
 out:
     if (nb_available > nb_needed)
         nb_available = nb_needed;
 
     *num = nb_available - index_in_cluster;
 
-    return cluster_offset;
+    return cluster_offset & ~QCOW_OFLAG_COPIED;
 }
 
 /*
@@ -862,15 +877,15 @@
     BDRVQcowState *s = bs->opaque;
     int l2_index, ret;
     uint64_t l2_offset, *l2_table, cluster_offset;
-    int nb_available, nb_clusters, i, j;
-    uint64_t start_sect, current;
+    int nb_available, nb_clusters, i = 0;
+    uint64_t start_sect;
 
     ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
     if (ret == 0)
         return 0;
 
-    nb_clusters = ((n_end << 9) + s->cluster_size - 1) >>
-                  s->cluster_bits;
+    nb_clusters = size_to_clusters(s, n_end << 9);
+
     if (nb_clusters > s->l2_size - l2_index)
             nb_clusters = s->l2_size - l2_index;
 
@@ -879,14 +894,9 @@
     /* We keep all QCOW_OFLAG_COPIED clusters */
 
     if (cluster_offset & QCOW_OFLAG_COPIED) {
+        nb_clusters = count_contiguous_clusters(nb_clusters, s->cluster_size,
+                &l2_table[l2_index], 0);
 
-        for (i = 1; i < nb_clusters; i++) {
-            current = be64_to_cpu(l2_table[l2_index + i]);
-            if (cluster_offset + (i << s->cluster_bits) != current)
-                break;
-        }
-        nb_clusters = i;
-
         nb_available = nb_clusters << (s->cluster_bits - 9);
         if (nb_available > n_end)
             nb_available = n_end;
@@ -903,46 +913,27 @@
 
     /* how many available clusters ? */
 
-    i = 0;
     while (i < nb_clusters) {
+        int j;
+        i += count_contiguous_free_clusters(nb_clusters - i,
+                &l2_table[l2_index + i]);
 
-        i++;
+        cluster_offset = be64_to_cpu(l2_table[l2_index + i]);
 
-        if (!cluster_offset) {
-
-            /* how many free clusters ? */
-
-            while (i < nb_clusters) {
-                cluster_offset = be64_to_cpu(l2_table[l2_index + i]);
-                if (cluster_offset != 0)
-                    break;
-                i++;
-            }
-
-            if ((cluster_offset & QCOW_OFLAG_COPIED) ||
+        if ((cluster_offset & QCOW_OFLAG_COPIED) ||
                 (cluster_offset & QCOW_OFLAG_COMPRESSED))
-                break;
+            break;
 
-        } else {
+        j = count_contiguous_clusters(nb_clusters - i, s->cluster_size,
+                &l2_table[l2_index + i], 0);
 
-            /* how many contiguous clusters ? */
+        if (j)
+            free_any_clusters(bs, cluster_offset, j);
 
-            j = 1;
-            current = 0;
-            while (i < nb_clusters) {
-                current = be64_to_cpu(l2_table[l2_index + i]);
-                if (cluster_offset + (j << s->cluster_bits) != current)
-                    break;
+        i += j;
 
-                i++;
-                j++;
-            }
-
-            free_any_clusters(bs, cluster_offset, j);
-            if (current)
-                break;
-            cluster_offset = current;
-        }
+        if(be64_to_cpu(l2_table[l2_index + i]))
+            break;
     }
     nb_clusters = i;
 
@@ -2194,26 +2185,19 @@
     BDRVQcowState *s = bs->opaque;
     int i, nb_clusters;
 
-    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
-    for(;;) {
-        if (get_refcount(bs, s->free_cluster_index) == 0) {
-            s->free_cluster_index++;
-            for(i = 1; i < nb_clusters; i++) {
-                if (get_refcount(bs, s->free_cluster_index) != 0)
-                    goto not_found;
-                s->free_cluster_index++;
-            }
+    nb_clusters = size_to_clusters(s, size);
+retry:
+    for(i = 0; i < nb_clusters; i++) {
+        int64_t i = s->free_cluster_index++;
+        if (get_refcount(bs, i) != 0)
+            goto retry;
+    }
 #ifdef DEBUG_ALLOC2
-            printf("alloc_clusters: size=%lld -> %lld\n",
-                   size,
-                   (s->free_cluster_index - nb_clusters) << s->cluster_bits);
+    printf("alloc_clusters: size=%lld -> %lld\n",
+            size,
+            (s->free_cluster_index - nb_clusters) << s->cluster_bits);
 #endif
-            return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
-        } else {
-        not_found:
-            s->free_cluster_index++;
-        }
-    }
+    return (s->free_cluster_index - nb_clusters) << s->cluster_bits;
 }
 
 static int64_t alloc_clusters(BlockDriverState *bs, int64_t size)
@@ -2548,7 +2532,7 @@
     uint16_t *refcount_table;
 
     size = bdrv_getlength(s->hd);
-    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
+    nb_clusters = size_to_clusters(s, size);
     refcount_table = qemu_mallocz(nb_clusters * sizeof(uint16_t));
 
     /* header */
@@ -2600,7 +2584,7 @@
     int refcount;
 
     size = bdrv_getlength(s->hd);
-    nb_clusters = (size + s->cluster_size - 1) >> s->cluster_bits;
+    nb_clusters = size_to_clusters(s, size);
     for(k = 0; k < nb_clusters;) {
         k1 = k;
         refcount = get_refcount(bs, k);

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Anthony Liguori]

Hi Gleb,

Anthony Liguori wrote:
> Revision: 5860
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
> Author:   aliguori
> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>   

According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this 
changeset breaks compressed qcow2 on write and savevm.

Can you please look into this?

Regards,

Anthony Liguori

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Gleb Natapov]

On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
> Hi Gleb,
>
> Anthony Liguori wrote:
>> Revision: 5860
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>> Author:   aliguori
>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>   
>
> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this  
> changeset breaks compressed qcow2 on write and savevm.
>
> Can you please look into this?
>
I will.

--
			Gleb.

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Gleb Natapov]

On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
> Hi Gleb,
>
> Anthony Liguori wrote:
>> Revision: 5860
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>> Author:   aliguori
>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>   
>
> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this  
> changeset breaks compressed qcow2 on write and savevm.
>
> Can you please look into this?
>
Alexander, can you try the patch below?


diff --git a/block-qcow2.c b/block-qcow2.c
index b3b5f8f..d8ac647 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -620,6 +620,9 @@ static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
     int i;
     uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
 
+    if (!offset)
+        return 0;
+
     for (i = 0; i < nb_clusters; i++)
         if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) & ~mask))
             break;
@@ -981,6 +984,12 @@ static uint64_t alloc_cluster_offset(BlockDriverState *bs,
     /* how many available clusters ? */
 
     while (i < nb_clusters) {
+        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
+                &l2_table[l2_index + i], 0);
+
+        if(be64_to_cpu(l2_table[l2_index + i]))
+            break;
+
         i += count_contiguous_free_clusters(nb_clusters - i,
                 &l2_table[l2_index + i]);
 
@@ -989,12 +998,6 @@ static uint64_t alloc_cluster_offset(BlockDriverState *bs,
         if ((cluster_offset & QCOW_OFLAG_COPIED) ||
                 (cluster_offset & QCOW_OFLAG_COMPRESSED))
             break;
-
-        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
-                &l2_table[l2_index + i], 0);
-
-        if(be64_to_cpu(l2_table[l2_index + i]))
-            break;
     }
     nb_clusters = i;
 
--
			Gleb.

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Alexander Graf]

Gleb,

do you have a Novell bugzilla account, so I can add you to the bug?  
Kevin apparently used some of his spare time (he's on vacation) to  
look at this as well.

Alex

On 11.12.2008, at 12:00, Gleb Natapov wrote:

> On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
>> Hi Gleb,
>>
>> Anthony Liguori wrote:
>>> Revision: 5860
>>>          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>>> Author:   aliguori
>>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>>
>>
>> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this
>> changeset breaks compressed qcow2 on write and savevm.
>>
>> Can you please look into this?
>>
> Alexander, can you try the patch below?
>
>
> diff --git a/block-qcow2.c b/block-qcow2.c
> index b3b5f8f..d8ac647 100644
> --- a/block-qcow2.c
> +++ b/block-qcow2.c
> @@ -620,6 +620,9 @@ static int count_contiguous_clusters(uint64_t  
> nb_clusters, int cluster_size,
>     int i;
>     uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
>
> +    if (!offset)
> +        return 0;
> +
>     for (i = 0; i < nb_clusters; i++)
>         if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) &  
> ~mask))
>             break;
> @@ -981,6 +984,12 @@ static uint64_t  
> alloc_cluster_offset(BlockDriverState *bs,
>     /* how many available clusters ? */
>
>     while (i < nb_clusters) {
> +        i += count_contiguous_clusters(nb_clusters - i, s- 
> >cluster_size,
> +                &l2_table[l2_index + i], 0);
> +
> +        if(be64_to_cpu(l2_table[l2_index + i]))
> +            break;
> +
>         i += count_contiguous_free_clusters(nb_clusters - i,
>                 &l2_table[l2_index + i]);
>
> @@ -989,12 +998,6 @@ static uint64_t  
> alloc_cluster_offset(BlockDriverState *bs,
>         if ((cluster_offset & QCOW_OFLAG_COPIED) ||
>                 (cluster_offset & QCOW_OFLAG_COMPRESSED))
>             break;
> -
> -        i += count_contiguous_clusters(nb_clusters - i, s- 
> >cluster_size,
> -                &l2_table[l2_index + i], 0);
> -
> -        if(be64_to_cpu(l2_table[l2_index + i]))
> -            break;
>     }
>     nb_clusters = i;
>
> --
> 			Gleb.

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Gleb Natapov]

On Thu, Dec 11, 2008 at 02:21:44PM +0100, Alexander Graf wrote:
> Gleb,
>
> do you have a Novell bugzilla account, so I can add you to the bug?  
Just opened one: glebn.

> Kevin apparently used some of his spare time (he's on vacation) to look 
> at this as well.
>
> Alex
>
> On 11.12.2008, at 12:00, Gleb Natapov wrote:
>
>> On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
>>> Hi Gleb,
>>>
>>> Anthony Liguori wrote:
>>>> Revision: 5860
>>>>          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>>>> Author:   aliguori
>>>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>>>
>>>
>>> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this
>>> changeset breaks compressed qcow2 on write and savevm.
>>>
>>> Can you please look into this?
>>>
>> Alexander, can you try the patch below?
>>
>>
>> diff --git a/block-qcow2.c b/block-qcow2.c
>> index b3b5f8f..d8ac647 100644
>> --- a/block-qcow2.c
>> +++ b/block-qcow2.c
>> @@ -620,6 +620,9 @@ static int count_contiguous_clusters(uint64_t  
>> nb_clusters, int cluster_size,
>>     int i;
>>     uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
>>
>> +    if (!offset)
>> +        return 0;
>> +
>>     for (i = 0; i < nb_clusters; i++)
>>         if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) &  
>> ~mask))
>>             break;
>> @@ -981,6 +984,12 @@ static uint64_t  
>> alloc_cluster_offset(BlockDriverState *bs,
>>     /* how many available clusters ? */
>>
>>     while (i < nb_clusters) {
>> +        i += count_contiguous_clusters(nb_clusters - i, s- 
>> >cluster_size,
>> +                &l2_table[l2_index + i], 0);
>> +
>> +        if(be64_to_cpu(l2_table[l2_index + i]))
>> +            break;
>> +
>>         i += count_contiguous_free_clusters(nb_clusters - i,
>>                 &l2_table[l2_index + i]);
>>
>> @@ -989,12 +998,6 @@ static uint64_t  
>> alloc_cluster_offset(BlockDriverState *bs,
>>         if ((cluster_offset & QCOW_OFLAG_COPIED) ||
>>                 (cluster_offset & QCOW_OFLAG_COMPRESSED))
>>             break;
>> -
>> -        i += count_contiguous_clusters(nb_clusters - i, s- 
>> >cluster_size,
>> -                &l2_table[l2_index + i], 0);
>> -
>> -        if(be64_to_cpu(l2_table[l2_index + i]))
>> -            break;
>>     }
>>     nb_clusters = i;
>>
>> --
>> 			Gleb.

--
			Gleb.

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Alexander Graf]

Gleb Natapov wrote:
> On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
>   
>> Hi Gleb,
>>
>> Anthony Liguori wrote:
>>     
>>> Revision: 5860
>>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>>> Author:   aliguori
>>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>>   
>>>       
>> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this  
>> changeset breaks compressed qcow2 on write and savevm.
>>
>> Can you please look into this?
>>
>>     
> Alexander, can you try the patch below?
>   

The patch seems to work just fine. Please also take a look at Kevin's
and see which one fits best. Since I'm not really into qcow2 code, I
can't tell which one  is the more accurate one.

Thanks a lot for looking so quickly into this!

Alex

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Gleb Natapov]

On Thu, Dec 11, 2008 at 04:16:04PM +0100, Alexander Graf wrote:
> Gleb Natapov wrote:
> > On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
> >   
> >> Hi Gleb,
> >>
> >> Anthony Liguori wrote:
> >>     
> >>> Revision: 5860
> >>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
> >>> Author:   aliguori
> >>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
> >>>   
> >>>       
> >> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this  
> >> changeset breaks compressed qcow2 on write and savevm.
> >>
> >> Can you please look into this?
> >>
> >>     
> > Alexander, can you try the patch below?
> >   
> 
> The patch seems to work just fine. Please also take a look at Kevin's
> and see which one fits best. Since I'm not really into qcow2 code, I
> can't tell which one  is the more accurate one.
> 
> Thanks a lot for looking so quickly into this!
> 
I considered initially something like Kevin solution. I think mine
fix is better for code readability reasons.

--
			Gleb.

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Kevin Wolf]

Am Do 11 Dez 2008 12:00:56 CET schrieb Gleb Natapov <gleb@redhat.com>:

> On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
>> Hi Gleb,
>>
>> Anthony Liguori wrote:
>>> Revision: 5860
>>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>>> Author:   aliguori
>>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>>
>>
>> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this
>> changeset breaks compressed qcow2 on write and savevm.
>>
>> Can you please look into this?
>>
> Alexander, can you try the patch below?

Hm, no signed-off line yet. Anyway:

Acked-by: Kevin Wolf <kwolf@suse.de>

>
>
> diff --git a/block-qcow2.c b/block-qcow2.c
> index b3b5f8f..d8ac647 100644
> --- a/block-qcow2.c
> +++ b/block-qcow2.c
> @@ -620,6 +620,9 @@ static int count_contiguous_clusters(uint64_t   
> nb_clusters, int cluster_size,
>      int i;
>      uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
>
> +    if (!offset)
> +        return 0;
> +
>      for (i = 0; i < nb_clusters; i++)
>          if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) & ~mask))
>              break;
> @@ -981,6 +984,12 @@ static uint64_t   
> alloc_cluster_offset(BlockDriverState *bs,
>      /* how many available clusters ? */
>
>      while (i < nb_clusters) {
> +        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
> +                &l2_table[l2_index + i], 0);
> +
> +        if(be64_to_cpu(l2_table[l2_index + i]))
> +            break;
> +
>          i += count_contiguous_free_clusters(nb_clusters - i,
>                  &l2_table[l2_index + i]);
>
> @@ -989,12 +998,6 @@ static uint64_t   
> alloc_cluster_offset(BlockDriverState *bs,
>          if ((cluster_offset & QCOW_OFLAG_COPIED) ||
>                  (cluster_offset & QCOW_OFLAG_COMPRESSED))
>              break;
> -
> -        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
> -                &l2_table[l2_index + i], 0);
> -
> -        if(be64_to_cpu(l2_table[l2_index + i]))
> -            break;
>      }
>      nb_clusters = i;
>
> --
> 			Gleb.
>
>
>

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Gleb Natapov]

On Thu, Dec 11, 2008 at 06:32:58PM +0100, Kevin Wolf wrote:
> Am Do 11 Dez 2008 12:00:56 CET schrieb Gleb Natapov <gleb@redhat.com>:
>
>> On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
>>> Hi Gleb,
>>>
>>> Anthony Liguori wrote:
>>>> Revision: 5860
>>>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>>>> Author:   aliguori
>>>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>>>
>>>
>>> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this
>>> changeset breaks compressed qcow2 on write and savevm.
>>>
>>> Can you please look into this?
>>>
>> Alexander, can you try the patch below?
>
> Hm, no signed-off line yet. Anyway:
>
Here it is ;)

Signed-off-by: Gleb Natapov <gleb@redhat.com>

> Acked-by: Kevin Wolf <kwolf@suse.de>
>
>>
>>
>> diff --git a/block-qcow2.c b/block-qcow2.c
>> index b3b5f8f..d8ac647 100644
>> --- a/block-qcow2.c
>> +++ b/block-qcow2.c
>> @@ -620,6 +620,9 @@ static int count_contiguous_clusters(uint64_t   
>> nb_clusters, int cluster_size,
>>      int i;
>>      uint64_t offset = be64_to_cpu(l2_table[0]) & ~mask;
>>
>> +    if (!offset)
>> +        return 0;
>> +
>>      for (i = 0; i < nb_clusters; i++)
>>          if (offset + i * cluster_size != (be64_to_cpu(l2_table[i]) & ~mask))
>>              break;
>> @@ -981,6 +984,12 @@ static uint64_t   
>> alloc_cluster_offset(BlockDriverState *bs,
>>      /* how many available clusters ? */
>>
>>      while (i < nb_clusters) {
>> +        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
>> +                &l2_table[l2_index + i], 0);
>> +
>> +        if(be64_to_cpu(l2_table[l2_index + i]))
>> +            break;
>> +
>>          i += count_contiguous_free_clusters(nb_clusters - i,
>>                  &l2_table[l2_index + i]);
>>
>> @@ -989,12 +998,6 @@ static uint64_t   
>> alloc_cluster_offset(BlockDriverState *bs,
>>          if ((cluster_offset & QCOW_OFLAG_COPIED) ||
>>                  (cluster_offset & QCOW_OFLAG_COMPRESSED))
>>              break;
>> -
>> -        i += count_contiguous_clusters(nb_clusters - i, s->cluster_size,
>> -                &l2_table[l2_index + i], 0);
>> -
>> -        if(be64_to_cpu(l2_table[l2_index + i]))
>> -            break;
>>      }
>>      nb_clusters = i;
>>
>> --
>> 			Gleb.
>>
>>
>>
>

--
			Gleb.

Re: [Qemu-devel] [5860] Cleanup {alloc|get}_cluster_offset() (Gleb Natapov)

[Kevin Wolf]

Kevin Wolf schrieb:
> Am Do 11 Dez 2008 12:00:56 CET schrieb Gleb Natapov <gleb@redhat.com>:
> 
>> On Wed, Dec 10, 2008 at 08:38:19AM -0600, Anthony Liguori wrote:
>>> Hi Gleb,
>>>
>>> Anthony Liguori wrote:
>>>> Revision: 5860
>>>>          
>>>> http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5860
>>>> Author:   aliguori
>>>> Date:     2008-12-02 20:11:27 +0000 (Tue, 02 Dec 2008)
>>>>
>>>
>>> According to https://bugzilla.novell.com/show_bug.cgi?id=457957, this
>>> changeset breaks compressed qcow2 on write and savevm.
>>>
>>> Can you please look into this?
>>>
>> Alexander, can you try the patch below?
> 
> Hm, no signed-off line yet. Anyway:
> 
> Acked-by: Kevin Wolf <kwolf@suse.de>

Anthony, care to apply Gleb's patch? It doesn't seem to be committed yet.

Kevin