Re: [Qemu-devel] [PATCH 03/13] block: Add .bdrv_co_pwrite_zeroes()

[Kevin Wolf <kwolf@redhat.com>]

Am 25.05.2016 um 00:25 hat Eric Blake geschrieben:
> Update bdrv_co_do_write_zeroes() to be byte-based, and select
> between the new byte-based bdrv_co_pwrite_zeroes() or the old
> bdrv_co_write_zeroes().  The next patches will convert drivers,
> then remove the old interface.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block_int.h |  4 ++-
>  block/io.c                | 81 +++++++++++++++++++++++++----------------------
>  2 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 4282ffd..fa7e3f9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -165,6 +165,8 @@ struct BlockDriver {
>       */
>      int coroutine_fn (*bdrv_co_write_zeroes)(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
> +    int coroutine_fn (*bdrv_co_pwrite_zeroes)(BlockDriverState *bs,
> +        int64_t offset, int count, BdrvRequestFlags flags);
>      int coroutine_fn (*bdrv_co_discard)(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors);
>      int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
> @@ -454,7 +456,7 @@ struct BlockDriverState {
>      unsigned int request_alignment;
>      /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
>      unsigned int supported_write_flags;
> -    /* Flags honored during write_zeroes (so far: BDRV_REQ_FUA,
> +    /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
>       * BDRV_REQ_MAY_UNMAP) */
>      unsigned int supported_zero_flags;
> 
> diff --git a/block/io.c b/block/io.c
> index 41b4e9d..c1d700b 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -42,8 +42,8 @@ static BlockAIOCB *bdrv_co_aio_rw_vector(BlockDriverState *bs,
>                                           void *opaque,
>                                           bool is_write);
>  static void coroutine_fn bdrv_co_do_rw(void *opaque);
> -static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> -    int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
> +static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> +    int64_t offset, int count, BdrvRequestFlags flags);
> 
>  static void bdrv_parent_drained_begin(BlockDriverState *bs)
>  {
> @@ -876,10 +876,12 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BlockDriverState *bs,
>          goto err;
>      }
> 
> -    if (drv->bdrv_co_write_zeroes &&
> +    if ((drv->bdrv_co_write_zeroes || drv->bdrv_co_pwrite_zeroes) &&
>          buffer_is_zero(bounce_buffer, iov.iov_len)) {
> -        ret = bdrv_co_do_write_zeroes(bs, cluster_sector_num,
> -                                      cluster_nb_sectors, 0);
> +        ret = bdrv_co_do_pwrite_zeroes(bs,
> +                                       cluster_sector_num * BDRV_SECTOR_SIZE,
> +                                       cluster_nb_sectors * BDRV_SECTOR_SIZE,
> +                                       0);
>      } else {
>          /* This does not change the data on the disk, it is not necessary
>           * to flush even in cache=writethrough mode.
> @@ -1111,8 +1113,8 @@ int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
> 
>  #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)
> +static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> +    int64_t offset, int count, BdrvRequestFlags flags)
>  {
>      BlockDriver *drv = bs->drv;
>      QEMUIOVector qiov;
> @@ -1121,40 +1123,45 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>      bool need_flush = false;
> 
>      int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
> -    int max_write_zeroes_sectors = max_write_zeroes >> BDRV_SECTOR_BITS;
> -    int write_zeroes_sector_align =
> -        bs->bl.pwrite_zeroes_alignment >> BDRV_SECTOR_BITS;
> +    int alignment = MAX(bs->bl.pwrite_zeroes_alignment, BDRV_SECTOR_SIZE);

Why do we round up to sector granularity? When everything is based on
bytes, this shouldn't be necessary, but even at the end of the series,
this is still done. Shouldn't bs->bl.pwrite_zeroes_alignment ?: 1 be
good enough?

Looks good to me otherwise.

Kevin