Patchwork [RFC,v2,13/23] qcow2: handle_copied(): Implement non-zero host_offset

login
register
mail settings
Submitter Kevin Wolf
Date Feb. 13, 2013, 1:22 p.m.
Message ID <1360761733-25347-14-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/220166/
State New
Headers show

Comments

Kevin Wolf - Feb. 13, 2013, 1:22 p.m.
Look only for clusters that start at a given physical offset.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)
Blue Swirl - Feb. 13, 2013, 9:17 p.m.
On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Look only for clusters that start at a given physical offset.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-cluster.c |   26 ++++++++++++++++++--------
>  1 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 5ce2c88..90fe36c 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>   *          the length of the area that can be written to.
>   *
>   *  -errno: in error cases
> - *
> - * TODO Make non-zero host_offset behave like describe above
>   */
>  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>      uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>
>      trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset,
>                                *bytes);
> -    assert(*host_offset == 0);
>
>      /*
>       * Calculate the number of clusters to look for. We stop at L2 table
> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>      if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
>          && (cluster_offset & QCOW_OFLAG_COPIED))
>      {
> +        /* If a specific host_offset is required, check it */
> +        if (*host_offset != 0
> +            && (cluster_offset & L2E_OFFSET_MASK) != *host_offset)
> +        {

Braces should cuddle with the previous line.

> +            *bytes = 0;
> +            ret = 0;
> +            goto out;
> +        }
> +
>          /* We keep all QCOW_OFLAG_COPIED clusters */
>          keep_clusters =
>              count_contiguous_clusters(nb_clusters, s->cluster_size,
> @@ -880,19 +886,22 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>
>          ret = 1;
>      } else {
> -        cluster_offset = 0;
>          ret = 0;
>      }
>
> -    cluster_offset &= L2E_OFFSET_MASK;
> -    *host_offset = cluster_offset;
> -
>      /* Cleanup */
> +out:
>      pret = qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
>      if (pret < 0) {
>          return pret;
>      }
>
> +    /* Only return a host offset if we actually made progress. Otherwise we
> +     * would make requirements for handle_alloc() that it can't fulfill */
> +    if (ret) {
> +        *host_offset = cluster_offset & L2E_OFFSET_MASK;
> +    }
> +
>      return ret;
>  }
>
> @@ -1162,7 +1171,6 @@ again:
>
>      /*
>       * 2. Count contiguous COPIED clusters.
> -     *    TODO: Consider cluster_offset if set in step 1c.
>       */
>      ret = handle_copied(bs, offset, &cluster_offset, &cur_bytes, m);
>      if (ret < 0) {
> @@ -1175,6 +1183,8 @@ again:
>          if (!*host_offset) {
>              *host_offset = cluster_offset;
>          }
> +    } else if (cur_bytes == 0) {
> +        goto done;
>      } else {
>          keep_clusters = 0;
>      }
> --
> 1.7.6.5
>
>
Kevin Wolf - Feb. 14, 2013, 9:40 a.m.
Am 13.02.2013 22:17, schrieb Blue Swirl:
> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Look only for clusters that start at a given physical offset.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/qcow2-cluster.c |   26 ++++++++++++++++++--------
>>  1 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 5ce2c88..90fe36c 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>>   *          the length of the area that can be written to.
>>   *
>>   *  -errno: in error cases
>> - *
>> - * TODO Make non-zero host_offset behave like describe above
>>   */
>>  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>      uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>
>>      trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset,
>>                                *bytes);
>> -    assert(*host_offset == 0);
>>
>>      /*
>>       * Calculate the number of clusters to look for. We stop at L2 table
>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>      if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
>>          && (cluster_offset & QCOW_OFLAG_COPIED))
>>      {
>> +        /* If a specific host_offset is required, check it */
>> +        if (*host_offset != 0
>> +            && (cluster_offset & L2E_OFFSET_MASK) != *host_offset)
>> +        {
> 
> Braces should cuddle with the previous line.

Can we get rid of this rule for multiline ifs? Having the
second/third/... line of the condition and the first line of the then
branch with no clear separation severely hurts readability in my opinion.

> 
>> +            *bytes = 0;
>> +            ret = 0;
>> +            goto out;
>> +        }
>> +
>>          /* We keep all QCOW_OFLAG_COPIED clusters */
>>          keep_clusters =
>>              count_contiguous_clusters(nb_clusters, s->cluster_size,

Kevin
Blue Swirl - Feb. 14, 2013, 9:40 p.m.
On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.02.2013 22:17, schrieb Blue Swirl:
>> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Look only for clusters that start at a given physical offset.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block/qcow2-cluster.c |   26 ++++++++++++++++++--------
>>>  1 files changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index 5ce2c88..90fe36c 100644
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>>>   *          the length of the area that can be written to.
>>>   *
>>>   *  -errno: in error cases
>>> - *
>>> - * TODO Make non-zero host_offset behave like describe above
>>>   */
>>>  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>>      uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
>>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>>
>>>      trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset,
>>>                                *bytes);
>>> -    assert(*host_offset == 0);
>>>
>>>      /*
>>>       * Calculate the number of clusters to look for. We stop at L2 table
>>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>>      if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
>>>          && (cluster_offset & QCOW_OFLAG_COPIED))
>>>      {
>>> +        /* If a specific host_offset is required, check it */
>>> +        if (*host_offset != 0
>>> +            && (cluster_offset & L2E_OFFSET_MASK) != *host_offset)
>>> +        {
>>
>> Braces should cuddle with the previous line.
>
> Can we get rid of this rule for multiline ifs? Having the
> second/third/... line of the condition and the first line of the then
> branch with no clear separation severely hurts readability in my opinion.

Perhaps the problem is that the condition is long, it could be
rewritten in this style:
bool has_host_offset = *host_offset != 0;
bool offset_matches = (cluster_offset & L2E_OFFSET_MASK) != *host_offset;
if (has_host_offset && offset_matches) {

>
>>
>>> +            *bytes = 0;
>>> +            ret = 0;
>>> +            goto out;
>>> +        }
>>> +
>>>          /* We keep all QCOW_OFLAG_COPIED clusters */
>>>          keep_clusters =
>>>              count_contiguous_clusters(nb_clusters, s->cluster_size,
>
> Kevin
Kevin Wolf - Feb. 15, 2013, 9:35 a.m.
On Thu, Feb 14, 2013 at 09:40:22PM +0000, Blue Swirl wrote:
> On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 13.02.2013 22:17, schrieb Blue Swirl:
> >> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> >>> Look only for clusters that start at a given physical offset.
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> ---
> >>>  block/qcow2-cluster.c |   26 ++++++++++++++++++--------
> >>>  1 files changed, 18 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> >>> index 5ce2c88..90fe36c 100644
> >>> --- a/block/qcow2-cluster.c
> >>> +++ b/block/qcow2-cluster.c
> >>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
> >>>   *          the length of the area that can be written to.
> >>>   *
> >>>   *  -errno: in error cases
> >>> - *
> >>> - * TODO Make non-zero host_offset behave like describe above
> >>>   */
> >>>  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
> >>>      uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
> >>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
> >>>
> >>>      trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset,
> >>>                                *bytes);
> >>> -    assert(*host_offset == 0);
> >>>
> >>>      /*
> >>>       * Calculate the number of clusters to look for. We stop at L2 table
> >>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
> >>>      if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
> >>>          && (cluster_offset & QCOW_OFLAG_COPIED))
> >>>      {
> >>> +        /* If a specific host_offset is required, check it */
> >>> +        if (*host_offset != 0
> >>> +            && (cluster_offset & L2E_OFFSET_MASK) != *host_offset)
> >>> +        {
> >>
> >> Braces should cuddle with the previous line.
> >
> > Can we get rid of this rule for multiline ifs? Having the
> > second/third/... line of the condition and the first line of the then
> > branch with no clear separation severely hurts readability in my opinion.
> 
> Perhaps the problem is that the condition is long, it could be
> rewritten in this style:
> bool has_host_offset = *host_offset != 0;
> bool offset_matches = (cluster_offset & L2E_OFFSET_MASK) != *host_offset;
> if (has_host_offset && offset_matches) {

I consider the usefulness of this about the same as adding code in order to
silence gcc warnings. Just that a complaining gcc breaks the build whereas a
complaining Blue Swirl doesn't. But I'll do it here.

Kevin
Blue Swirl - Feb. 16, 2013, 11:18 a.m.
On Fri, Feb 15, 2013 at 9:35 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> On Thu, Feb 14, 2013 at 09:40:22PM +0000, Blue Swirl wrote:
>> On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Am 13.02.2013 22:17, schrieb Blue Swirl:
>> >> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> >>> Look only for clusters that start at a given physical offset.
>> >>>
>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> >>> ---
>> >>>  block/qcow2-cluster.c |   26 ++++++++++++++++++--------
>> >>>  1 files changed, 18 insertions(+), 8 deletions(-)
>> >>>
>> >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> >>> index 5ce2c88..90fe36c 100644
>> >>> --- a/block/qcow2-cluster.c
>> >>> +++ b/block/qcow2-cluster.c
>> >>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>> >>>   *          the length of the area that can be written to.
>> >>>   *
>> >>>   *  -errno: in error cases
>> >>> - *
>> >>> - * TODO Make non-zero host_offset behave like describe above
>> >>>   */
>> >>>  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>> >>>      uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
>> >>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>> >>>
>> >>>      trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset,
>> >>>                                *bytes);
>> >>> -    assert(*host_offset == 0);
>> >>>
>> >>>      /*
>> >>>       * Calculate the number of clusters to look for. We stop at L2 table
>> >>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>> >>>      if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
>> >>>          && (cluster_offset & QCOW_OFLAG_COPIED))
>> >>>      {
>> >>> +        /* If a specific host_offset is required, check it */
>> >>> +        if (*host_offset != 0
>> >>> +            && (cluster_offset & L2E_OFFSET_MASK) != *host_offset)
>> >>> +        {
>> >>
>> >> Braces should cuddle with the previous line.
>> >
>> > Can we get rid of this rule for multiline ifs? Having the
>> > second/third/... line of the condition and the first line of the then
>> > branch with no clear separation severely hurts readability in my opinion.
>>
>> Perhaps the problem is that the condition is long, it could be
>> rewritten in this style:
>> bool has_host_offset = *host_offset != 0;
>> bool offset_matches = (cluster_offset & L2E_OFFSET_MASK) != *host_offset;
>> if (has_host_offset && offset_matches) {
>
> I consider the usefulness of this about the same as adding code in order to
> silence gcc warnings. Just that a complaining gcc breaks the build whereas a
> complaining Blue Swirl doesn't. But I'll do it here.

Maybe I complain too much. I think the booleans could actually make
the logic clearer to someone not familiar with the code.

>
> Kevin

Patch

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5ce2c88..90fe36c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -827,8 +827,6 @@  static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
  *          the length of the area that can be written to.
  *
  *  -errno: in error cases
- *
- * TODO Make non-zero host_offset behave like describe above
  */
 static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
     uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
@@ -843,7 +841,6 @@  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
 
     trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset,
                               *bytes);
-    assert(*host_offset == 0);
 
     /*
      * Calculate the number of clusters to look for. We stop at L2 table
@@ -867,6 +864,15 @@  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
     if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
         && (cluster_offset & QCOW_OFLAG_COPIED))
     {
+        /* If a specific host_offset is required, check it */
+        if (*host_offset != 0
+            && (cluster_offset & L2E_OFFSET_MASK) != *host_offset)
+        {
+            *bytes = 0;
+            ret = 0;
+            goto out;
+        }
+
         /* We keep all QCOW_OFLAG_COPIED clusters */
         keep_clusters =
             count_contiguous_clusters(nb_clusters, s->cluster_size,
@@ -880,19 +886,22 @@  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
 
         ret = 1;
     } else {
-        cluster_offset = 0;
         ret = 0;
     }
 
-    cluster_offset &= L2E_OFFSET_MASK;
-    *host_offset = cluster_offset;
-
     /* Cleanup */
+out:
     pret = qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
     if (pret < 0) {
         return pret;
     }
 
+    /* Only return a host offset if we actually made progress. Otherwise we
+     * would make requirements for handle_alloc() that it can't fulfill */
+    if (ret) {
+        *host_offset = cluster_offset & L2E_OFFSET_MASK;
+    }
+
     return ret;
 }
 
@@ -1162,7 +1171,6 @@  again:
 
     /*
      * 2. Count contiguous COPIED clusters.
-     *    TODO: Consider cluster_offset if set in step 1c.
      */
     ret = handle_copied(bs, offset, &cluster_offset, &cur_bytes, m);
     if (ret < 0) {
@@ -1175,6 +1183,8 @@  again:
         if (!*host_offset) {
             *host_offset = cluster_offset;
         }
+    } else if (cur_bytes == 0) {
+        goto done;
     } else {
         keep_clusters = 0;
     }