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

[Kevin Wolf <kwolf@redhat.com>]

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