[Qemu-devel] [PATCH v6 0/7] eliminate data write in bdrv_write_zeroes on Linux in raw-posix.c

[Qemu-devel] [PATCH v6 0/7] eliminate data write in bdrv_write_zeroes on Linux in raw-posix.c

[Denis V. Lunev]

I have performed several tests with non-aligned fallocate calls and
in all cases (with non-aligned fallocates) Linux performs fine, i.e.
areas are zeroed correctly. Checks were made on
Linux 3.16.0-28-generic #38-Ubuntu SMP

This should seriously increase performance of bdrv_write_zeroes

Changes from v5:
- changed order between patches 5/6
- check has_fallocate in FALLOC_FL_PUNCH_HOLE branch of the code
- minor comment tweaks as pointed out by Max Reitz

Changes from v4:
- comments to patches are improved by Max Reitz suggestions
- replaced 'return ret' with 'return -ENOTSUP' handle_aiocb_write_zeroes
  The idea is that we can reach that point only if original ret was
  equal to -ENOTSUP
- added has_fallocate flag to indicate that fallocate is working for
  given BDS
- checked file length with bdrv_getlength

Changes from v3:
- dropped original patch 1, equivalent stuff was merged already
- reordered patches as suggested by Fam
- fixes spelling errors as suggested by Fam
- fixed not initialized value in handle_aiocb_write_zeroes
- fixed wrong error processing from do_fallocate(FALLOC_FL_ZERO_RANGE)

Changes from v2:
- added Peter Lieven to CC
- added CONFIG_FALLOCATE check to call do_fallocate in patch 7
- dropped patch 1 as NACK-ed
- added processing of very large data areas in bdrv_co_write_zeroes (new
  patch 1)
- set bl.max_write_zeroes to INT_MAX in raw-posix.c for regular files
  (new patch 8)

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH 1/7] block/raw-posix: create translate_err helper to merge errno values

[Denis V. Lunev]

actually the code
    if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
        ret == -ENOTTY) {
        ret = -ENOTSUP;
    }
is present twice and will be added a couple more times. Create helper
for this.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e51293a..24300d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -893,6 +893,15 @@ static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
 }
 #endif
 
+static int translate_err(int err)
+{
+    if (err == -ENODEV || err == -ENOSYS || err == -EOPNOTSUPP ||
+        err == -ENOTTY) {
+        err = -ENOTSUP;
+    }
+    return err;
+}
+
 static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
 {
     int ret = -EOPNOTSUPP;
@@ -921,10 +930,9 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
 #endif
     }
 
-    if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
-        ret == -ENOTTY) {
+    ret = translate_err(ret);
+    if (ret == -ENOTSUP) {
         s->has_write_zeroes = false;
-        ret = -ENOTSUP;
     }
     return ret;
 }
@@ -968,10 +976,9 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
 #endif
     }
 
-    if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
-        ret == -ENOTTY) {
+    ret = translate_err(ret);
+    if (ret == -ENOTSUP) {
         s->has_discard = false;
-        ret = -ENOTSUP;
     }
     return ret;
 }
-- 
1.9.1

[Qemu-devel] [PATCH 2/7] block/raw-posix: create do_fallocate helper

[Denis V. Lunev]

The pattern
    do {
        if (fallocate(s->fd, mode, offset, len) == 0) {
            return 0;
        }
    } while (errno == EINTR);
    ret = translate_err(-errno);
will be commonly useful in next patches. Create helper for it.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 24300d0..2aa268a 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -902,6 +902,18 @@ static int translate_err(int err)
     return err;
 }
 
+#if defined(CONFIG_FALLOCATE_PUNCH_HOLE)
+static int do_fallocate(int fd, int mode, off_t offset, off_t len)
+{
+    do {
+        if (fallocate(fd, mode, offset, len) == 0) {
+            return 0;
+        }
+    } while (errno == EINTR);
+    return translate_err(-errno);
+}
+#endif
+
 static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
 {
     int ret = -EOPNOTSUPP;
@@ -965,14 +977,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
 #endif
 
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
-        do {
-            if (fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
-                          aiocb->aio_offset, aiocb->aio_nbytes) == 0) {
-                return 0;
-            }
-        } while (errno == EINTR);
-
-        ret = -errno;
+        ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+                           aiocb->aio_offset, aiocb->aio_nbytes);
 #endif
     }
 
-- 
1.9.1

[Qemu-devel] [PATCH 3/7] block/raw-posix: refactor handle_aiocb_write_zeroes a bit

[Denis V. Lunev]

move code dealing with a block device to a separate function. This will
allow to implement additional processing for ordinary files.

Please note, that xfs_code has been moved before checking for
s->has_write_zeroes as xfs_write_zeroes does not touch this flag inside.
This makes code a bit more consistent.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2aa268a..d3910fd 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -914,41 +914,49 @@ static int do_fallocate(int fd, int mode, off_t offset, off_t len)
 }
 #endif
 
-static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
+static ssize_t handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
 {
-    int ret = -EOPNOTSUPP;
+    int ret = -ENOTSUP;
     BDRVRawState *s = aiocb->bs->opaque;
 
-    if (s->has_write_zeroes == 0) {
+    if (!s->has_write_zeroes) {
         return -ENOTSUP;
     }
 
-    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
 #ifdef BLKZEROOUT
-        do {
-            uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
-            if (ioctl(aiocb->aio_fildes, BLKZEROOUT, range) == 0) {
-                return 0;
-            }
-        } while (errno == EINTR);
-
-        ret = -errno;
-#endif
-    } else {
-#ifdef CONFIG_XFS
-        if (s->is_xfs) {
-            return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
+    do {
+        uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
+        if (ioctl(aiocb->aio_fildes, BLKZEROOUT, range) == 0) {
+            return 0;
         }
+    } while (errno == EINTR);
+
+    ret = translate_err(-errno);
 #endif
-    }
 
-    ret = translate_err(ret);
     if (ret == -ENOTSUP) {
         s->has_write_zeroes = false;
     }
     return ret;
 }
 
+static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
+{
+    BDRVRawState *s = aiocb->bs->opaque;
+
+    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
+        return handle_aiocb_write_zeroes_block(aiocb);
+    }
+
+#ifdef CONFIG_XFS
+    if (s->is_xfs) {
+        return xfs_write_zeroes(s, aiocb->aio_offset, aiocb->aio_nbytes);
+    }
+#endif
+
+    return -ENOTSUP;
+}
+
 static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
 {
     int ret = -EOPNOTSUPP;
-- 
1.9.1

[Qemu-devel] [PATCH 4/7] block: use fallocate(FALLOC_FL_ZERO_RANGE) in handle_aiocb_write_zeroes

[Denis V. Lunev]

This efficiently writes zeroes on Linux if the kernel is capable enough.
FALLOC_FL_ZERO_RANGE correctly handles all cases, including and not
including file expansion.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 15 +++++++++++++--
 configure         | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index d3910fd..5a777e7 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -60,7 +60,7 @@
 #define FS_NOCOW_FL                     0x00800000 /* Do not cow file */
 #endif
 #endif
-#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
+#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE)
 #include <linux/falloc.h>
 #endif
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
@@ -902,7 +902,7 @@ static int translate_err(int err)
     return err;
 }
 
-#if defined(CONFIG_FALLOCATE_PUNCH_HOLE)
+#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE)
 static int do_fallocate(int fd, int mode, off_t offset, off_t len)
 {
     do {
@@ -954,6 +954,17 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
     }
 #endif
 
+#ifdef CONFIG_FALLOCATE_ZERO_RANGE
+    if (s->has_write_zeroes) {
+        int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE,
+                               aiocb->aio_offset, aiocb->aio_nbytes);
+        if (ret == 0 || ret != -ENOTSUP) {
+            return ret;
+        }
+        s->has_write_zeroes = false;
+    }
+#endif
+
     return -ENOTSUP;
 }
 
diff --git a/configure b/configure
index f185dd0..e00e03a 100755
--- a/configure
+++ b/configure
@@ -3335,6 +3335,22 @@ if compile_prog "" "" ; then
   fallocate_punch_hole=yes
 fi
 
+# check that fallocate supports range zeroing inside the file
+fallocate_zero_range=no
+cat > $TMPC << EOF
+#include <fcntl.h>
+#include <linux/falloc.h>
+
+int main(void)
+{
+    fallocate(0, FALLOC_FL_ZERO_RANGE, 0, 0);
+    return 0;
+}
+EOF
+if compile_prog "" "" ; then
+  fallocate_zero_range=yes
+fi
+
 # check for posix_fallocate
 posix_fallocate=no
 cat > $TMPC << EOF
@@ -4567,6 +4583,9 @@ fi
 if test "$fallocate_punch_hole" = "yes" ; then
   echo "CONFIG_FALLOCATE_PUNCH_HOLE=y" >> $config_host_mak
 fi
+if test "$fallocate_zero_range" = "yes" ; then
+  echo "CONFIG_FALLOCATE_ZERO_RANGE=y" >> $config_host_mak
+fi
 if test "$posix_fallocate" = "yes" ; then
   echo "CONFIG_POSIX_FALLOCATE=y" >> $config_host_mak
 fi
-- 
1.9.1

[Qemu-devel] [PATCH 5/7] block/raw-posix: call plain fallocate in handle_aiocb_write_zeroes

[Denis V. Lunev]

There is a possibility that we are extending our image and thus writing
zeroes beyond the end of the file. In this case we do not need to care
about the hole to make sure that there is no data in the file under
this offset (pre-condition to fallocate(0) to work). We could simply call
fallocate(0).

This improves the performance of writing zeroes even on really old
platforms which do not have even FALLOC_FL_PUNCH_HOLE.

Before the patch do_fallocate was used when either
CONFIG_FALLOCATE_PUNCH_HOLE or CONFIG_FALLOCATE_ZERO_RANGE are defined.
Now the story is different. CONFIG_FALLOCATE is defined when Linux
fallocate is defined, posix_fallocate is completely different story
(CONFIG_POSIX_FALLOCATE). CONFIG_FALLOCATE is mandatory prerequite
for both CONFIG_FALLOCATE_PUNCH_HOLE and CONFIG_FALLOCATE_ZERO_RANGE
thus we are on the safe side.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 5a777e7..1c88ad8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -147,6 +147,7 @@ typedef struct BDRVRawState {
     bool has_discard:1;
     bool has_write_zeroes:1;
     bool discard_zeroes:1;
+    bool has_fallocate;
     bool needs_alignment;
 } BDRVRawState;
 
@@ -452,6 +453,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     if (S_ISREG(st.st_mode)) {
         s->discard_zeroes = true;
+        s->has_fallocate = true;
     }
     if (S_ISBLK(st.st_mode)) {
 #ifdef BLKDISCARDZEROES
@@ -902,7 +904,7 @@ static int translate_err(int err)
     return err;
 }
 
-#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE)
+#ifdef CONFIG_FALLOCATE
 static int do_fallocate(int fd, int mode, off_t offset, off_t len)
 {
     do {
@@ -965,6 +967,16 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
     }
 #endif
 
+#ifdef CONFIG_FALLOCATE
+    if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
+        int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
+        if (ret == 0 || ret != -ENOTSUP) {
+            return ret;
+        }
+        s->has_fallocate = false;
+    }
+#endif
+
     return -ENOTSUP;
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 6/7] block: use fallocate(FALLOC_FL_PUNCH_HOLE) & fallocate(0) to write zeroes

[Denis V. Lunev]

This sequence works efficiently if FALLOC_FL_ZERO_RANGE is not supported.
Unfortunately, FALLOC_FL_ZERO_RANGE is supported on really modern systems
and only for a couple of filesystems. FALLOC_FL_PUNCH_HOLE is much more
mature.

The sequence of 2 operations FALLOC_FL_PUNCH_HOLE and 0 is necessary due
to the following reasons:
- FALLOC_FL_PUNCH_HOLE creates a hole in the file, the file becomes
  sparse. In order to retain original functionality we must allocate
  disk space afterwards. This is done using fallocate(0) call
- fallocate(0) without preceeding FALLOC_FL_PUNCH_HOLE will do nothing
  if called above already allocated areas of the file, i.e. the content
  will not be zeroed

This should increase the performance a bit for not-so-modern kernels.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 1c88ad8..7b42f37 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -967,6 +967,25 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
     }
 #endif
 
+#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
+    if (s->has_discard && s->has_fallocate) {
+        int ret = do_fallocate(s->fd,
+                               FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+                               aiocb->aio_offset, aiocb->aio_nbytes);
+        if (ret == 0) {
+            ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
+            if (ret == 0 || ret != -ENOTSUP) {
+                return ret;
+            }
+            s->has_fallocate = false;
+        } else if (ret != -ENOTSUP) {
+            return ret;
+        } else {
+            s->has_discard = false;
+        }
+    }
+#endif
+
 #ifdef CONFIG_FALLOCATE
     if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
         int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
-- 
1.9.1

[Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Denis V. Lunev]

fallocate() works fine and could handle properly with arbitrary size
requests. There is no sense to reduce the amount of space to fallocate.
The bigger is the size, the better is the performance as the amount of
journal updates is reduced.

The patch changes behavior for both generic filesystem and XFS codepaths,
which are different in handle_aiocb_write_zeroes. The implementation
of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
thus the change is fine for both ways.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Peter Lieven <pl@kamp.de>
CC: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 7b42f37..933c778 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     }
 }
 
+static void raw_probe_max_write_zeroes(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    struct stat st;
+
+    if (fstat(s->fd, &st) < 0) {
+        return; /* no problem, keep default value */
+    }
+    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
+        return;
+    }
+    bs->bl.max_write_zeroes = INT_MAX;
+}
+
 static void raw_parse_flags(int bdrv_flags, int *open_flags)
 {
     assert(open_flags != NULL);
@@ -600,6 +614,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     /* Fail already reopen_prepare() if we can't get a working O_DIRECT
      * alignment with the new fd. */
     if (raw_s->fd != -1) {
+        raw_probe_max_write_zeroes(state->bs);
         raw_probe_alignment(state->bs, raw_s->fd, &local_err);
         if (local_err) {
             qemu_close(raw_s->fd);
@@ -653,6 +668,8 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 
     raw_probe_alignment(bs, s->fd, errp);
     bs->bl.opt_mem_alignment = s->buf_align;
+
+    raw_probe_max_write_zeroes(bs);
 }
 
 static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/7] block/raw-posix: create translate_err helper to merge errno values

[Peter Lieven]

Am 30.01.2015 um 09:42 schrieb Denis V. Lunev:
> actually the code
>     if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
>         ret == -ENOTTY) {
>         ret = -ENOTSUP;
>     }
> is present twice and will be added a couple more times. Create helper
> for this.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Peter Lieven <pl@kamp.de>
> CC: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e51293a..24300d0 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -893,6 +893,15 @@ static int xfs_discard(BDRVRawState *s, int64_t offset, uint64_t bytes)
>  }
>  #endif
>  
> +static int translate_err(int err)
> +{
> +    if (err == -ENODEV || err == -ENOSYS || err == -EOPNOTSUPP ||
> +        err == -ENOTTY) {
> +        err = -ENOTSUP;
> +    }
> +    return err;
> +}
> +
>  static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>  {
>      int ret = -EOPNOTSUPP;
> @@ -921,10 +930,9 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>  #endif
>      }
>  
> -    if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
> -        ret == -ENOTTY) {
> +    ret = translate_err(ret);
> +    if (ret == -ENOTSUP) {
>          s->has_write_zeroes = false;
> -        ret = -ENOTSUP;
>      }
>      return ret;
>  }
> @@ -968,10 +976,9 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
>  #endif
>      }
>  
> -    if (ret == -ENODEV || ret == -ENOSYS || ret == -EOPNOTSUPP ||
> -        ret == -ENOTTY) {
> +    ret = translate_err(ret);
> +    if (ret == -ENOTSUP) {
>          s->has_discard = false;
> -        ret = -ENOTSUP;
>      }
>      return ret;
>  }

Reviewed-by: Peter Lieven <pl@kamp.de>

Re: [Qemu-devel] [PATCH 2/7] block/raw-posix: create do_fallocate helper

[Peter Lieven]

Am 30.01.2015 um 09:42 schrieb Denis V. Lunev:
> The pattern
>     do {
>         if (fallocate(s->fd, mode, offset, len) == 0) {
>             return 0;
>         }
>     } while (errno == EINTR);
>     ret = translate_err(-errno);
> will be commonly useful in next patches. Create helper for it.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Peter Lieven <pl@kamp.de>
> CC: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 24300d0..2aa268a 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -902,6 +902,18 @@ static int translate_err(int err)
>      return err;
>  }
>  
> +#if defined(CONFIG_FALLOCATE_PUNCH_HOLE)
> +static int do_fallocate(int fd, int mode, off_t offset, off_t len)
> +{
> +    do {
> +        if (fallocate(fd, mode, offset, len) == 0) {
> +            return 0;
> +        }
> +    } while (errno == EINTR);
> +    return translate_err(-errno);
> +}
> +#endif
> +
>  static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>  {
>      int ret = -EOPNOTSUPP;
> @@ -965,14 +977,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
>  #endif
>  
>  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> -        do {
> -            if (fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -                          aiocb->aio_offset, aiocb->aio_nbytes) == 0) {
> -                return 0;
> -            }
> -        } while (errno == EINTR);
> -
> -        ret = -errno;
> +        ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> +                           aiocb->aio_offset, aiocb->aio_nbytes);

You also introduce the conversion via translate_err here. Is that ok?

Peter

Re: [Qemu-devel] [PATCH 2/7] block/raw-posix: create do_fallocate helper

[Denis V. Lunev]

On 30/01/15 11:47, Peter Lieven wrote:
> Am 30.01.2015 um 09:42 schrieb Denis V. Lunev:
>> The pattern
>>      do {
>>          if (fallocate(s->fd, mode, offset, len) == 0) {
>>              return 0;
>>          }
>>      } while (errno == EINTR);
>>      ret = translate_err(-errno);
>> will be commonly useful in next patches. Create helper for it.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Peter Lieven <pl@kamp.de>
>> CC: Fam Zheng <famz@redhat.com>
>> ---
>>   block/raw-posix.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 24300d0..2aa268a 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -902,6 +902,18 @@ static int translate_err(int err)
>>       return err;
>>   }
>>   
>> +#if defined(CONFIG_FALLOCATE_PUNCH_HOLE)
>> +static int do_fallocate(int fd, int mode, off_t offset, off_t len)
>> +{
>> +    do {
>> +        if (fallocate(fd, mode, offset, len) == 0) {
>> +            return 0;
>> +        }
>> +    } while (errno == EINTR);
>> +    return translate_err(-errno);
>> +}
>> +#endif
>> +
>>   static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>>   {
>>       int ret = -EOPNOTSUPP;
>> @@ -965,14 +977,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
>>   #endif
>>   
>>   #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
>> -        do {
>> -            if (fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
>> -                          aiocb->aio_offset, aiocb->aio_nbytes) == 0) {
>> -                return 0;
>> -            }
>> -        } while (errno == EINTR);
>> -
>> -        ret = -errno;
>> +        ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
>> +                           aiocb->aio_offset, aiocb->aio_nbytes);
> You also introduce the conversion via translate_err here. Is that ok?
>
> Peter
>
yes, this is redundant but harmless

Re: [Qemu-devel] [PATCH 2/7] block/raw-posix: create do_fallocate helper

[Peter Lieven]

Am 30.01.2015 um 09:49 schrieb Denis V. Lunev:
> On 30/01/15 11:47, Peter Lieven wrote:
>> Am 30.01.2015 um 09:42 schrieb Denis V. Lunev:
>>> The pattern
>>>      do {
>>>          if (fallocate(s->fd, mode, offset, len) == 0) {
>>>              return 0;
>>>          }
>>>      } while (errno == EINTR);
>>>      ret = translate_err(-errno);
>>> will be commonly useful in next patches. Create helper for it.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Peter Lieven <pl@kamp.de>
>>> CC: Fam Zheng <famz@redhat.com>
>>> ---
>>>   block/raw-posix.c | 22 ++++++++++++++--------
>>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>> index 24300d0..2aa268a 100644
>>> --- a/block/raw-posix.c
>>> +++ b/block/raw-posix.c
>>> @@ -902,6 +902,18 @@ static int translate_err(int err)
>>>       return err;
>>>   }
>>>   +#if defined(CONFIG_FALLOCATE_PUNCH_HOLE)
>>> +static int do_fallocate(int fd, int mode, off_t offset, off_t len)
>>> +{
>>> +    do {
>>> +        if (fallocate(fd, mode, offset, len) == 0) {
>>> +            return 0;
>>> +        }
>>> +    } while (errno == EINTR);
>>> +    return translate_err(-errno);
>>> +}
>>> +#endif
>>> +
>>>   static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>>>   {
>>>       int ret = -EOPNOTSUPP;
>>> @@ -965,14 +977,8 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
>>>   #endif
>>>     #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
>>> -        do {
>>> -            if (fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
>>> -                          aiocb->aio_offset, aiocb->aio_nbytes) == 0) {
>>> -                return 0;
>>> -            }
>>> -        } while (errno == EINTR);
>>> -
>>> -        ret = -errno;
>>> +        ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
>>> +                           aiocb->aio_offset, aiocb->aio_nbytes);
>> You also introduce the conversion via translate_err here. Is that ok?
>>
>> Peter
>>
> yes, this is redundant but harmless

Ok, then please add:

Reviewed-by: Peter Lieven <pl@kamp.de>

Re: [Qemu-devel] [PATCH 5/7] block/raw-posix: call plain fallocate in handle_aiocb_write_zeroes

[Max Reitz]

On 2015-01-30 at 03:42, Denis V. Lunev wrote:
> There is a possibility that we are extending our image and thus writing
> zeroes beyond the end of the file. In this case we do not need to care
> about the hole to make sure that there is no data in the file under
> this offset (pre-condition to fallocate(0) to work). We could simply call
> fallocate(0).
>
> This improves the performance of writing zeroes even on really old
> platforms which do not have even FALLOC_FL_PUNCH_HOLE.
>
> Before the patch do_fallocate was used when either
> CONFIG_FALLOCATE_PUNCH_HOLE or CONFIG_FALLOCATE_ZERO_RANGE are defined.
> Now the story is different. CONFIG_FALLOCATE is defined when Linux
> fallocate is defined, posix_fallocate is completely different story
> (CONFIG_POSIX_FALLOCATE). CONFIG_FALLOCATE is mandatory prerequite
> for both CONFIG_FALLOCATE_PUNCH_HOLE and CONFIG_FALLOCATE_ZERO_RANGE
> thus we are on the safe side.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Peter Lieven <pl@kamp.de>
> CC: Fam Zheng <famz@redhat.com>
> ---
>   block/raw-posix.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 5a777e7..1c88ad8 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -147,6 +147,7 @@ typedef struct BDRVRawState {
>       bool has_discard:1;
>       bool has_write_zeroes:1;
>       bool discard_zeroes:1;
> +    bool has_fallocate;
>       bool needs_alignment;
>   } BDRVRawState;
>   
> @@ -452,6 +453,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>       }
>       if (S_ISREG(st.st_mode)) {
>           s->discard_zeroes = true;
> +        s->has_fallocate = true;

This could be moved upwards where has_discard and has_write_zeroes are 
initialized; but it won't matter in practice, I hope. Thus:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>       }
>       if (S_ISBLK(st.st_mode)) {
>   #ifdef BLKDISCARDZEROES
> @@ -902,7 +904,7 @@ static int translate_err(int err)
>       return err;
>   }
>   
> -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE_ZERO_RANGE)
> +#ifdef CONFIG_FALLOCATE
>   static int do_fallocate(int fd, int mode, off_t offset, off_t len)
>   {
>       do {
> @@ -965,6 +967,16 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
>       }
>   #endif
>   
> +#ifdef CONFIG_FALLOCATE
> +    if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
> +        int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
> +        if (ret == 0 || ret != -ENOTSUP) {
> +            return ret;
> +        }
> +        s->has_fallocate = false;
> +    }
> +#endif
> +
>       return -ENOTSUP;
>   }
>   

Re: [Qemu-devel] [PATCH 6/7] block: use fallocate(FALLOC_FL_PUNCH_HOLE) & fallocate(0) to write zeroes

[Max Reitz]

On 2015-01-30 at 03:42, Denis V. Lunev wrote:
> This sequence works efficiently if FALLOC_FL_ZERO_RANGE is not supported.
> Unfortunately, FALLOC_FL_ZERO_RANGE is supported on really modern systems
> and only for a couple of filesystems. FALLOC_FL_PUNCH_HOLE is much more
> mature.
>
> The sequence of 2 operations FALLOC_FL_PUNCH_HOLE and 0 is necessary due
> to the following reasons:
> - FALLOC_FL_PUNCH_HOLE creates a hole in the file, the file becomes
>    sparse. In order to retain original functionality we must allocate
>    disk space afterwards. This is done using fallocate(0) call
> - fallocate(0) without preceeding FALLOC_FL_PUNCH_HOLE will do nothing
>    if called above already allocated areas of the file, i.e. the content
>    will not be zeroed
>
> This should increase the performance a bit for not-so-modern kernels.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Peter Lieven <pl@kamp.de>
> CC: Fam Zheng <famz@redhat.com>
> ---
>   block/raw-posix.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 5/7] block/raw-posix: call plain fallocate in handle_aiocb_write_zeroes

[Denis V. Lunev]

On 30/01/15 17:58, Max Reitz wrote:
> On 2015-01-30 at 03:42, Denis V. Lunev wrote:
>> There is a possibility that we are extending our image and thus writing
>> zeroes beyond the end of the file. In this case we do not need to care
>> about the hole to make sure that there is no data in the file under
>> this offset (pre-condition to fallocate(0) to work). We could simply 
>> call
>> fallocate(0).
>>
>> This improves the performance of writing zeroes even on really old
>> platforms which do not have even FALLOC_FL_PUNCH_HOLE.
>>
>> Before the patch do_fallocate was used when either
>> CONFIG_FALLOCATE_PUNCH_HOLE or CONFIG_FALLOCATE_ZERO_RANGE are defined.
>> Now the story is different. CONFIG_FALLOCATE is defined when Linux
>> fallocate is defined, posix_fallocate is completely different story
>> (CONFIG_POSIX_FALLOCATE). CONFIG_FALLOCATE is mandatory prerequite
>> for both CONFIG_FALLOCATE_PUNCH_HOLE and CONFIG_FALLOCATE_ZERO_RANGE
>> thus we are on the safe side.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Max Reitz <mreitz@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Peter Lieven <pl@kamp.de>
>> CC: Fam Zheng <famz@redhat.com>
>> ---
>>   block/raw-posix.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 5a777e7..1c88ad8 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -147,6 +147,7 @@ typedef struct BDRVRawState {
>>       bool has_discard:1;
>>       bool has_write_zeroes:1;
>>       bool discard_zeroes:1;
>> +    bool has_fallocate;
>>       bool needs_alignment;
>>   } BDRVRawState;
>>   @@ -452,6 +453,7 @@ static int raw_open_common(BlockDriverState 
>> *bs, QDict *options,
>>       }
>>       if (S_ISREG(st.st_mode)) {
>>           s->discard_zeroes = true;
>> +        s->has_fallocate = true;
>
> This could be moved upwards where has_discard and has_write_zeroes are 
> initialized; but it won't matter in practice, I hope. Thus:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

This does matter as has_discard and has_write_zeroes are bit fields
thus I can not insert something useful into the middle of those
fields.

Re: [Qemu-devel] [PATCH 5/7] block/raw-posix: call plain fallocate in handle_aiocb_write_zeroes

[Max Reitz]

On 2015-01-30 at 10:41, Denis V. Lunev wrote:
> On 30/01/15 17:58, Max Reitz wrote:
>> On 2015-01-30 at 03:42, Denis V. Lunev wrote:
>>> There is a possibility that we are extending our image and thus writing
>>> zeroes beyond the end of the file. In this case we do not need to care
>>> about the hole to make sure that there is no data in the file under
>>> this offset (pre-condition to fallocate(0) to work). We could simply 
>>> call
>>> fallocate(0).
>>>
>>> This improves the performance of writing zeroes even on really old
>>> platforms which do not have even FALLOC_FL_PUNCH_HOLE.
>>>
>>> Before the patch do_fallocate was used when either
>>> CONFIG_FALLOCATE_PUNCH_HOLE or CONFIG_FALLOCATE_ZERO_RANGE are defined.
>>> Now the story is different. CONFIG_FALLOCATE is defined when Linux
>>> fallocate is defined, posix_fallocate is completely different story
>>> (CONFIG_POSIX_FALLOCATE). CONFIG_FALLOCATE is mandatory prerequite
>>> for both CONFIG_FALLOCATE_PUNCH_HOLE and CONFIG_FALLOCATE_ZERO_RANGE
>>> thus we are on the safe side.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Max Reitz <mreitz@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Peter Lieven <pl@kamp.de>
>>> CC: Fam Zheng <famz@redhat.com>
>>> ---
>>>   block/raw-posix.c | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>> index 5a777e7..1c88ad8 100644
>>> --- a/block/raw-posix.c
>>> +++ b/block/raw-posix.c
>>> @@ -147,6 +147,7 @@ typedef struct BDRVRawState {
>>>       bool has_discard:1;
>>>       bool has_write_zeroes:1;
>>>       bool discard_zeroes:1;
>>> +    bool has_fallocate;
>>>       bool needs_alignment;
>>>   } BDRVRawState;
>>>   @@ -452,6 +453,7 @@ static int raw_open_common(BlockDriverState 
>>> *bs, QDict *options,
>>>       }
>>>       if (S_ISREG(st.st_mode)) {
>>>           s->discard_zeroes = true;
>>> +        s->has_fallocate = true;
>>
>> This could be moved upwards where has_discard and has_write_zeroes 
>> are initialized; but it won't matter in practice, I hope. Thus:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> This does matter as has_discard and has_write_zeroes are bit fields
> thus I can not insert something useful into the middle of those
> fields.

Right, but I did not mean the placement inside of the structure but the 
placement of the initialization statement (s->has_fallocate = true) in 
raw_open_common().

Max

Re: [Qemu-devel] [PATCH 5/7] block/raw-posix: call plain fallocate in handle_aiocb_write_zeroes

[Denis V. Lunev]

On 30/01/15 18:42, Max Reitz wrote:
> On 2015-01-30 at 10:41, Denis V. Lunev wrote:
>> On 30/01/15 17:58, Max Reitz wrote:
>>> On 2015-01-30 at 03:42, Denis V. Lunev wrote:
>>>> There is a possibility that we are extending our image and thus 
>>>> writing
>>>> zeroes beyond the end of the file. In this case we do not need to care
>>>> about the hole to make sure that there is no data in the file under
>>>> this offset (pre-condition to fallocate(0) to work). We could 
>>>> simply call
>>>> fallocate(0).
>>>>
>>>> This improves the performance of writing zeroes even on really old
>>>> platforms which do not have even FALLOC_FL_PUNCH_HOLE.
>>>>
>>>> Before the patch do_fallocate was used when either
>>>> CONFIG_FALLOCATE_PUNCH_HOLE or CONFIG_FALLOCATE_ZERO_RANGE are 
>>>> defined.
>>>> Now the story is different. CONFIG_FALLOCATE is defined when Linux
>>>> fallocate is defined, posix_fallocate is completely different story
>>>> (CONFIG_POSIX_FALLOCATE). CONFIG_FALLOCATE is mandatory prerequite
>>>> for both CONFIG_FALLOCATE_PUNCH_HOLE and CONFIG_FALLOCATE_ZERO_RANGE
>>>> thus we are on the safe side.
>>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> CC: Max Reitz <mreitz@redhat.com>
>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>> CC: Peter Lieven <pl@kamp.de>
>>>> CC: Fam Zheng <famz@redhat.com>
>>>> ---
>>>>   block/raw-posix.c | 14 +++++++++++++-
>>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>>> index 5a777e7..1c88ad8 100644
>>>> --- a/block/raw-posix.c
>>>> +++ b/block/raw-posix.c
>>>> @@ -147,6 +147,7 @@ typedef struct BDRVRawState {
>>>>       bool has_discard:1;
>>>>       bool has_write_zeroes:1;
>>>>       bool discard_zeroes:1;
>>>> +    bool has_fallocate;
>>>>       bool needs_alignment;
>>>>   } BDRVRawState;
>>>>   @@ -452,6 +453,7 @@ static int raw_open_common(BlockDriverState 
>>>> *bs, QDict *options,
>>>>       }
>>>>       if (S_ISREG(st.st_mode)) {
>>>>           s->discard_zeroes = true;
>>>> +        s->has_fallocate = true;
>>>
>>> This could be moved upwards where has_discard and has_write_zeroes 
>>> are initialized; but it won't matter in practice, I hope. Thus:
>>>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> This does matter as has_discard and has_write_zeroes are bit fields
>> thus I can not insert something useful into the middle of those
>> fields.
>
> Right, but I did not mean the placement inside of the structure but 
> the placement of the initialization statement (s->has_fallocate = 
> true) in raw_open_common().
>
> Max
hmm, you are right. This is possible but I don't want
to have this bit set for block/character etc devices
even if they are not using this bit/code. With my
approach the assignment is made in a way to indicate
application area.

Thank you for a review :) It is somewhat difficult
to obtain feedback here in comparison with Linux
kernel.

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Kevin Wolf]

Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
> fallocate() works fine and could handle properly with arbitrary size
> requests. There is no sense to reduce the amount of space to fallocate.
> The bigger is the size, the better is the performance as the amount of
> journal updates is reduced.
> 
> The patch changes behavior for both generic filesystem and XFS codepaths,
> which are different in handle_aiocb_write_zeroes. The implementation
> of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
> thus the change is fine for both ways.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Peter Lieven <pl@kamp.de>
> CC: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 7b42f37..933c778 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>      }
>  }
>  
> +static void raw_probe_max_write_zeroes(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    struct stat st;
> +
> +    if (fstat(s->fd, &st) < 0) {
> +        return; /* no problem, keep default value */
> +    }
> +    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
> +        return;
> +    }
> +    bs->bl.max_write_zeroes = INT_MAX;
> +}

Peter, do you remember why INT_MAX isn't actually the default? I think
the most reasonable behaviour would be that a limitation is only used if
a block driver requests it, and otherwise unlimited is assumed.

We can take this patch to raw-posix, it is certainly not wrong. But any
format driver or filter will still, in most cases needlessly, apply
MAX_WRITE_ZEROES_DEFAULT, i.e. a 16 MB maximum, so I think we should
consider making a change to the default.

Kevin

Re: [Qemu-devel] [PATCH v6 0/7] eliminate data write in bdrv_write_zeroes on Linux in raw-posix.c

[Kevin Wolf]

Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
> I have performed several tests with non-aligned fallocate calls and
> in all cases (with non-aligned fallocates) Linux performs fine, i.e.
> areas are zeroed correctly. Checks were made on
> Linux 3.16.0-28-generic #38-Ubuntu SMP
> 
> This should seriously increase performance of bdrv_write_zeroes

Thanks, applied all to the block branch.

Note that I'm hoping to remove patch 7/7 from the queue again if we come
to the conclusion that changing the global default instead makes more
sense.

Kevin

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Peter Lieven]

Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
> Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
>> fallocate() works fine and could handle properly with arbitrary size
>> requests. There is no sense to reduce the amount of space to fallocate.
>> The bigger is the size, the better is the performance as the amount of
>> journal updates is reduced.
>>
>> The patch changes behavior for both generic filesystem and XFS codepaths,
>> which are different in handle_aiocb_write_zeroes. The implementation
>> of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
>> thus the change is fine for both ways.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Peter Lieven <pl@kamp.de>
>> CC: Fam Zheng <famz@redhat.com>
>> ---
>>   block/raw-posix.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 7b42f37..933c778 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>       }
>>   }
>>   
>> +static void raw_probe_max_write_zeroes(BlockDriverState *bs)
>> +{
>> +    BDRVRawState *s = bs->opaque;
>> +    struct stat st;
>> +
>> +    if (fstat(s->fd, &st) < 0) {
>> +        return; /* no problem, keep default value */
>> +    }
>> +    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
>> +        return;
>> +    }
>> +    bs->bl.max_write_zeroes = INT_MAX;
>> +}
> Peter, do you remember why INT_MAX isn't actually the default? I think
> the most reasonable behaviour would be that a limitation is only used if
> a block driver requests it, and otherwise unlimited is assumed.

The default (0) actually means unlimited or undefined. We introduced
that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
sized requests because there is no guarantee that write zeroes is a
fast operation. We should set INT_MAX only if we know that write
zeroes of an arbitrary size is always fast.

Peter

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Kevin Wolf]

Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:
> Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
> >Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
> >>fallocate() works fine and could handle properly with arbitrary size
> >>requests. There is no sense to reduce the amount of space to fallocate.
> >>The bigger is the size, the better is the performance as the amount of
> >>journal updates is reduced.
> >>
> >>The patch changes behavior for both generic filesystem and XFS codepaths,
> >>which are different in handle_aiocb_write_zeroes. The implementation
> >>of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
> >>thus the change is fine for both ways.
> >>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>CC: Kevin Wolf <kwolf@redhat.com>
> >>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>CC: Peter Lieven <pl@kamp.de>
> >>CC: Fam Zheng <famz@redhat.com>
> >>---
> >>  block/raw-posix.c | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >>diff --git a/block/raw-posix.c b/block/raw-posix.c
> >>index 7b42f37..933c778 100644
> >>--- a/block/raw-posix.c
> >>+++ b/block/raw-posix.c
> >>@@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> >>      }
> >>  }
> >>+static void raw_probe_max_write_zeroes(BlockDriverState *bs)
> >>+{
> >>+    BDRVRawState *s = bs->opaque;
> >>+    struct stat st;
> >>+
> >>+    if (fstat(s->fd, &st) < 0) {
> >>+        return; /* no problem, keep default value */
> >>+    }
> >>+    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
> >>+        return;
> >>+    }
> >>+    bs->bl.max_write_zeroes = INT_MAX;
> >>+}
> >Peter, do you remember why INT_MAX isn't actually the default? I think
> >the most reasonable behaviour would be that a limitation is only used if
> >a block driver requests it, and otherwise unlimited is assumed.
> 
> The default (0) actually means unlimited or undefined. We introduced
> that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
> sized requests because there is no guarantee that write zeroes is a
> fast operation. We should set INT_MAX only if we know that write
> zeroes of an arbitrary size is always fast.

Well, splitting it up doesn't make it any faster. I think we can assume
that drv->bdrv_co_write_zeroes() wants to know the full request size
unless the driver has explicitly set bs->bl.max_write_zeroes.

Only if we go on emulating the operation with a zero-filled buffer, I
understand that we might need to split it up so that our bounce buffer
doesn't become huge.

Kevin

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Peter Lieven]

Am 02.02.2015 um 15:04 schrieb Kevin Wolf:
> Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:
>> Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
>>> Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
>>>> fallocate() works fine and could handle properly with arbitrary size
>>>> requests. There is no sense to reduce the amount of space to fallocate.
>>>> The bigger is the size, the better is the performance as the amount of
>>>> journal updates is reduced.
>>>>
>>>> The patch changes behavior for both generic filesystem and XFS codepaths,
>>>> which are different in handle_aiocb_write_zeroes. The implementation
>>>> of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
>>>> thus the change is fine for both ways.
>>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>> CC: Peter Lieven <pl@kamp.de>
>>>> CC: Fam Zheng <famz@redhat.com>
>>>> ---
>>>>   block/raw-posix.c | 17 +++++++++++++++++
>>>>   1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>>> index 7b42f37..933c778 100644
>>>> --- a/block/raw-posix.c
>>>> +++ b/block/raw-posix.c
>>>> @@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>>>       }
>>>>   }
>>>> +static void raw_probe_max_write_zeroes(BlockDriverState *bs)
>>>> +{
>>>> +    BDRVRawState *s = bs->opaque;
>>>> +    struct stat st;
>>>> +
>>>> +    if (fstat(s->fd, &st) < 0) {
>>>> +        return; /* no problem, keep default value */
>>>> +    }
>>>> +    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
>>>> +        return;
>>>> +    }
>>>> +    bs->bl.max_write_zeroes = INT_MAX;
>>>> +}
>>> Peter, do you remember why INT_MAX isn't actually the default? I think
>>> the most reasonable behaviour would be that a limitation is only used if
>>> a block driver requests it, and otherwise unlimited is assumed.
>> The default (0) actually means unlimited or undefined. We introduced
>> that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
>> sized requests because there is no guarantee that write zeroes is a
>> fast operation. We should set INT_MAX only if we know that write
>> zeroes of an arbitrary size is always fast.
> Well, splitting it up doesn't make it any faster. I think we can assume
> that drv->bdrv_co_write_zeroes() wants to know the full request size
> unless the driver has explicitly set bs->bl.max_write_zeroes.

You mean sth like this:

diff --git a/block.c b/block.c
index 61412e9..8272ef9 100644
--- a/block.c
+++ b/block.c
@@ -3192,10 +3192,7 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
                              BDRV_REQ_COPY_ON_READ);
  }

-/* if no limit is specified in the BlockLimits use a default
- * of 32768 512-byte sectors (16 MiB) per request.
- */
-#define MAX_WRITE_ZEROES_DEFAULT 32768
+#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768

  static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
      int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
@@ -3206,7 +3203,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
      int ret = 0;

      int max_write_zeroes = bs->bl.max_write_zeroes ?
-                           bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT;
+                           bs->bl.max_write_zeroes : INT_MAX;

      while (nb_sectors > 0 && !ret) {
          int num = nb_sectors;
@@ -3242,7 +3239,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
          if (ret == -ENOTSUP) {
              /* Fall back to bounce buffer if write zeroes is unsupported */
              int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
- MAX_WRITE_ZEROES_DEFAULT);
+ MAX_WRITE_ZEROES_BOUNCE_BUFFER);
              num = MIN(num, max_xfer_len);
              iov.iov_len = num * BDRV_SECTOR_SIZE;
              if (iov.iov_base == NULL) {
@@ -5099,11 +5096,6 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
      rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
  }

-/* if no limit is specified in the BlockLimits use a default
- * of 32768 512-byte sectors (16 MiB) per request.
- */
-#define MAX_DISCARD_DEFAULT 32768
-
  int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
                                   int nb_sectors)
  {
@@ -5128,7 +5120,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
          return 0;
      }

-    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : MAX_DISCARD_DEFAULT;
+    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
      while (nb_sectors > 0) {
          int ret;
          int num = nb_sectors;



Peter

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Kevin Wolf]

Am 02.02.2015 um 15:12 hat Peter Lieven geschrieben:
> Am 02.02.2015 um 15:04 schrieb Kevin Wolf:
> >Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:
> >>Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
> >>>Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
> >>>>fallocate() works fine and could handle properly with arbitrary size
> >>>>requests. There is no sense to reduce the amount of space to fallocate.
> >>>>The bigger is the size, the better is the performance as the amount of
> >>>>journal updates is reduced.
> >>>>
> >>>>The patch changes behavior for both generic filesystem and XFS codepaths,
> >>>>which are different in handle_aiocb_write_zeroes. The implementation
> >>>>of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
> >>>>thus the change is fine for both ways.
> >>>>
> >>>>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>>>Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>>>CC: Kevin Wolf <kwolf@redhat.com>
> >>>>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>>>CC: Peter Lieven <pl@kamp.de>
> >>>>CC: Fam Zheng <famz@redhat.com>
> >>>>---
> >>>>  block/raw-posix.c | 17 +++++++++++++++++
> >>>>  1 file changed, 17 insertions(+)
> >>>>
> >>>>diff --git a/block/raw-posix.c b/block/raw-posix.c
> >>>>index 7b42f37..933c778 100644
> >>>>--- a/block/raw-posix.c
> >>>>+++ b/block/raw-posix.c
> >>>>@@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> >>>>      }
> >>>>  }
> >>>>+static void raw_probe_max_write_zeroes(BlockDriverState *bs)
> >>>>+{
> >>>>+    BDRVRawState *s = bs->opaque;
> >>>>+    struct stat st;
> >>>>+
> >>>>+    if (fstat(s->fd, &st) < 0) {
> >>>>+        return; /* no problem, keep default value */
> >>>>+    }
> >>>>+    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
> >>>>+        return;
> >>>>+    }
> >>>>+    bs->bl.max_write_zeroes = INT_MAX;
> >>>>+}
> >>>Peter, do you remember why INT_MAX isn't actually the default? I think
> >>>the most reasonable behaviour would be that a limitation is only used if
> >>>a block driver requests it, and otherwise unlimited is assumed.
> >>The default (0) actually means unlimited or undefined. We introduced
> >>that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
> >>sized requests because there is no guarantee that write zeroes is a
> >>fast operation. We should set INT_MAX only if we know that write
> >>zeroes of an arbitrary size is always fast.
> >Well, splitting it up doesn't make it any faster. I think we can assume
> >that drv->bdrv_co_write_zeroes() wants to know the full request size
> >unless the driver has explicitly set bs->bl.max_write_zeroes.
> 
> You mean sth like this:

Yes, I think that's what I meant.

Kevin

> diff --git a/block.c b/block.c
> index 61412e9..8272ef9 100644
> --- a/block.c
> +++ b/block.c
> @@ -3192,10 +3192,7 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
>                              BDRV_REQ_COPY_ON_READ);
>  }
> 
> -/* if no limit is specified in the BlockLimits use a default
> - * of 32768 512-byte sectors (16 MiB) per request.
> - */
> -#define MAX_WRITE_ZEROES_DEFAULT 32768
> +#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
> 
>  static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>      int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
> @@ -3206,7 +3203,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>      int ret = 0;
> 
>      int max_write_zeroes = bs->bl.max_write_zeroes ?
> -                           bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT;
> +                           bs->bl.max_write_zeroes : INT_MAX;
> 
>      while (nb_sectors > 0 && !ret) {
>          int num = nb_sectors;
> @@ -3242,7 +3239,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>          if (ret == -ENOTSUP) {
>              /* Fall back to bounce buffer if write zeroes is unsupported */
>              int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
> - MAX_WRITE_ZEROES_DEFAULT);
> + MAX_WRITE_ZEROES_BOUNCE_BUFFER);
>              num = MIN(num, max_xfer_len);
>              iov.iov_len = num * BDRV_SECTOR_SIZE;
>              if (iov.iov_base == NULL) {
> @@ -5099,11 +5096,6 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
>      rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
>  }
> 
> -/* if no limit is specified in the BlockLimits use a default
> - * of 32768 512-byte sectors (16 MiB) per request.
> - */
> -#define MAX_DISCARD_DEFAULT 32768
> -
>  int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>                                   int nb_sectors)
>  {
> @@ -5128,7 +5120,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>          return 0;
>      }
> 
> -    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : MAX_DISCARD_DEFAULT;
> +    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
>      while (nb_sectors > 0) {
>          int ret;
>          int num = nb_sectors;
> 
> 
> 
> Peter

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Peter Lieven]

Am 02.02.2015 um 15:16 schrieb Kevin Wolf:
> Am 02.02.2015 um 15:12 hat Peter Lieven geschrieben:
>> Am 02.02.2015 um 15:04 schrieb Kevin Wolf:
>>> Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:
>>>> Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
>>>>> Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
>>>>>> fallocate() works fine and could handle properly with arbitrary size
>>>>>> requests. There is no sense to reduce the amount of space to fallocate.
>>>>>> The bigger is the size, the better is the performance as the amount of
>>>>>> journal updates is reduced.
>>>>>>
>>>>>> The patch changes behavior for both generic filesystem and XFS codepaths,
>>>>>> which are different in handle_aiocb_write_zeroes. The implementation
>>>>>> of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
>>>>>> thus the change is fine for both ways.
>>>>>>
>>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>> CC: Peter Lieven <pl@kamp.de>
>>>>>> CC: Fam Zheng <famz@redhat.com>
>>>>>> ---
>>>>>>   block/raw-posix.c | 17 +++++++++++++++++
>>>>>>   1 file changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>>>>> index 7b42f37..933c778 100644
>>>>>> --- a/block/raw-posix.c
>>>>>> +++ b/block/raw-posix.c
>>>>>> @@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>>>>>       }
>>>>>>   }
>>>>>> +static void raw_probe_max_write_zeroes(BlockDriverState *bs)
>>>>>> +{
>>>>>> +    BDRVRawState *s = bs->opaque;
>>>>>> +    struct stat st;
>>>>>> +
>>>>>> +    if (fstat(s->fd, &st) < 0) {
>>>>>> +        return; /* no problem, keep default value */
>>>>>> +    }
>>>>>> +    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +    bs->bl.max_write_zeroes = INT_MAX;
>>>>>> +}
>>>>> Peter, do you remember why INT_MAX isn't actually the default? I think
>>>>> the most reasonable behaviour would be that a limitation is only used if
>>>>> a block driver requests it, and otherwise unlimited is assumed.
>>>> The default (0) actually means unlimited or undefined. We introduced
>>>> that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
>>>> sized requests because there is no guarantee that write zeroes is a
>>>> fast operation. We should set INT_MAX only if we know that write
>>>> zeroes of an arbitrary size is always fast.
>>> Well, splitting it up doesn't make it any faster. I think we can assume
>>> that drv->bdrv_co_write_zeroes() wants to know the full request size
>>> unless the driver has explicitly set bs->bl.max_write_zeroes.
>> You mean sth like this:
> Yes, I think that's what I meant.

I can't find the original discussion why we added this limit. It was actually the default
before we introduced BlockLimits. And, it was also the default in the unsupported path
of write zeroes which created big memory allocations. This might be the reason why
we introduced a limit.

Peter

>
> Kevin
>
>> diff --git a/block.c b/block.c
>> index 61412e9..8272ef9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3192,10 +3192,7 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
>>                               BDRV_REQ_COPY_ON_READ);
>>   }
>>
>> -/* if no limit is specified in the BlockLimits use a default
>> - * of 32768 512-byte sectors (16 MiB) per request.
>> - */
>> -#define MAX_WRITE_ZEROES_DEFAULT 32768
>> +#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
>>
>>   static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>       int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
>> @@ -3206,7 +3203,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>       int ret = 0;
>>
>>       int max_write_zeroes = bs->bl.max_write_zeroes ?
>> -                           bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT;
>> +                           bs->bl.max_write_zeroes : INT_MAX;
>>
>>       while (nb_sectors > 0 && !ret) {
>>           int num = nb_sectors;
>> @@ -3242,7 +3239,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>           if (ret == -ENOTSUP) {
>>               /* Fall back to bounce buffer if write zeroes is unsupported */
>>               int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
>> - MAX_WRITE_ZEROES_DEFAULT);
>> + MAX_WRITE_ZEROES_BOUNCE_BUFFER);
>>               num = MIN(num, max_xfer_len);
>>               iov.iov_len = num * BDRV_SECTOR_SIZE;
>>               if (iov.iov_base == NULL) {
>> @@ -5099,11 +5096,6 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
>>       rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
>>   }
>>
>> -/* if no limit is specified in the BlockLimits use a default
>> - * of 32768 512-byte sectors (16 MiB) per request.
>> - */
>> -#define MAX_DISCARD_DEFAULT 32768
>> -
>>   int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>                                    int nb_sectors)
>>   {
>> @@ -5128,7 +5120,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>           return 0;
>>       }
>>
>> -    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : MAX_DISCARD_DEFAULT;
>> +    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
>>       while (nb_sectors > 0) {
>>           int ret;
>>           int num = nb_sectors;
>>
>>
>>
>> Peter

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Denis V. Lunev]

On 02/02/15 17:20, Peter Lieven wrote:
> Am 02.02.2015 um 15:16 schrieb Kevin Wolf:
>> Am 02.02.2015 um 15:12 hat Peter Lieven geschrieben:
>>> Am 02.02.2015 um 15:04 schrieb Kevin Wolf:
>>>> Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:
>>>>> Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
>>>>>> Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
>>>>>>> fallocate() works fine and could handle properly with arbitrary 
>>>>>>> size
>>>>>>> requests. There is no sense to reduce the amount of space to 
>>>>>>> fallocate.
>>>>>>> The bigger is the size, the better is the performance as the 
>>>>>>> amount of
>>>>>>> journal updates is reduced.
>>>>>>>
>>>>>>> The patch changes behavior for both generic filesystem and XFS 
>>>>>>> codepaths,
>>>>>>> which are different in handle_aiocb_write_zeroes. The 
>>>>>>> implementation
>>>>>>> of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly 
>>>>>>> the same
>>>>>>> thus the change is fine for both ways.
>>>>>>>
>>>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>>> CC: Peter Lieven <pl@kamp.de>
>>>>>>> CC: Fam Zheng <famz@redhat.com>
>>>>>>> ---
>>>>>>>   block/raw-posix.c | 17 +++++++++++++++++
>>>>>>>   1 file changed, 17 insertions(+)
>>>>>>>
>>>>>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>>>>>> index 7b42f37..933c778 100644
>>>>>>> --- a/block/raw-posix.c
>>>>>>> +++ b/block/raw-posix.c
>>>>>>> @@ -293,6 +293,20 @@ static void 
>>>>>>> raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>>>>>>       }
>>>>>>>   }
>>>>>>> +static void raw_probe_max_write_zeroes(BlockDriverState *bs)
>>>>>>> +{
>>>>>>> +    BDRVRawState *s = bs->opaque;
>>>>>>> +    struct stat st;
>>>>>>> +
>>>>>>> +    if (fstat(s->fd, &st) < 0) {
>>>>>>> +        return; /* no problem, keep default value */
>>>>>>> +    }
>>>>>>> +    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +    bs->bl.max_write_zeroes = INT_MAX;
>>>>>>> +}
>>>>>> Peter, do you remember why INT_MAX isn't actually the default? I 
>>>>>> think
>>>>>> the most reasonable behaviour would be that a limitation is only 
>>>>>> used if
>>>>>> a block driver requests it, and otherwise unlimited is assumed.
>>>>> The default (0) actually means unlimited or undefined. We introduced
>>>>> that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
>>>>> sized requests because there is no guarantee that write zeroes is a
>>>>> fast operation. We should set INT_MAX only if we know that write
>>>>> zeroes of an arbitrary size is always fast.
>>>> Well, splitting it up doesn't make it any faster. I think we can 
>>>> assume
>>>> that drv->bdrv_co_write_zeroes() wants to know the full request size
>>>> unless the driver has explicitly set bs->bl.max_write_zeroes.
>>> You mean sth like this:
>> Yes, I think that's what I meant.
>
> I can't find the original discussion why we added this limit. It was 
> actually the default
> before we introduced BlockLimits. And, it was also the default in the 
> unsupported path
> of write zeroes which created big memory allocations. This might be 
> the reason why
> we introduced a limit.
>
> Peter
>
my $0.02 here is that even if the patch below adds regression
(though I can not imagine how at the moment after some
checking), we should fix bogus driver.

Personally I do not like such unnatural limitations.

Den

>>
>> Kevin
>>
>>> diff --git a/block.c b/block.c
>>> index 61412e9..8272ef9 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3192,10 +3192,7 @@ int coroutine_fn 
>>> bdrv_co_copy_on_readv(BlockDriverState *bs,
>>>                               BDRV_REQ_COPY_ON_READ);
>>>   }
>>>
>>> -/* if no limit is specified in the BlockLimits use a default
>>> - * of 32768 512-byte sectors (16 MiB) per request.
>>> - */
>>> -#define MAX_WRITE_ZEROES_DEFAULT 32768
>>> +#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
>>>
>>>   static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>>       int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
>>> @@ -3206,7 +3203,7 @@ static int coroutine_fn 
>>> bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>>       int ret = 0;
>>>
>>>       int max_write_zeroes = bs->bl.max_write_zeroes ?
>>> -                           bs->bl.max_write_zeroes : 
>>> MAX_WRITE_ZEROES_DEFAULT;
>>> +                           bs->bl.max_write_zeroes : INT_MAX;
>>>
>>>       while (nb_sectors > 0 && !ret) {
>>>           int num = nb_sectors;
>>> @@ -3242,7 +3239,7 @@ static int coroutine_fn 
>>> bdrv_co_do_write_zeroes(BlockDriverState *bs,
>>>           if (ret == -ENOTSUP) {
>>>               /* Fall back to bounce buffer if write zeroes is 
>>> unsupported */
>>>               int max_xfer_len = 
>>> MIN_NON_ZERO(bs->bl.max_transfer_length,
>>> - MAX_WRITE_ZEROES_DEFAULT);
>>> + MAX_WRITE_ZEROES_BOUNCE_BUFFER);
>>>               num = MIN(num, max_xfer_len);
>>>               iov.iov_len = num * BDRV_SECTOR_SIZE;
>>>               if (iov.iov_base == NULL) {
>>> @@ -5099,11 +5096,6 @@ static void coroutine_fn 
>>> bdrv_discard_co_entry(void *opaque)
>>>       rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, 
>>> rwco->nb_sectors);
>>>   }
>>>
>>> -/* if no limit is specified in the BlockLimits use a default
>>> - * of 32768 512-byte sectors (16 MiB) per request.
>>> - */
>>> -#define MAX_DISCARD_DEFAULT 32768
>>> -
>>>   int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t 
>>> sector_num,
>>>                                    int nb_sectors)
>>>   {
>>> @@ -5128,7 +5120,7 @@ int coroutine_fn 
>>> bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>>           return 0;
>>>       }
>>>
>>> -    max_discard = bs->bl.max_discard ? bs->bl.max_discard : 
>>> MAX_DISCARD_DEFAULT;
>>> +    max_discard = bs->bl.max_discard ? bs->bl.max_discard : INT_MAX;
>>>       while (nb_sectors > 0) {
>>>           int ret;
>>>           int num = nb_sectors;
>>>
>>>
>>>
>>> Peter
>
>

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Kevin Wolf]

Am 02.02.2015 um 15:20 hat Peter Lieven geschrieben:
> Am 02.02.2015 um 15:16 schrieb Kevin Wolf:
> >Am 02.02.2015 um 15:12 hat Peter Lieven geschrieben:
> >>Am 02.02.2015 um 15:04 schrieb Kevin Wolf:
> >>>Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:
> >>>>Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
> >>>>>Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
> >>>>>>fallocate() works fine and could handle properly with arbitrary size
> >>>>>>requests. There is no sense to reduce the amount of space to fallocate.
> >>>>>>The bigger is the size, the better is the performance as the amount of
> >>>>>>journal updates is reduced.
> >>>>>>
> >>>>>>The patch changes behavior for both generic filesystem and XFS codepaths,
> >>>>>>which are different in handle_aiocb_write_zeroes. The implementation
> >>>>>>of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
> >>>>>>thus the change is fine for both ways.
> >>>>>>
> >>>>>>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>>>>>Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>>>>>CC: Kevin Wolf <kwolf@redhat.com>
> >>>>>>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>>>>>CC: Peter Lieven <pl@kamp.de>
> >>>>>>CC: Fam Zheng <famz@redhat.com>
> >>>>>>---
> >>>>>>  block/raw-posix.c | 17 +++++++++++++++++
> >>>>>>  1 file changed, 17 insertions(+)
> >>>>>>
> >>>>>>diff --git a/block/raw-posix.c b/block/raw-posix.c
> >>>>>>index 7b42f37..933c778 100644
> >>>>>>--- a/block/raw-posix.c
> >>>>>>+++ b/block/raw-posix.c
> >>>>>>@@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> >>>>>>      }
> >>>>>>  }
> >>>>>>+static void raw_probe_max_write_zeroes(BlockDriverState *bs)
> >>>>>>+{
> >>>>>>+    BDRVRawState *s = bs->opaque;
> >>>>>>+    struct stat st;
> >>>>>>+
> >>>>>>+    if (fstat(s->fd, &st) < 0) {
> >>>>>>+        return; /* no problem, keep default value */
> >>>>>>+    }
> >>>>>>+    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
> >>>>>>+        return;
> >>>>>>+    }
> >>>>>>+    bs->bl.max_write_zeroes = INT_MAX;
> >>>>>>+}
> >>>>>Peter, do you remember why INT_MAX isn't actually the default? I think
> >>>>>the most reasonable behaviour would be that a limitation is only used if
> >>>>>a block driver requests it, and otherwise unlimited is assumed.
> >>>>The default (0) actually means unlimited or undefined. We introduced
> >>>>that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
> >>>>sized requests because there is no guarantee that write zeroes is a
> >>>>fast operation. We should set INT_MAX only if we know that write
> >>>>zeroes of an arbitrary size is always fast.
> >>>Well, splitting it up doesn't make it any faster. I think we can assume
> >>>that drv->bdrv_co_write_zeroes() wants to know the full request size
> >>>unless the driver has explicitly set bs->bl.max_write_zeroes.
> >>You mean sth like this:
> >Yes, I think that's what I meant.
> 
> I can't find the original discussion why we added this limit. It was actually the default
> before we introduced BlockLimits. And, it was also the default in the unsupported path
> of write zeroes which created big memory allocations. This might be the reason why
> we introduced a limit.

Commit c31cb707 added the limit to bdrv_co_do_write_zeroes(). Before, we
used a bounce buffer of unbounded size.

Anyway, it seems that none of us can think of a reason not to apply the
patch to block.c. Let's just do it, and if it does break something,
we'll figure it out. Can you send it as a proper patch?

Denis, if we apply that patch, would you be okay with dropping 7/7 from
this series, or would still something be missing?

Kevin

> >>diff --git a/block.c b/block.c
> >>index 61412e9..8272ef9 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -3192,10 +3192,7 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
> >>                              BDRV_REQ_COPY_ON_READ);
> >>  }
> >>
> >>-/* if no limit is specified in the BlockLimits use a default
> >>- * of 32768 512-byte sectors (16 MiB) per request.
> >>- */
> >>-#define MAX_WRITE_ZEROES_DEFAULT 32768
> >>+#define MAX_WRITE_ZEROES_BOUNCE_BUFFER 32768
> >>
> >>  static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> >>      int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
> >>@@ -3206,7 +3203,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> >>      int ret = 0;
> >>
> >>      int max_write_zeroes = bs->bl.max_write_zeroes ?
> >>-                           bs->bl.max_write_zeroes : MAX_WRITE_ZEROES_DEFAULT;
> >>+                           bs->bl.max_write_zeroes : INT_MAX;
> >>
> >>      while (nb_sectors > 0 && !ret) {
> >>          int num = nb_sectors;
> >>@@ -3242,7 +3239,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> >>          if (ret == -ENOTSUP) {
> >>              /* Fall back to bounce buffer if write zeroes is unsupported */
> >>              int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
> >>- MAX_WRITE_ZEROES_DEFAULT);
> >>+ MAX_WRITE_ZEROES_BOUNCE_BUFFER);
> >>              num = MIN(num, max_xfer_len);
> >>              iov.iov_len = num * BDRV_SECTOR_SIZE;
> >>              if (iov.iov_base == NULL) {
> >>@@ -5099,11 +5096,6 @@ static void coroutine_fn bdrv_discard_co_entry(void *opaque)
> >>      rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num, rwco->nb_sectors);
> >>  }
> >>
> >>-/* if no limit is specified in the BlockLimits use a default
> >>- * of 32768 512-byte sectors (16 MiB) per request.
> >>- */
> >>-#define MAX_DISCARD_DEFAULT 32768
> >>-
> >>  int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
> >>                                   int nb_sectors)
> >>  {
> >>@@ -5128,7 +5120,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
> >>          return 0;
> >>      }
> >>
> >>-    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : MAX_DISCARD_DEFAULT;
> >>+    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
> >>      while (nb_sectors > 0) {
> >>          int ret;
> >>          int num = nb_sectors;
> >>
> >>
> >>
> >>Peter
> 
> 

Re: [Qemu-devel] [PATCH 7/7] block/raw-posix: set max_write_zeroes to INT_MAX for regular files

[Denis V. Lunev]

On 02/02/15 17:49, Kevin Wolf wrote:
> Am 02.02.2015 um 15:20 hat Peter Lieven geschrieben:
>> Am 02.02.2015 um 15:16 schrieb Kevin Wolf:
>>> Am 02.02.2015 um 15:12 hat Peter Lieven geschrieben:
>>>> Am 02.02.2015 um 15:04 schrieb Kevin Wolf:
>>>>> Am 02.02.2015 um 14:55 hat Peter Lieven geschrieben:
>>>>>> Am 02.02.2015 um 14:23 schrieb Kevin Wolf:
>>>>>>> Am 30.01.2015 um 09:42 hat Denis V. Lunev geschrieben:
>>>>>>>> fallocate() works fine and could handle properly with arbitrary size
>>>>>>>> requests. There is no sense to reduce the amount of space to fallocate.
>>>>>>>> The bigger is the size, the better is the performance as the amount of
>>>>>>>> journal updates is reduced.
>>>>>>>>
>>>>>>>> The patch changes behavior for both generic filesystem and XFS codepaths,
>>>>>>>> which are different in handle_aiocb_write_zeroes. The implementation
>>>>>>>> of fallocate and xfsctl(XFS_IOC_ZERO_RANGE) for XFS are exactly the same
>>>>>>>> thus the change is fine for both ways.
>>>>>>>>
>>>>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>>>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>>>> CC: Peter Lieven <pl@kamp.de>
>>>>>>>> CC: Fam Zheng <famz@redhat.com>
>>>>>>>> ---
>>>>>>>>   block/raw-posix.c | 17 +++++++++++++++++
>>>>>>>>   1 file changed, 17 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>>>>>>> index 7b42f37..933c778 100644
>>>>>>>> --- a/block/raw-posix.c
>>>>>>>> +++ b/block/raw-posix.c
>>>>>>>> @@ -293,6 +293,20 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>>>>>>>>       }
>>>>>>>>   }
>>>>>>>> +static void raw_probe_max_write_zeroes(BlockDriverState *bs)
>>>>>>>> +{
>>>>>>>> +    BDRVRawState *s = bs->opaque;
>>>>>>>> +    struct stat st;
>>>>>>>> +
>>>>>>>> +    if (fstat(s->fd, &st) < 0) {
>>>>>>>> +        return; /* no problem, keep default value */
>>>>>>>> +    }
>>>>>>>> +    if (!S_ISREG(st.st_mode) || !s->discard_zeroes) {
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +    bs->bl.max_write_zeroes = INT_MAX;
>>>>>>>> +}
>>>>>>> Peter, do you remember why INT_MAX isn't actually the default? I think
>>>>>>> the most reasonable behaviour would be that a limitation is only used if
>>>>>>> a block driver requests it, and otherwise unlimited is assumed.
>>>>>> The default (0) actually means unlimited or undefined. We introduced
>>>>>> that limit of 16MB in bdrv_co_write_zeroes to create only reasonable
>>>>>> sized requests because there is no guarantee that write zeroes is a
>>>>>> fast operation. We should set INT_MAX only if we know that write
>>>>>> zeroes of an arbitrary size is always fast.
>>>>> Well, splitting it up doesn't make it any faster. I think we can assume
>>>>> that drv->bdrv_co_write_zeroes() wants to know the full request size
>>>>> unless the driver has explicitly set bs->bl.max_write_zeroes.
>>>> You mean sth like this:
>>> Yes, I think that's what I meant.
>> I can't find the original discussion why we added this limit. It was actually the default
>> before we introduced BlockLimits. And, it was also the default in the unsupported path
>> of write zeroes which created big memory allocations. This might be the reason why
>> we introduced a limit.
> Commit c31cb707 added the limit to bdrv_co_do_write_zeroes(). Before, we
> used a bounce buffer of unbounded size.
>
> Anyway, it seems that none of us can think of a reason not to apply the
> patch to block.c. Let's just do it, and if it does break something,
> we'll figure it out. Can you send it as a proper patch?
>
> Denis, if we apply that patch, would you be okay with dropping 7/7 from
> this series, or would still something be missing?
>
> Kevin
Sure. This will be even better. Something similar was implemented in
v1/v2 of the patchset.

Regards,
     Den