[Qemu-devel] [PATCH] Detect zero writes (for discussion only, not to be applied)

[Qemu-devel] [PATCH] Detect zero writes (for discussion only, not to be applied)

[Richard W.M. Jones]

[Applies against qemu 1.1.0, not HEAD, because qemu from git seems to
be broken at the moment.  But the patch is easy to rebase.]

This patch adds a -drive detect_zero=on|off flag (defaults to "off").

Setting this on causes the write path to detect all zero byte writes
and, for block device drivers which support this, turn the request
into the more space-efficient bdrv_co_do_write_zeroes call.

The use case is for virt-sparsify.  We want to create an intermediate
qcow2 overlay file which we know will mostly contain zeroes, and we
want qcow2 to handle this without creating a huge empty file.  In my
testing, the size of the intermediate file is reduced considerably
(down to about 1/4 of the original size), making sparsification of
large mostly-empty guests possible for the first time.

The implementation is very simple-minded.  It tests if the entire
request is all zero bytes or not.  A better implementation would
almost certainly be even more space-efficient, but at the cost of
splitting some write requests (if that's a problem).

Testing: I booted and tested an F16 guest with this patch and the
setting "on", without problems.  I also modified libguestfs to add
this flag, then ran the whole libguestfs test suite with no reported
problems.

Rich.

[Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Richard W.M. Jones]

From: "Richard W.M. Jones" <rjones@redhat.com>

This change adds a new block device option, "detect_zero=on|off".

If "detect_zero=on" then when a guest writes sectors that contain all
zero bytes, we call the internal "bdrv_co_write_zeroes" function
instead of the standard "bdrv_co_writev".  Some drivers, notably qed
and qcow2 version 3, will handle this more space-efficiently by just
recording a zero flag instead of allocating new clusters.

The flip side is that we have to scan each block being written by the
guest to see if it contains all zero bytes.  Therefore this option
defaults to "off".

In the special case where we expect the guest to be mostly writing
zeroes (virt-sparsify) this makes a huge difference in the size of the
intermediate qcow2 file which is created.

Note that if you want to test this feature, you must use a qcow2
version 3 file.  To create that, do:

  qemu-img create -f qcow2 -o compat=1.1 <name> <size>

Ordinary qcow2 (v2) and raw do NOT know how to treat zero blocks
specially, so you won't notice any difference.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 block.c         |   33 ++++++++++++++++++++++++++++++++-
 block.h         |    1 +
 block_int.h     |    1 +
 blockdev.c      |   10 ++++++++++
 hmp-commands.hx |    3 ++-
 qemu-config.c   |    4 ++++
 qemu-options.hx |    7 ++++++-
 7 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 7547051..49e8186 100644
--- a/block.c
+++ b/block.c
@@ -639,6 +639,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
         bdrv_enable_copy_on_read(bs);
     }
 
+    bs->detect_zero = !!(flags & BDRV_O_DETECT_ZERO);
+
     pstrcpy(bs->filename, sizeof(bs->filename), filename);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
@@ -654,7 +656,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
      * Clear flags that are internal to the block layer before opening the
      * image.
      */
-    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_DETECT_ZERO);
 
     /*
      * Snapshots should be writable.
@@ -1940,6 +1942,33 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
 }
 
 /*
+ * Detect zeroes: This is very conservative and could be a lot more
+ * clever/complex.
+ *
+ * All it does now is detect if the whole iovec contains nothing but
+ * zero bytes, and if so call bdrv_co_do_write_zeroes, otherwise call
+ * drv->bdrv_co_writev.
+ *
+ * It would be possible to split requests up and do this
+ * iovec-element-by-element or even sector-by-sector.
+ */
+static int detect_zeroes(BlockDriverState *bs,
+    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
+{
+    BlockDriver *drv = bs->drv;
+    int i;
+
+    for (i = 0; i < qiov->niov; i++) {
+        if (!buffer_is_zero (qiov->iov[i].iov_base, qiov->iov[i].iov_len))
+            goto not_zero;
+    }
+    return bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
+
+ not_zero:
+    return drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
+}
+
+/*
  * Handle a write request in coroutine context
  */
 static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
@@ -1973,6 +2002,8 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
 
     if (flags & BDRV_REQ_ZERO_WRITE) {
         ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
+    } else if (bs->detect_zero) {
+        ret = detect_zeroes (bs, sector_num, nb_sectors, qiov);
     } else {
         ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
     }
diff --git a/block.h b/block.h
index 7408acc..fcc6de6 100644
--- a/block.h
+++ b/block.h
@@ -79,6 +79,7 @@ typedef struct BlockDevOps {
 #define BDRV_O_NO_FLUSH    0x0200 /* disable flushing on this disk */
 #define BDRV_O_COPY_ON_READ 0x0400 /* copy read backing sectors into image */
 #define BDRV_O_INCOMING    0x0800  /* consistency hint for incoming migration */
+#define BDRV_O_DETECT_ZERO 0x1000 /* detect zero writes */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
 
diff --git a/block_int.h b/block_int.h
index 3d4abc6..1008103 100644
--- a/block_int.h
+++ b/block_int.h
@@ -271,6 +271,7 @@ struct BlockDriverState {
     int sg;        /* if true, the device is a /dev/sg* */
     int copy_on_read; /* if true, copy read backing sectors into image
                          note this is a reference count */
+    int detect_zero; /* if true, detect all zero byte writes */
 
     BlockDriver *drv; /* NULL means no media */
     void *opaque;
diff --git a/blockdev.c b/blockdev.c
index 67895b2..580c078 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -296,6 +296,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     BlockIOLimit io_limits;
     int snapshot = 0;
     bool copy_on_read;
+    bool detect_zero;
     int ret;
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
@@ -313,6 +314,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
     ro = qemu_opt_get_bool(opts, "readonly", 0);
     copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
+    detect_zero = qemu_opt_get_bool(opts, "detect-zero", false);
 
     file = qemu_opt_get(opts, "file");
     serial = qemu_opt_get(opts, "serial");
@@ -595,6 +597,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         bdrv_flags |= BDRV_O_COPY_ON_READ;
     }
 
+    if (detect_zero) {
+        bdrv_flags |= BDRV_O_DETECT_ZERO;
+    }
+
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         bdrv_flags |= BDRV_O_INCOMING;
     }
@@ -612,6 +618,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
+    if (ro && detect_zero) {
+        error_report("warning: ignoring detect-zero on readonly drive");
+    }
+
     ret = bdrv_open(dinfo->bdrv, file, bdrv_flags, drv);
     if (ret < 0) {
         error_report("could not open disk image %s: %s",
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 18cb415..7dcdcfe 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -908,7 +908,8 @@ ETEXI
                       "[,unit=m][,media=d][,index=i]\n"
                       "[,cyls=c,heads=h,secs=s[,trans=t]]\n"
                       "[,snapshot=on|off][,cache=on|off]\n"
-                      "[,readonly=on|off][,copy-on-read=on|off]",
+                      "[,readonly=on|off][,copy-on-read=on|off]\n"
+                      "[,detect-zero=on|off]\n",
         .help       = "add drive to PCI storage controller",
         .mhandler.cmd = drive_hot_add,
     },
diff --git a/qemu-config.c b/qemu-config.c
index be84a03..fb19d15 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -113,6 +113,10 @@ static QemuOptsList qemu_drive_opts = {
             .name = "copy-on-read",
             .type = QEMU_OPT_BOOL,
             .help = "copy read data from backing file into image file",
+        },{
+            .name = "detect-zero",
+            .type = QEMU_OPT_BOOL,
+            .help = "detect zero writes and handle space-efficiently",
         },
         { /* end of list */ }
     },
diff --git a/qemu-options.hx b/qemu-options.hx
index 8b66264..5d5da6e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -141,7 +141,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
     "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
-    "       [,readonly=on|off][,copy-on-read=on|off]\n"
+    "       [,readonly=on|off][,copy-on-read=on|off][,detect-zero=on|off]\n"
     "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r][,iops_wr=w]]\n"
     "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
 STEXI
@@ -196,6 +196,11 @@ Open drive @option{file} as read-only. Guest write attempts will fail.
 @item copy-on-read=@var{copy-on-read}
 @var{copy-on-read} is "on" or "off" and enables whether to copy read backing
 file sectors into the image file.
+@item detect-zero=@var{detect-zero}
+@var{detect-zero} is "on" or "off". If "on", guest writes containing
+all zero bytes are detected and may be handled more space-efficiently by
+the underlying block device. Under some workloads this may result in
+worse performance, so it defaults to "off".
 @end table
 
 By default, writethrough caching is used for all block device.  This means that
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Anthony Liguori]

"Richard W.M. Jones" <rjones@redhat.com> writes:

> From: "Richard W.M. Jones" <rjones@redhat.com>
>
> This change adds a new block device option, "detect_zero=on|off".
>
> If "detect_zero=on" then when a guest writes sectors that contain all
> zero bytes, we call the internal "bdrv_co_write_zeroes" function
> instead of the standard "bdrv_co_writev".  Some drivers, notably qed
> and qcow2 version 3, will handle this more space-efficiently by just
> recording a zero flag instead of allocating new clusters.
>
> The flip side is that we have to scan each block being written by the
> guest to see if it contains all zero bytes.  Therefore this option
> defaults to "off".
>
> In the special case where we expect the guest to be mostly writing
> zeroes (virt-sparsify) this makes a huge difference in the size of the
> intermediate qcow2 file which is created.
>
> Note that if you want to test this feature, you must use a qcow2
> version 3 file.  To create that, do:
>
>   qemu-img create -f qcow2 -o compat=1.1 <name> <size>
>
> Ordinary qcow2 (v2) and raw do NOT know how to treat zero blocks
> specially, so you won't notice any difference.

It would be a lot more useful if this was implemented as a dynamic
option.  Offline sparsification is one use-case, but online
sparsification is another.

This can be orchestrated through the qemu-ga binary.  First, you enable
zero detection, then you ask qemu-ga to create a file filled with zeros
of 90% of the available free space unlinking the file as soon as you
open it.  You then close the fd after you've written out the zeros.
Then you disable zero detection and keep going.

This would let virt-manager or oVirt implement a "compact disk" option
against running VMs.

I don't think it's much of a change to your existing patch either.

Regards,

Anthony Liguori

>
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  block.c         |   33 ++++++++++++++++++++++++++++++++-
>  block.h         |    1 +
>  block_int.h     |    1 +
>  blockdev.c      |   10 ++++++++++
>  hmp-commands.hx |    3 ++-
>  qemu-config.c   |    4 ++++
>  qemu-options.hx |    7 ++++++-
>  7 files changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7547051..49e8186 100644
> --- a/block.c
> +++ b/block.c
> @@ -639,6 +639,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
>          bdrv_enable_copy_on_read(bs);
>      }
>  
> +    bs->detect_zero = !!(flags & BDRV_O_DETECT_ZERO);
> +
>      pstrcpy(bs->filename, sizeof(bs->filename), filename);
>  
>      if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
> @@ -654,7 +656,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
>       * Clear flags that are internal to the block layer before opening the
>       * image.
>       */
> -    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_DETECT_ZERO);
>  
>      /*
>       * Snapshots should be writable.
> @@ -1940,6 +1942,33 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>  }
>  
>  /*
> + * Detect zeroes: This is very conservative and could be a lot more
> + * clever/complex.
> + *
> + * All it does now is detect if the whole iovec contains nothing but
> + * zero bytes, and if so call bdrv_co_do_write_zeroes, otherwise call
> + * drv->bdrv_co_writev.
> + *
> + * It would be possible to split requests up and do this
> + * iovec-element-by-element or even sector-by-sector.
> + */
> +static int detect_zeroes(BlockDriverState *bs,
> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int i;
> +
> +    for (i = 0; i < qiov->niov; i++) {
> +        if (!buffer_is_zero (qiov->iov[i].iov_base, qiov->iov[i].iov_len))
> +            goto not_zero;
> +    }
> +    return bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
> +
> + not_zero:
> +    return drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
> +}
> +
> +/*
>   * Handle a write request in coroutine context
>   */
>  static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
> @@ -1973,6 +2002,8 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>  
>      if (flags & BDRV_REQ_ZERO_WRITE) {
>          ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
> +    } else if (bs->detect_zero) {
> +        ret = detect_zeroes (bs, sector_num, nb_sectors, qiov);
>      } else {
>          ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
>      }
> diff --git a/block.h b/block.h
> index 7408acc..fcc6de6 100644
> --- a/block.h
> +++ b/block.h
> @@ -79,6 +79,7 @@ typedef struct BlockDevOps {
>  #define BDRV_O_NO_FLUSH    0x0200 /* disable flushing on this disk */
>  #define BDRV_O_COPY_ON_READ 0x0400 /* copy read backing sectors into image */
>  #define BDRV_O_INCOMING    0x0800  /* consistency hint for incoming migration */
> +#define BDRV_O_DETECT_ZERO 0x1000 /* detect zero writes */
>  
>  #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>  
> diff --git a/block_int.h b/block_int.h
> index 3d4abc6..1008103 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -271,6 +271,7 @@ struct BlockDriverState {
>      int sg;        /* if true, the device is a /dev/sg* */
>      int copy_on_read; /* if true, copy read backing sectors into image
>                           note this is a reference count */
> +    int detect_zero; /* if true, detect all zero byte writes */
>  
>      BlockDriver *drv; /* NULL means no media */
>      void *opaque;
> diff --git a/blockdev.c b/blockdev.c
> index 67895b2..580c078 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -296,6 +296,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      BlockIOLimit io_limits;
>      int snapshot = 0;
>      bool copy_on_read;
> +    bool detect_zero;
>      int ret;
>  
>      translation = BIOS_ATA_TRANSLATION_AUTO;
> @@ -313,6 +314,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
>      ro = qemu_opt_get_bool(opts, "readonly", 0);
>      copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
> +    detect_zero = qemu_opt_get_bool(opts, "detect-zero", false);
>  
>      file = qemu_opt_get(opts, "file");
>      serial = qemu_opt_get(opts, "serial");
> @@ -595,6 +597,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>          bdrv_flags |= BDRV_O_COPY_ON_READ;
>      }
>  
> +    if (detect_zero) {
> +        bdrv_flags |= BDRV_O_DETECT_ZERO;
> +    }
> +
>      if (runstate_check(RUN_STATE_INMIGRATE)) {
>          bdrv_flags |= BDRV_O_INCOMING;
>      }
> @@ -612,6 +618,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>  
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
> +    if (ro && detect_zero) {
> +        error_report("warning: ignoring detect-zero on readonly drive");
> +    }
> +
>      ret = bdrv_open(dinfo->bdrv, file, bdrv_flags, drv);
>      if (ret < 0) {
>          error_report("could not open disk image %s: %s",
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 18cb415..7dcdcfe 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -908,7 +908,8 @@ ETEXI
>                        "[,unit=m][,media=d][,index=i]\n"
>                        "[,cyls=c,heads=h,secs=s[,trans=t]]\n"
>                        "[,snapshot=on|off][,cache=on|off]\n"
> -                      "[,readonly=on|off][,copy-on-read=on|off]",
> +                      "[,readonly=on|off][,copy-on-read=on|off]\n"
> +                      "[,detect-zero=on|off]\n",
>          .help       = "add drive to PCI storage controller",
>          .mhandler.cmd = drive_hot_add,
>      },
> diff --git a/qemu-config.c b/qemu-config.c
> index be84a03..fb19d15 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -113,6 +113,10 @@ static QemuOptsList qemu_drive_opts = {
>              .name = "copy-on-read",
>              .type = QEMU_OPT_BOOL,
>              .help = "copy read data from backing file into image file",
> +        },{
> +            .name = "detect-zero",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "detect zero writes and handle space-efficiently",
>          },
>          { /* end of list */ }
>      },
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8b66264..5d5da6e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -141,7 +141,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>      "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
>      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>      "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
> -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
> +    "       [,readonly=on|off][,copy-on-read=on|off][,detect-zero=on|off]\n"
>      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r][,iops_wr=w]]\n"
>      "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
>  STEXI
> @@ -196,6 +196,11 @@ Open drive @option{file} as read-only. Guest write attempts will fail.
>  @item copy-on-read=@var{copy-on-read}
>  @var{copy-on-read} is "on" or "off" and enables whether to copy read backing
>  file sectors into the image file.
> +@item detect-zero=@var{detect-zero}
> +@var{detect-zero} is "on" or "off". If "on", guest writes containing
> +all zero bytes are detected and may be handled more space-efficiently by
> +the underlying block device. Under some workloads this may result in
> +worse performance, so it defaults to "off".
>  @end table
>  
>  By default, writethrough caching is used for all block device.  This means that
> -- 
> 1.7.10.4

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Paolo Bonzini]

Il 27/07/2012 15:58, Anthony Liguori ha scritto:
>> Note that if you want to test this feature, you must use a qcow2
>> version 3 file.  To create that, do:
>>
>>   qemu-img create -f qcow2 -o compat=1.1 <name> <size>
>>
>> Ordinary qcow2 (v2) and raw do NOT know how to treat zero blocks
>> specially, so you won't notice any difference.
> 
> It would be a lot more useful if this was implemented as a dynamic
> option.  Offline sparsification is one use-case, but online
> sparsification is another.
> 
> This can be orchestrated through the qemu-ga binary.  First, you enable
> zero detection, then you ask qemu-ga to create a file filled with zeros
> of 90% of the available free space unlinking the file as soon as you
> open it.  You then close the fd after you've written out the zeros.
> Then you disable zero detection and keep going.

Or just finish up discard support and use the existing fstrim command of
qemu-ga.  :)

Paolo

> 
> This would let virt-manager or oVirt implement a "compact disk" option
> against running VMs.
> 
> I don't think it's much of a change to your existing patch either.
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
>> ---
>>  block.c         |   33 ++++++++++++++++++++++++++++++++-
>>  block.h         |    1 +
>>  block_int.h     |    1 +
>>  blockdev.c      |   10 ++++++++++
>>  hmp-commands.hx |    3 ++-
>>  qemu-config.c   |    4 ++++
>>  qemu-options.hx |    7 ++++++-
>>  7 files changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 7547051..49e8186 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -639,6 +639,8 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
>>          bdrv_enable_copy_on_read(bs);
>>      }
>>  
>> +    bs->detect_zero = !!(flags & BDRV_O_DETECT_ZERO);
>> +
>>      pstrcpy(bs->filename, sizeof(bs->filename), filename);
>>  
>>      if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
>> @@ -654,7 +656,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
>>       * Clear flags that are internal to the block layer before opening the
>>       * image.
>>       */
>> -    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>> +    open_flags = flags & ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_DETECT_ZERO);
>>  
>>      /*
>>       * Snapshots should be writable.
>> @@ -1940,6 +1942,33 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>  }
>>  
>>  /*
>> + * Detect zeroes: This is very conservative and could be a lot more
>> + * clever/complex.
>> + *
>> + * All it does now is detect if the whole iovec contains nothing but
>> + * zero bytes, and if so call bdrv_co_do_write_zeroes, otherwise call
>> + * drv->bdrv_co_writev.
>> + *
>> + * It would be possible to split requests up and do this
>> + * iovec-element-by-element or even sector-by-sector.
>> + */
>> +static int detect_zeroes(BlockDriverState *bs,
>> +    int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    int i;
>> +
>> +    for (i = 0; i < qiov->niov; i++) {
>> +        if (!buffer_is_zero (qiov->iov[i].iov_base, qiov->iov[i].iov_len))
>> +            goto not_zero;
>> +    }
>> +    return bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
>> +
>> + not_zero:
>> +    return drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
>> +}
>> +
>> +/*
>>   * Handle a write request in coroutine context
>>   */
>>  static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>> @@ -1973,6 +2002,8 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>>  
>>      if (flags & BDRV_REQ_ZERO_WRITE) {
>>          ret = bdrv_co_do_write_zeroes(bs, sector_num, nb_sectors);
>> +    } else if (bs->detect_zero) {
>> +        ret = detect_zeroes (bs, sector_num, nb_sectors, qiov);
>>      } else {
>>          ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
>>      }
>> diff --git a/block.h b/block.h
>> index 7408acc..fcc6de6 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -79,6 +79,7 @@ typedef struct BlockDevOps {
>>  #define BDRV_O_NO_FLUSH    0x0200 /* disable flushing on this disk */
>>  #define BDRV_O_COPY_ON_READ 0x0400 /* copy read backing sectors into image */
>>  #define BDRV_O_INCOMING    0x0800  /* consistency hint for incoming migration */
>> +#define BDRV_O_DETECT_ZERO 0x1000 /* detect zero writes */
>>  
>>  #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>>  
>> diff --git a/block_int.h b/block_int.h
>> index 3d4abc6..1008103 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -271,6 +271,7 @@ struct BlockDriverState {
>>      int sg;        /* if true, the device is a /dev/sg* */
>>      int copy_on_read; /* if true, copy read backing sectors into image
>>                           note this is a reference count */
>> +    int detect_zero; /* if true, detect all zero byte writes */
>>  
>>      BlockDriver *drv; /* NULL means no media */
>>      void *opaque;
>> diff --git a/blockdev.c b/blockdev.c
>> index 67895b2..580c078 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -296,6 +296,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>      BlockIOLimit io_limits;
>>      int snapshot = 0;
>>      bool copy_on_read;
>> +    bool detect_zero;
>>      int ret;
>>  
>>      translation = BIOS_ATA_TRANSLATION_AUTO;
>> @@ -313,6 +314,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>      snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
>>      ro = qemu_opt_get_bool(opts, "readonly", 0);
>>      copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
>> +    detect_zero = qemu_opt_get_bool(opts, "detect-zero", false);
>>  
>>      file = qemu_opt_get(opts, "file");
>>      serial = qemu_opt_get(opts, "serial");
>> @@ -595,6 +597,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>          bdrv_flags |= BDRV_O_COPY_ON_READ;
>>      }
>>  
>> +    if (detect_zero) {
>> +        bdrv_flags |= BDRV_O_DETECT_ZERO;
>> +    }
>> +
>>      if (runstate_check(RUN_STATE_INMIGRATE)) {
>>          bdrv_flags |= BDRV_O_INCOMING;
>>      }
>> @@ -612,6 +618,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>  
>>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>  
>> +    if (ro && detect_zero) {
>> +        error_report("warning: ignoring detect-zero on readonly drive");
>> +    }
>> +
>>      ret = bdrv_open(dinfo->bdrv, file, bdrv_flags, drv);
>>      if (ret < 0) {
>>          error_report("could not open disk image %s: %s",
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 18cb415..7dcdcfe 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -908,7 +908,8 @@ ETEXI
>>                        "[,unit=m][,media=d][,index=i]\n"
>>                        "[,cyls=c,heads=h,secs=s[,trans=t]]\n"
>>                        "[,snapshot=on|off][,cache=on|off]\n"
>> -                      "[,readonly=on|off][,copy-on-read=on|off]",
>> +                      "[,readonly=on|off][,copy-on-read=on|off]\n"
>> +                      "[,detect-zero=on|off]\n",
>>          .help       = "add drive to PCI storage controller",
>>          .mhandler.cmd = drive_hot_add,
>>      },
>> diff --git a/qemu-config.c b/qemu-config.c
>> index be84a03..fb19d15 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -113,6 +113,10 @@ static QemuOptsList qemu_drive_opts = {
>>              .name = "copy-on-read",
>>              .type = QEMU_OPT_BOOL,
>>              .help = "copy read data from backing file into image file",
>> +        },{
>> +            .name = "detect-zero",
>> +            .type = QEMU_OPT_BOOL,
>> +            .help = "detect zero writes and handle space-efficiently",
>>          },
>>          { /* end of list */ }
>>      },
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 8b66264..5d5da6e 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -141,7 +141,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>>      "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
>>      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>>      "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
>> -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
>> +    "       [,readonly=on|off][,copy-on-read=on|off][,detect-zero=on|off]\n"
>>      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r][,iops_wr=w]]\n"
>>      "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
>>  STEXI
>> @@ -196,6 +196,11 @@ Open drive @option{file} as read-only. Guest write attempts will fail.
>>  @item copy-on-read=@var{copy-on-read}
>>  @var{copy-on-read} is "on" or "off" and enables whether to copy read backing
>>  file sectors into the image file.
>> +@item detect-zero=@var{detect-zero}
>> +@var{detect-zero} is "on" or "off". If "on", guest writes containing
>> +all zero bytes are detected and may be handled more space-efficiently by
>> +the underlying block device. Under some workloads this may result in
>> +worse performance, so it defaults to "off".
>>  @end table
>>  
>>  By default, writethrough caching is used for all block device.  This means that
>> -- 
>> 1.7.10.4
> 
> 

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Richard W.M. Jones]

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

On Fri, Jul 27, 2012 at 09:01:38PM +0200, Paolo Bonzini wrote:
> Or just finish up discard support and use the existing fstrim command of
> qemu-ga.  :)

What's actually involved to do this?  I noticed that a virtio-scsi on
qcow2 v3 device exported to the guest does not appear to support TRIM
at all (see attached test script and output).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

[-- Attachment #2: test.sh --]
[-- Type: application/x-sh, Size: 700 bytes --]

[-- Attachment #3: test-output.txt --]
[-- Type: text/plain, Size: 1629 bytes --]

Formatting 'test1.qcow2', fmt=qcow2 size=104857600 compat='1.1' encryption=off cluster_size=65536 
libguestfs: trace: add_drive "test1.qcow2"
libguestfs: trace: add_drive = 0
libguestfs: trace: launch
libguestfs: trace: launch = 0
libguestfs: trace: debug "sh" "uname -a"
libguestfs: trace: debug = "Linux (none) 3.5.0-1.fc18.x86_64 #1 SMP Mon Jul 23 17:43:39 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux"
Linux (none) 3.5.0-1.fc18.x86_64 #1 SMP Mon Jul 23 17:43:39 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
libguestfs: trace: debug "sh" "head -n99999999 /sys/block/sd*/queue/discard_max_bytes"
libguestfs: trace: debug = "==> /sys/block/sda/queue/discard_max_bytes <==\n0\n\n==> /sys/block/sdb/queue/discard_max_bytes <==\n0"
==> /sys/block/sda/queue/discard_max_bytes <==
0

==> /sys/block/sdb/queue/discard_max_bytes <==
0
libguestfs: trace: part_disk "/dev/sda" "mbr"
libguestfs: trace: part_disk = 0
libguestfs: trace: mkfs "ext4" "/dev/sda1"
libguestfs: trace: mkfs = 0
libguestfs: trace: mount_options "discard" "/dev/sda1" "/"
libguestfs: trace: mount_options = 0
libguestfs: trace: fill 88 52428800 "/data"
libguestfs: trace: fill = 0
libguestfs: trace: sync
libguestfs: trace: sync = 0
-rw-r--r--. 1 rjones rjones 56M Jul 30 14:49 test1.qcow2
libguestfs: trace: rm "/data"
libguestfs: trace: rm = 0
libguestfs: trace: fstrim "/"
libguestfs: trace: fstrim = -1 (error)
*stdin*:23: libguestfs: error: fstrim: fstrim: /: FITRIM ioctl failed: Operation not supported
libguestfs: trace: close
libguestfs: trace: shutdown
libguestfs: trace: internal_autosync
libguestfs: trace: internal_autosync = 0
libguestfs: trace: shutdown = 0

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Paolo Bonzini]

Il 30/07/2012 15:51, Richard W.M. Jones ha scritto:
>> > Or just finish up discard support and use the existing fstrim command of
>> > qemu-ga.  :)
> What's actually involved to do this?  I noticed that a virtio-scsi on
> qcow2 v3 device exported to the guest does not appear to support TRIM
> at all (see attached test script and output).

The most basic thing to do is to pass -device discard_granularity=NNN,
where NNN should be the cluster size of the device.  On top of this you
need to add the ability to do TRIM asynchronously, quite some testing,
and perhaps improving the disk formats so that they support
sector-granularity discard.

Patches for asynchronous TRIM are in the works (the threadpool patches
on the list are the first step).

Long term I would like the ability to distinguish
discard-for-thin-provisioning ("may cause fragmentation, all subsequent
accesses could be slower") from discard-for-wear-leveling ("only the
first subsequent access would be slowed down").  This requires changes
at all levels (host kernel, QEMU, management, guest kernel, and possibly
guest applications).

Paolo

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Eric Blake]

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

On 07/30/2012 07:57 AM, Paolo Bonzini wrote:
> Il 30/07/2012 15:51, Richard W.M. Jones ha scritto:
>>>> Or just finish up discard support and use the existing fstrim command of
>>>> qemu-ga.  :)
>> What's actually involved to do this?  I noticed that a virtio-scsi on
>> qcow2 v3 device exported to the guest does not appear to support TRIM
>> at all (see attached test script and output).
> 
> The most basic thing to do is to pass -device discard_granularity=NNN,
> where NNN should be the cluster size of the device.  On top of this you
> need to add the ability to do TRIM asynchronously, quite some testing,
> and perhaps improving the disk formats so that they support
> sector-granularity discard.
> 
> Patches for asynchronous TRIM are in the works (the threadpool patches
> on the list are the first step).
> 
> Long term I would like the ability to distinguish
> discard-for-thin-provisioning ("may cause fragmentation, all subsequent
> accesses could be slower") from discard-for-wear-leveling ("only the
> first subsequent access would be slowed down").  This requires changes
> at all levels (host kernel, QEMU, management, guest kernel, and possibly
> guest applications).

Ooh, nice bullet point to add to my upcoming presentation at Linux
Plumber's Conference in a month:
http://summit.linuxplumbersconf.org/lpc-2012/meeting/33/lpc2012-ref-improved-virt-disk-handling/

Does anyone else have some annoyances about large sparse file handling
where improving the kernel would make our life easier, that I should
incorporate into my discussion?

-- 
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: 620 bytes --]

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Paolo Bonzini]

Il 30/07/2012 16:03, Eric Blake ha scritto:
> Ooh, nice bullet point to add to my upcoming presentation at Linux
> Plumber's Conference in a month:
> http://summit.linuxplumbersconf.org/lpc-2012/meeting/33/lpc2012-ref-improved-virt-disk-handling/
> 
> Does anyone else have some annoyances about large sparse file handling
> where improving the kernel would make our life easier, that I should
> incorporate into my discussion?

In case you haven't mentioned them:

* lseek(SEEK_HOLE/SEEK_DATA) support in the block layer using GET LBA
STATUS.

* support for connecting to an iSCSI target without scanning partitions.

* support for fsync/fdatasync with ranges (or alternatively,
sync_file_range that writes metadata).

Paolo

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Kevin Wolf]

Am 30.07.2012 16:09, schrieb Paolo Bonzini:
> Il 30/07/2012 16:03, Eric Blake ha scritto:
>> Ooh, nice bullet point to add to my upcoming presentation at Linux
>> Plumber's Conference in a month:
>> http://summit.linuxplumbersconf.org/lpc-2012/meeting/33/lpc2012-ref-improved-virt-disk-handling/
>>
>> Does anyone else have some annoyances about large sparse file handling
>> where improving the kernel would make our life easier, that I should
>> incorporate into my discussion?
> 
> In case you haven't mentioned them:
> 
> * lseek(SEEK_HOLE/SEEK_DATA) support in the block layer using GET LBA
> STATUS.
> 
> * support for connecting to an iSCSI target without scanning partitions.
> 
> * support for fsync/fdatasync with ranges (or alternatively,
> sync_file_range that writes metadata).

In the context of libguestfs we recently found this one:

* sync() should actually flush the disk caches

Kevin

Re: [Qemu-devel] [PATCH] block: Add -drive detect_zero=on|off option to detect all zero writes.

[Richard W.M. Jones]

On Mon, Jul 30, 2012 at 04:38:25PM +0200, Kevin Wolf wrote:
> Am 30.07.2012 16:09, schrieb Paolo Bonzini:
> > Il 30/07/2012 16:03, Eric Blake ha scritto:
> >> Ooh, nice bullet point to add to my upcoming presentation at Linux
> >> Plumber's Conference in a month:
> >> http://summit.linuxplumbersconf.org/lpc-2012/meeting/33/lpc2012-ref-improved-virt-disk-handling/
> >>
> >> Does anyone else have some annoyances about large sparse file handling
> >> where improving the kernel would make our life easier, that I should
> >> incorporate into my discussion?
> > 
> > In case you haven't mentioned them:
> > 
> > * lseek(SEEK_HOLE/SEEK_DATA) support in the block layer using GET LBA
> > STATUS.
> > 
> > * support for connecting to an iSCSI target without scanning partitions.
> > 
> > * support for fsync/fdatasync with ranges (or alternatively,
> > sync_file_range that writes metadata).
> 
> In the context of libguestfs we recently found this one:
> 
> * sync() should actually flush the disk caches

I have high hopes that the sync patches by Jan Kara
(9e9ad5f408..4ea425b63a) that went upstream a couple of weeks ago
should fix this properly, although I've not tried a kernel with these
patches yet.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

Re: [Qemu-devel] [PATCH] Detect zero writes (for discussion only, not to be applied)

[Anthony Liguori]

"Richard W.M. Jones" <rjones@redhat.com> writes:

> [Applies against qemu 1.1.0, not HEAD, because qemu from git seems to
> be broken at the moment.  But the patch is easy to rebase.]

How's it broken?

Regards,

Anthony Liguori

> This patch adds a -drive detect_zero=on|off flag (defaults to "off").
>
> Setting this on causes the write path to detect all zero byte writes
> and, for block device drivers which support this, turn the request
> into the more space-efficient bdrv_co_do_write_zeroes call.
>
> The use case is for virt-sparsify.  We want to create an intermediate
> qcow2 overlay file which we know will mostly contain zeroes, and we
> want qcow2 to handle this without creating a huge empty file.  In my
> testing, the size of the intermediate file is reduced considerably
> (down to about 1/4 of the original size), making sparsification of
> large mostly-empty guests possible for the first time.
>
> The implementation is very simple-minded.  It tests if the entire
> request is all zero bytes or not.  A better implementation would
> almost certainly be even more space-efficient, but at the cost of
> splitting some write requests (if that's a problem).
>
> Testing: I booted and tested an F16 guest with this patch and the
> setting "on", without problems.  I also modified libguestfs to add
> this flag, then ran the whole libguestfs test suite with no reported
> problems.
>
> Rich.

Re: [Qemu-devel] [PATCH] Detect zero writes (for discussion only, not to be applied)

[Richard W.M. Jones]

On Fri, Jul 27, 2012 at 08:55:31AM -0500, Anthony Liguori wrote:
> "Richard W.M. Jones" <rjones@redhat.com> writes:
> 
> > [Applies against qemu 1.1.0, not HEAD, because qemu from git seems to
> > be broken at the moment.  But the patch is easy to rebase.]
> 
> How's it broken?

I take this back -- it was a seabios incompatibility.  I tried the
patch with qemu.git on another machine and it works fine.  Will rebase
accordingly when I post the next version.

> [dynamic option]

Agreed.  I'll take a look at implementing it that way.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/