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

[Max Reitz <mreitz@redhat.com>]

On 2015-01-27 at 13:19, Denis V. Lunev wrote:
> On 27/01/15 20:57, Max Reitz wrote:
>> 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 <den@openvz.org>
>>> 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 | 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.
>>
>
> here is a check for fallocate and posix_fallocate in configure script
>
> # check for fallocate
> fallocate=no
> cat > $TMPC << EOF
> #include <fcntl.h>
>
> int main(void)
> {
>     fallocate(0, 0, 0, 0);
>     return 0;
> }
> EOF
> if compile_prog "" "" ; then
>   fallocate=yes
> fi
> ...
> # check for posix_fallocate
> posix_fallocate=no
> cat > $TMPC << EOF
> #include <fcntl.h>
>
> int main(void)
> {
>     posix_fallocate(0, 0, 0);
>     return 0;
> }
> EOF
> if compile_prog "" "" ; then
>     posix_fallocate=yes
> fi
> ...
> if test "$fallocate" = "yes" ; then
>   echo "CONFIG_FALLOCATE=y" >> $config_host_mak
> fi
> ...
> if test "$posix_fallocate" = "yes" ; then
>   echo "CONFIG_POSIX_FALLOCATE=y" >> $config_host_mak
> fi
>
> Thus my check looks correct to me.

Oh, sorry, I somehow mixed those checks. You're right.

Very well then; maybe you want to mention this change in the commit 
message, though?

Max

>
>>>   #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
>>
> ok, will do