Am 26.12.2014 um 13:35 schrieb Denis V. Lunev:
> The check for maximum length was added by
> commit c31cb70728d2c0c8900b35a66784baa446fd5147
> Author: Peter Lieven <p...@kamp.de>
> Date: Thu Oct 24 12:06:58 2013 +0200
> block: honour BlockLimits in bdrv_co_do_write_zeroes
>
> but actually if driver provides .bdrv_co_write_zeroes callback, there is
> no need to limit the size to 32 MB. Callback should provide effective
> implementation which normally should not write any zeroes in comparable
> amount.
NACK.
First there is no guarantee that bdrv_co_do_write_zeroes is a fast operation.
This heaviliy depends on several circumstances that the block layer is not
aware of.
If a specific protocol knows it is very fast in writing zeroes under any
circumstance
it should provide INT_MAX in bs->bl.max_write_zeroes. It is then still allowed
to
return -ENOTSUP if the request size or alignment doesn't fit.
There are known backends e.g. anything that deals with SCSI that have a known
limitation of the maximum number of zeroes they can write fast in a single
request.
This number MUST NOT be exceeded. The below patch would break all those
backends.
What issue are you trying to fix with this patch? Maybe there is a better way
to fix
it at another point in the code.
Peter
>
> Correct check should be as follows to honour BlockLimits for slow writes:
> - if bs->bl.max_write_zeroes is specified, it should be applied to all
> paths (fast and slow)
> - MAX_WRITE_ZEROES_DEFAULT should be applied for slow path only
>
> Signed-off-by: Denis V. Lunev <d...@openvz.org>
> CC: Eric Blake <ebl...@redhat.com>
> CC: Peter Lieven <p...@kamp.de>
> CC: Kevin Wolf <kw...@redhat.com>
> CC: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> block.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index 4165d42..0ec8b15 100644
> --- a/block.c
> +++ b/block.c
> @@ -3182,8 +3182,12 @@ static int coroutine_fn
> bdrv_co_do_write_zeroes(BlockDriverState *bs,
> struct iovec iov = {0};
> int ret = 0;
>
> - int max_write_zeroes = bs->bl.max_write_zeroes ?
> - bs->bl.max_write_zeroes :
> MAX_WRITE_ZEROES_DEFAULT;
> + int max_fast_write_size = nb_sectors;
> + int max_slow_write_size = MAX_WRITE_ZEROES_DEFAULT;
> + if (bs->bl.max_write_zeroes != 0) {
> + max_fast_write_size = bs->bl.max_write_zeroes;
> + max_slow_write_size = bs->bl.max_write_zeroes;
> + }
>
> while (nb_sectors > 0 && !ret) {
> int num = nb_sectors;
> @@ -3205,9 +3209,9 @@ static int coroutine_fn
> bdrv_co_do_write_zeroes(BlockDriverState *bs,
> }
> }
>
> - /* limit request size */
> - if (num > max_write_zeroes) {
> - num = max_write_zeroes;
> + /* limit request size for fast path */
> + if (num > max_fast_write_size) {
> + num = max_fast_write_size;
> }
>
> ret = -ENOTSUP;
> @@ -3217,6 +3221,11 @@ static int coroutine_fn
> bdrv_co_do_write_zeroes(BlockDriverState *bs,
> }
>
> if (ret == -ENOTSUP) {
> + /* limit request size further for slow path */
> + if (num > max_slow_write_size) {
> + num = max_slow_write_size;
> + }
> +
> /* Fall back to bounce buffer if write zeroes is unsupported */
> iov.iov_len = num * BDRV_SECTOR_SIZE;
> if (iov.iov_base == NULL) {
> @@ -3234,7 +3243,7 @@ static int coroutine_fn
> bdrv_co_do_write_zeroes(BlockDriverState *bs,
> /* Keep bounce buffer around if it is big enough for all
> * all future requests.
> */
> - if (num < max_write_zeroes) {
> + if (num < max_slow_write_size) {
> qemu_vfree(iov.iov_base);
> iov.iov_base = NULL;
> }
