[Qemu-devel] [PATCH 0/6] qcow2_co_write_zeroes and related improvements

[Qemu-devel] [PATCH 0/6] qcow2_co_write_zeroes and related improvements

[Denis V. Lunev]

This series enables tracepoints in qemu-io and improves
bdrv/qcow2_co_write_zeroes framework.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>

Denis V. Lunev (6):
  qcow2: add tracepoints for qcow2_co_write_zeroes
  qemu-io: enable tracing in qemu-io
  block: split write_zeroes always
  qcow2: simplify logic in qcow2_co_write_zeroes
  qcow2: fix condition in is_zero_cluster
  qcow2: merge is_zero_cluster helpers into qcow2_co_write_zeroes

 block/io.c    |  6 +++---
 block/qcow2.c | 63 ++++++++++++++++++++++-------------------------------------
 qemu-io.c     | 43 ++++++++++++++++++++++++++++++++++++++++
 trace-events  |  2 ++
 4 files changed, 71 insertions(+), 43 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/6] qemu-io: enable tracing in qemu-io

[Denis V. Lunev]

It would be convinient to enable tracepoints in qemu-io binary. This would
allow to perform investigations without additional code recompilations.

The command line will be exactly the same as in qemu-system.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-io.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/qemu-io.c b/qemu-io.c
index 5ef3ef7..2d0d2b0 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -18,6 +18,7 @@
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qemu/readline.h"
+#include "qemu/log.h"
 #include "qapi/qmp/qstring.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
@@ -419,6 +420,25 @@ static QemuOptsList qemu_object_opts = {
     },
 };
 
+static QemuOptsList qemu_trace_opts = {
+    .name = "trace",
+    .implied_opt_name = "enable",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_trace_opts.head),
+    .desc = {
+        {
+            .name = "enable",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "events",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
 
 static QemuOptsList file_opts = {
     .name = "file",
@@ -473,6 +493,7 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QOM);
     qemu_add_opts(&qemu_object_opts);
+    qemu_add_opts(&qemu_trace_opts);
     bdrv_init();
 
     while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
@@ -512,10 +533,32 @@ int main(int argc, char **argv)
             }
             break;
         case 'T':
+        {
+            QemuOpts *trace_opts;
+            char *trace_file;
+
+            trace_opts = qemu_opts_parse_noisily(qemu_find_opts("trace"),
+                                                 optarg, true);
+            if (trace_opts == NULL) {
+                exit(1);
+            }
+            if (qemu_opt_get(trace_opts, "enable")) {
+                trace_enable_events(qemu_opt_get(trace_opts, "enable"));
+            }
+            trace_init_events(qemu_opt_get(trace_opts, "events"));
+
+            trace_file = g_strdup(qemu_opt_get(trace_opts, "file"));
+            qemu_opts_del(trace_opts);
+
             if (!trace_init_backends()) {
                 exit(1); /* error message will have been printed */
             }
+            trace_init_file(trace_file);
+            g_free(trace_file);
+
+            qemu_set_log(LOG_TRACE);
             break;
+        }
         case 'V':
             printf("%s version %s\n", progname, QEMU_VERSION);
             exit(0);
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/6] qemu-io: enable tracing in qemu-io

[Paolo Bonzini]

On 14/05/2016 14:01, Denis V. Lunev wrote:
> It would be convinient to enable tracepoints in qemu-io binary. This would
> allow to perform investigations without additional code recompilations.
> 
> The command line will be exactly the same as in qemu-system.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-io.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/qemu-io.c b/qemu-io.c
> index 5ef3ef7..2d0d2b0 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -18,6 +18,7 @@
>  #include "qemu/option.h"
>  #include "qemu/config-file.h"
>  #include "qemu/readline.h"
> +#include "qemu/log.h"

Why does this need qemu/log.h, as opposed to trace/control.h (which
probably should be moved to include/qemu/trace.h, but that's a patch for
another day)?

>  #include "qapi/qmp/qstring.h"
>  #include "qom/object_interfaces.h"
>  #include "sysemu/block-backend.h"
> @@ -419,6 +420,25 @@ static QemuOptsList qemu_object_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_trace_opts = {
> +    .name = "trace",
> +    .implied_opt_name = "enable",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_trace_opts.head),
> +    .desc = {
> +        {
> +            .name = "enable",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "events",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "file",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};

Stefan, should this be in trace/control.c instead, so that vl.c and
qemu-io.c can share it?

Otherwise looks sane.

Thanks,

Paolo

>  static QemuOptsList file_opts = {
>      .name = "file",
> @@ -473,6 +493,7 @@ int main(int argc, char **argv)
>  
>      module_call_init(MODULE_INIT_QOM);
>      qemu_add_opts(&qemu_object_opts);
> +    qemu_add_opts(&qemu_trace_opts);
>      bdrv_init();
>  
>      while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
> @@ -512,10 +533,32 @@ int main(int argc, char **argv)
>              }
>              break;
>          case 'T':
> +        {
> +            QemuOpts *trace_opts;
> +            char *trace_file;
> +
> +            trace_opts = qemu_opts_parse_noisily(qemu_find_opts("trace"),
> +                                                 optarg, true);
> +            if (trace_opts == NULL) {
> +                exit(1);
> +            }
> +            if (qemu_opt_get(trace_opts, "enable")) {
> +                trace_enable_events(qemu_opt_get(trace_opts, "enable"));
> +            }
> +            trace_init_events(qemu_opt_get(trace_opts, "events"));
> +
> +            trace_file = g_strdup(qemu_opt_get(trace_opts, "file"));
> +            qemu_opts_del(trace_opts);
> +
>              if (!trace_init_backends()) {
>                  exit(1); /* error message will have been printed */
>              }
> +            trace_init_file(trace_file);
> +            g_free(trace_file);
> +
> +            qemu_set_log(LOG_TRACE);
>              break;
> +        }
>          case 'V':
>              printf("%s version %s\n", progname, QEMU_VERSION);
>              exit(0);
> 

Re: [Qemu-devel] [PATCH 1/6] qemu-io: enable tracing in qemu-io

[Denis V. Lunev]

On 05/16/2016 04:09 PM, Paolo Bonzini wrote:
>
> On 14/05/2016 14:01, Denis V. Lunev wrote:
>> It would be convinient to enable tracepoints in qemu-io binary. This would
>> allow to perform investigations without additional code recompilations.
>>
>> The command line will be exactly the same as in qemu-system.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   qemu-io.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>
>> diff --git a/qemu-io.c b/qemu-io.c
>> index 5ef3ef7..2d0d2b0 100644
>> --- a/qemu-io.c
>> +++ b/qemu-io.c
>> @@ -18,6 +18,7 @@
>>   #include "qemu/option.h"
>>   #include "qemu/config-file.h"
>>   #include "qemu/readline.h"
>> +#include "qemu/log.h"
> Why does this need qemu/log.h, as opposed to trace/control.h (which
> probably should be moved to include/qemu/trace.h, but that's a patch for
> another day)?
no, this is necessary for  qemu_set_log(LOG_TRACE); which is performed 
as a last
step to enable tracing.

Re: [Qemu-devel] [PATCH 1/6] qemu-io: enable tracing in qemu-io

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

On 05/14/2016 06:01 AM, Denis V. Lunev wrote:
> It would be convinient to enable tracepoints in qemu-io binary. This would

s/convinient/convenient/

> allow to perform investigations without additional code recompilations.
> 
> The command line will be exactly the same as in qemu-system.

Reads awkwardly.  Grammar suggestion for the whole commit body:

For convenience, enable tracepoints in the qemu-io binary just like what
is done for qemu-system.  This allows trace investigations without
additional code recompilation.

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-io.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/6] block: split write_zeroes always

[Denis V. Lunev]

We should split requests even if they are less than write_zeroes_alignment.
For example we can have the following request:
  offset 62k
  size   4k
  write_zeroes_alignment 64k
Original code will send 1 request covering 2 qcow2 clusters. One cluster
could be zeroed as a whole, another could not be. In this case we will have
both 2 clusters allocated.

After the patch 2 requests to qcow2 layer will be sent and thus only one
cluster will be allocated.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index cd6d71a..2d6f48f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1172,13 +1172,13 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
         /* Align request.  Block drivers can expect the "bulk" of the request
          * to be aligned.
          */
-        if (bs->bl.write_zeroes_alignment
-            && num > bs->bl.write_zeroes_alignment) {
+        if (bs->bl.write_zeroes_alignment) {
             if (sector_num % bs->bl.write_zeroes_alignment != 0) {
                 /* Make a small request up to the first aligned sector.  */
                 num = bs->bl.write_zeroes_alignment;
                 num -= sector_num % bs->bl.write_zeroes_alignment;
-            } else if ((sector_num + num) % bs->bl.write_zeroes_alignment != 0) {
+            } else if (num > bs->bl.write_zeroes_alignment &&
+                    (sector_num + num) % bs->bl.write_zeroes_alignment != 0) {
                 /* Shorten the request to the last aligned sector.  num cannot
                  * underflow because num > bs->bl.write_zeroes_alignment.
                  */
-- 
2.5.0

Re: [Qemu-devel] [PATCH 2/6] block: split write_zeroes always

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On 05/14/2016 06:01 AM, Denis V. Lunev wrote:
> We should split requests even if they are less than write_zeroes_alignment.
> For example we can have the following request:
>   offset 62k
>   size   4k
>   write_zeroes_alignment 64k
> Original code will send 1 request covering 2 qcow2 clusters. One cluster
> could be zeroed as a whole, another could not be. In this case we will have
> both 2 clusters allocated.
> 
> After the patch 2 requests to qcow2 layer will be sent and thus only one
> cluster will be allocated.

Grammar suggestion:

The original code sent 1 request covering 2 qcow2 clusters, and resulted
in both clusters being allocated. But by splitting the request, we can
cater to the case where one of the two clusters can be zeroed as a
whole, for only 1 cluster allocated after the operation.

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

>                  num -= sector_num % bs->bl.write_zeroes_alignment;
> -            } else if ((sector_num + num) % bs->bl.write_zeroes_alignment != 0) {
> +            } else if (num > bs->bl.write_zeroes_alignment &&
> +                    (sector_num + num) % bs->bl.write_zeroes_alignment != 0) {

Alignment looks off. Also, if it were me, I'd write 'a % b' rather than
'a % b != 0'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 3/6] qcow2: simplify logic in qcow2_co_write_zeroes

[Denis V. Lunev]

Unaligned request could occupy only one cluster. This is true since the
previous commit. Simplify the code taking this considiration into
account.

There are no other changes so far.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 62febfc..9a54bbd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2436,33 +2436,20 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
     int tail = (sector_num + nb_sectors) % s->cluster_sectors;
 
     if (head != 0 || tail != 0) {
-        int64_t cl_end = -1;
+        int64_t cl_start = sector_num - head;
 
-        sector_num -= head;
-        nb_sectors += head;
+        assert(cl_start + s->cluster_sectors >= sector_num + nb_sectors);
 
-        if (tail != 0) {
-            nb_sectors += s->cluster_sectors - tail;
-        }
+        sector_num = cl_start;
+        nb_sectors = s->cluster_sectors;
 
         if (!is_zero_cluster(bs, sector_num)) {
             return -ENOTSUP;
         }
 
-        if (nb_sectors > s->cluster_sectors) {
-            /* Technically the request can cover 2 clusters, f.e. 4k write
-               at s->cluster_sectors - 2k offset. One of these cluster can
-               be zeroed, one unallocated */
-            cl_end = sector_num + nb_sectors - s->cluster_sectors;
-            if (!is_zero_cluster(bs, cl_end)) {
-                return -ENOTSUP;
-            }
-        }
-
         qemu_co_mutex_lock(&s->lock);
         /* We can have new write after previous check */
-        if (!is_zero_cluster_top_locked(bs, sector_num) ||
-                (cl_end > 0 && !is_zero_cluster_top_locked(bs, cl_end))) {
+        if (!is_zero_cluster_top_locked(bs, sector_num)) {
             qemu_co_mutex_unlock(&s->lock);
             return -ENOTSUP;
         }
-- 
2.5.0

Re: [Qemu-devel] [PATCH 3/6] qcow2: simplify logic in qcow2_co_write_zeroes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

On 05/14/2016 06:01 AM, Denis V. Lunev wrote:
> Unaligned request could occupy only one cluster. This is true since the

s/request could/requests will/

> previous commit. Simplify the code taking this considiration into

s/considiration/consideration/

> account.
> 
> There are no other changes so far.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 62febfc..9a54bbd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2436,33 +2436,20 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
>      int tail = (sector_num + nb_sectors) % s->cluster_sectors;
>  
>      if (head != 0 || tail != 0) {
> -        int64_t cl_end = -1;
> +        int64_t cl_start = sector_num - head;
>  
> -        sector_num -= head;
> -        nb_sectors += head;
> +        assert(cl_start + s->cluster_sectors >= sector_num + nb_sectors);

In other words, the caller is now buggy if it ever passes us an
unaligned request that crosses cluster boundaries (the only requests
that can cross boundaries will be aligned).

With the grammar fix in the commit message,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 4/6] qcow2: add tracepoints for qcow2_co_write_zeroes

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 5 +++++
 trace-events  | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 9a54bbd..97bf870 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2435,6 +2435,9 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
     int head = sector_num % s->cluster_sectors;
     int tail = (sector_num + nb_sectors) % s->cluster_sectors;
 
+    trace_qcow2_write_zeroes_start_req(qemu_coroutine_self(), sector_num,
+                                       nb_sectors);
+
     if (head != 0 || tail != 0) {
         int64_t cl_start = sector_num - head;
 
@@ -2457,6 +2460,8 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
         qemu_co_mutex_lock(&s->lock);
     }
 
+    trace_qcow2_write_zeroes(qemu_coroutine_self(), sector_num, nb_sectors);
+
     /* Whatever is left can use real zero clusters */
     ret = qcow2_zero_clusters(bs, sector_num << BDRV_SECTOR_BITS, nb_sectors);
     qemu_co_mutex_unlock(&s->lock);
diff --git a/trace-events b/trace-events
index 4fce005..627f34f 100644
--- a/trace-events
+++ b/trace-events
@@ -612,6 +612,8 @@ qcow2_writev_done_req(void *co, int ret) "co %p ret %d"
 qcow2_writev_start_part(void *co) "co %p"
 qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d"
 qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64
+qcow2_write_zeroes_start_req(void *co, int64_t sector, int nb_sectors) "co %p sector %" PRIx64 " nb_sectors %d"
+qcow2_write_zeroes(void *co, int64_t sector, int nb_sectors) "co %p sector %" PRIx64 " nb_sectors %d"
 
 # block/qcow2-cluster.c
 qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offset %" PRIx64 " num %d"
-- 
2.5.0

Re: [Qemu-devel] [PATCH 4/6] qcow2: add tracepoints for qcow2_co_write_zeroes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On 05/14/2016 06:01 AM, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 5 +++++
>  trace-events  | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 9a54bbd..97bf870 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2435,6 +2435,9 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
>      int head = sector_num % s->cluster_sectors;
>      int tail = (sector_num + nb_sectors) % s->cluster_sectors;
>  
> +    trace_qcow2_write_zeroes_start_req(qemu_coroutine_self(), sector_num,
> +                                       nb_sectors);
> +

Can we trace these by byte locations rather than by sector count?
Ultimately, I think we want to move write_zeroes to a byte-based
interface, even if we still assert internally that it is at least
sector-aligned.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/6] qcow2: add tracepoints for qcow2_co_write_zeroes

[Denis V. Lunev]

On 05/16/2016 07:32 PM, Eric Blake wrote:
> On 05/14/2016 06:01 AM, Denis V. Lunev wrote:
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/qcow2.c | 5 +++++
>>   trace-events  | 2 ++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 9a54bbd..97bf870 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2435,6 +2435,9 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
>>       int head = sector_num % s->cluster_sectors;
>>       int tail = (sector_num + nb_sectors) % s->cluster_sectors;
>>   
>> +    trace_qcow2_write_zeroes_start_req(qemu_coroutine_self(), sector_num,
>> +                                       nb_sectors);
>> +
> Can we trace these by byte locations rather than by sector count?
> Ultimately, I think we want to move write_zeroes to a byte-based
> interface, even if we still assert internally that it is at least
> sector-aligned.
>
we can, but this patches traces for qcow2_co_writev

static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
                            int64_t sector_num,
                            int remaining_sectors,
                            QEMUIOVector *qiov)
{
     [...]
     trace_qcow2_writev_start_req(qemu_coroutine_self(), sector_num,
                                  remaining_sectors);

Thus I'd better either stick to this implementation or change
all tracepoints in QEMU code.

[Qemu-devel] [PATCH 5/6] qcow2: fix condition in is_zero_cluster

[Denis V. Lunev]

We should check for (res & BDRV_BLOCK_ZERO) only. The situation when we
will have !(res & BDRV_BLOCK_DATA) and will not have BDRV_BLOCK_ZERO is
not possible.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 97bf870..05beb64 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2412,7 +2412,7 @@ static bool is_zero_cluster(BlockDriverState *bs, int64_t start)
     BlockDriverState *file;
     int64_t res = bdrv_get_block_status_above(bs, NULL, start,
                                               s->cluster_sectors, &nr, &file);
-    return res >= 0 && ((res & BDRV_BLOCK_ZERO) || !(res & BDRV_BLOCK_DATA));
+    return res >= 0 && (res & BDRV_BLOCK_ZERO);
 }
 
 static bool is_zero_cluster_top_locked(BlockDriverState *bs, int64_t start)
-- 
2.5.0

[Qemu-devel] [PATCH 6/6] qcow2: merge is_zero_cluster helpers into qcow2_co_write_zeroes

[Denis V. Lunev]

They are used once only. This makes code more compact.

The patch also improves comments in the code.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 05beb64..feaf146 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2405,27 +2405,6 @@ finish:
 }
 
 
-static bool is_zero_cluster(BlockDriverState *bs, int64_t start)
-{
-    BDRVQcow2State *s = bs->opaque;
-    int nr;
-    BlockDriverState *file;
-    int64_t res = bdrv_get_block_status_above(bs, NULL, start,
-                                              s->cluster_sectors, &nr, &file);
-    return res >= 0 && (res & BDRV_BLOCK_ZERO);
-}
-
-static bool is_zero_cluster_top_locked(BlockDriverState *bs, int64_t start)
-{
-    BDRVQcow2State *s = bs->opaque;
-    int nr = s->cluster_sectors;
-    uint64_t off;
-    int ret;
-
-    ret = qcow2_get_cluster_offset(bs, start << BDRV_SECTOR_BITS, &nr, &off);
-    return ret == QCOW2_CLUSTER_UNALLOCATED || ret == QCOW2_CLUSTER_ZERO;
-}
-
 static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
 {
@@ -2439,20 +2418,32 @@ static coroutine_fn int qcow2_co_write_zeroes(BlockDriverState *bs,
                                        nb_sectors);
 
     if (head != 0 || tail != 0) {
-        int64_t cl_start = sector_num - head;
+        BlockDriverState *file;
+        uint64_t off;
+        int nr;
+
+        int64_t cl_start = sector_num - head, res;
 
         assert(cl_start + s->cluster_sectors >= sector_num + nb_sectors);
 
         sector_num = cl_start;
         nb_sectors = s->cluster_sectors;
 
-        if (!is_zero_cluster(bs, sector_num)) {
+        /* check that the cluster is zeroed taking into account entire
+           backing chain */
+        nr = s->cluster_sectors;
+        res = bdrv_get_block_status_above(bs, NULL, cl_start,
+                                          s->cluster_sectors, &nr, &file);
+        if (res < 0 || !(res & BDRV_BLOCK_ZERO)) {
             return -ENOTSUP;
         }
 
         qemu_co_mutex_lock(&s->lock);
         /* We can have new write after previous check */
-        if (!is_zero_cluster_top_locked(bs, sector_num)) {
+        nr = s->cluster_sectors;
+        ret = qcow2_get_cluster_offset(bs, cl_start << BDRV_SECTOR_BITS,
+                                       &nr, &off);
+        if (ret != QCOW2_CLUSTER_UNALLOCATED && ret != QCOW2_CLUSTER_ZERO) {
             qemu_co_mutex_unlock(&s->lock);
             return -ENOTSUP;
         }
-- 
2.5.0