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

["Denis V. Lunev" <den@openvz.org>]

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
>
>