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

[Max Reitz]

On 2015-01-27 at 08:51, Denis V. Lunev wrote:
> 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 <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> CC: Peter Lieven <address@hidden>
> CC: Fam Zheng <address@hidden>
> ---
>   block/raw-posix.c | 16 ++++++++++++++--
>   configure         | 19 +++++++++++++++++++
>   2 files changed, 33 insertions(+), 2 deletions(-)

Okay, now the "ret" in handle_aiocb_write_zeroes() is necessary, so 
please disregard my statement about removing it in patch 3.

> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 24e1fab..3c35b2f 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 {
> @@ -955,6 +955,18 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
> *aiocb)
>       }
>   #endif
>   
> +#ifdef CONFIG_FALLOCATE_ZERO_RANGE
> +    if (s->has_write_zeroes) {
> +        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;
> +        return ret;
> +    }

First, you probably want to simply fall through here; right now, you are 
immediately failing with -ENOTSUP on the first call, but falling through 
on the second call. After this patch, it doesn't make a difference, but 
after the next one, it might.

Second, while using s->has_write_zeroes here seems correct to me, I 
personally don't like sharing it with handle_aiocb_write_zeroes_block(); 
and if you do introduce a new flag like "has_zero_range", please don't 
make it a bit field (I will give you an R-b regardless of whether you 
make it a bit field or not, I just won't like it).

Feel free to keep has_write_zeroes, though, while it doesn't look good 
to me it certainly is correct from a technical perspective.

Max