Re: [Qemu-devel] [PATCH 09/13] qed: Convert to bdrv_co_pwrite_zeroes()

[Kevin Wolf]

Am 25.05.2016 um 00:25 hat Eric Blake geschrieben:
> Another step on our continuing quest to switch to byte-based
> interfaces.
> 
> Kill an abuse of the comma operator while at it (fortunately,
> the semantics were still right).
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  block/qed.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 0ab5b40..a0be886 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1419,7 +1419,7 @@ typedef struct {
>      bool done;
>  } QEDWriteZeroesCB;
> 
> -static void coroutine_fn qed_co_write_zeroes_cb(void *opaque, int ret)
> +static void coroutine_fn qed_co_pwrite_zeroes_cb(void *opaque, int ret)
>  {
>      QEDWriteZeroesCB *cb = opaque;
> 
> @@ -1430,10 +1430,10 @@ static void coroutine_fn qed_co_write_zeroes_cb(void 
> *opaque, int ret)
>      }
>  }
> 
> -static int coroutine_fn bdrv_qed_co_write_zeroes(BlockDriverState *bs,
> -                                                 int64_t sector_num,
> -                                                 int nb_sectors,
> -                                                 BdrvRequestFlags flags)
> +static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
> +                                                  int64_t offset,
> +                                                  int count,
> +                                                  BdrvRequestFlags flags)
>  {
>      BlockAIOCB *blockacb;
>      BDRVQEDState *s = bs->opaque;
> @@ -1443,10 +1443,10 @@ static int coroutine_fn 
> bdrv_qed_co_write_zeroes(BlockDriverState *bs,
> 
>      /* Refuse if there are untouched backing file sectors */
>      if (bs->backing) {
> -        if (qed_offset_into_cluster(s, sector_num * BDRV_SECTOR_SIZE) != 0) {
> +        if (qed_offset_into_cluster(s, offset) != 0) {
>              return -ENOTSUP;
>          }
> -        if (qed_offset_into_cluster(s, nb_sectors * BDRV_SECTOR_SIZE) != 0) {
> +        if (qed_offset_into_cluster(s, count) != 0) {
>              return -ENOTSUP;
>          }
>      }

Unaligned requests are only emulated if there is no backing file...

> @@ -1454,12 +1454,13 @@ static int coroutine_fn 
> bdrv_qed_co_write_zeroes(BlockDriverState *bs,
>      /* Zero writes start without an I/O buffer.  If a buffer becomes 
> necessary
>       * then it will be allocated during request processing.
>       */
> -    iov.iov_base = NULL,
> -    iov.iov_len  = nb_sectors * BDRV_SECTOR_SIZE,
> +    iov.iov_base = NULL;
> +    iov.iov_len = count;
> 
>      qemu_iovec_init_external(&qiov, &iov, 1);
> -    blockacb = qed_aio_setup(bs, sector_num, &qiov, nb_sectors,
> -                             qed_co_write_zeroes_cb, &cb,
> +    blockacb = qed_aio_setup(bs, offset >> BDRV_SECTOR_BITS, &qiov,
> +                             count >> BDRV_SECTOR_BITS,

...so offset and count can still be unaligned here and we end up zeroing
out the wrong part of the sector. I guess we need to return -ENOTSUP for
all sub-sector requests, even without a backing file.

> +                             qed_co_pwrite_zeroes_cb, &cb,
>                               QED_AIOCB_WRITE | QED_AIOCB_ZERO);
>      if (!blockacb) {
>          return -EIO;

Kevin