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

[Kevin Wolf <kwolf@redhat.com>]

Am 01.06.2016 um 23:10 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).  Also, the test for requests
> not aligned to clusters should be applied always, not just
> when a backing file is present.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qed.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 0ab5b40..45ec13d 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;
> @@ -1441,25 +1441,22 @@ static int coroutine_fn bdrv_qed_co_write_zeroes(BlockDriverState *bs,
>      QEMUIOVector qiov;
>      struct iovec iov;
> 
> -    /* Refuse if there are untouched backing file sectors */
> -    if (bs->backing) {
> -        if (qed_offset_into_cluster(s, sector_num * BDRV_SECTOR_SIZE) != 0) {
> -            return -ENOTSUP;
> -        }
> -        if (qed_offset_into_cluster(s, nb_sectors * BDRV_SECTOR_SIZE) != 0) {
> -            return -ENOTSUP;
> -        }
> +    /* Fall back if the request is not */

...aligned?

> +    if (qed_offset_into_cluster(s, offset) ||
> +        qed_offset_into_cluster(s, count)) {
> +        return -ENOTSUP;
>      }

This is obviously correct and almost as obviously suboptimal compared to
the original version (we need cluster alignment with a backing file, but
without a backing file, sector alignment would be enough).

But as this is QED, which is only supported for compatibility these
days, simpler if slightly suboptimal code is okay with me.

Kevin