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

[Max Reitz]

On 2015-01-27 at 08:51, 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.
>
> 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 | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index c039bef..fa05239 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
> -#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || 
> defined(CONFIG_FALLOCATE_ZERO_RANGE)
> +#ifdef CONFIG_FALLOCATE

This change doesn't seem right; CONFIG_FALLOCATE is set if 
posix_fallocate() is available, not for the Linux-specific fallocate() 
from linux/falloc.h.

>   #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) || defined(CONFIG_FALLOCATE_ZERO_RANGE)
> +#ifdef CONFIG_FALLOCATE

Same here.

>   static int do_fallocate(int fd, int mode, off_t offset, off_t len)
>   {
>       do {
> @@ -981,6 +981,12 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
> *aiocb)
>       }
>   #endif
>   
> +#ifdef CONFIG_FALLOCATE
> +    if (aiocb->aio_offset >= aiocb->bs->total_sectors << BDRV_SECTOR_BITS) {
> +        return do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
> +    }
> +#endif
> +

This seems fine though, but as I've asked in patch 5: Do we want to have 
a "has_fallocate"?

Other than that, this is the first usage of bs->total_sectors in this 
file; raw_co_get_block_status() does a similar check, but it uses 
bdrv_getlength() instead. If bs->total_sectors is correct, 
bdrv_getlength() will actually do nothing but return bs->total_sectors * 
BDRV_SECTOR_SIZE; it will only do more (that is, update 
bs->total_sectors) if it is not correct to use bs->total_sectors (and I 
feel like it may not be correct because BlockDriver.has_variable_length 
is true).

Max

>       s->has_write_zeroes = false;
>       return ret;
>   }