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:49, Kevin Wolf wrote:
> Am 02.02.2015 um 15:20 hat Peter Lieven geschrieben:
>> 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.
> Commit c31cb707 added the limit to bdrv_co_do_write_zeroes(). Before, we
> used a bounce buffer of unbounded size.
>
> Anyway, it seems that none of us can think of a reason not to apply the
> patch to block.c. Let's just do it, and if it does break something,
> we'll figure it out. Can you send it as a proper patch?
>
> Denis, if we apply that patch, would you be okay with dropping 7/7 from
> this series, or would still something be missing?
>
> Kevin
Sure. This will be even better. Something similar was implemented in
v1/v2 of the patchset.

Regards,
     Den