12.10.2017 21:59, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Update the qed driver accordingly, taking the opportunity
> to inline qed_is_allocated_cb() into its lone caller (the callback
> used to be important, until we switched qed to coroutines). There is
> no intent to optimize based on the want_zero flag for this format.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: rebase to interface change, inline pointless callback
> v3: no change
> v2: rebase to mapping flag, fix mask in qed_is_allocated_cb
> ---
> block/qed.c | 84 +++++++++++++++++++++----------------------------------------
> 1 file changed, 28 insertions(+), 56 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index 28e2ec89e8..cbab5b6f83 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -688,74 +688,46 @@ finish:
> return ret;
> }
>
> -typedef struct {
> - BlockDriverState *bs;
> - Coroutine *co;
> - uint64_t pos;
> - int64_t status;
> - int *pnum;
> - BlockDriverState **file;
> -} QEDIsAllocatedCB;
> -
> -/* Called with table_lock held. */
> -static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
> -{
> - QEDIsAllocatedCB *cb = opaque;
> - BDRVQEDState *s = cb->bs->opaque;
> - *cb->pnum = len / BDRV_SECTOR_SIZE;
> - switch (ret) {
> - case QED_CLUSTER_FOUND:
> - offset |= qed_offset_into_cluster(s, cb->pos);
> - cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
> - *cb->file = cb->bs->file->bs;
> - break;
> - case QED_CLUSTER_ZERO:
> - cb->status = BDRV_BLOCK_ZERO;
> - break;
> - case QED_CLUSTER_L2:
> - case QED_CLUSTER_L1:
> - cb->status = 0;
> - break;
> - default:
> - assert(ret < 0);
> - cb->status = ret;
> - break;
> - }
> -
> - if (cb->co) {
> - aio_co_wake(cb->co);
> - }
> -}
> -
> -static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
> - int64_t sector_num,
> - int nb_sectors, int *pnum,
> +static int coroutine_fn bdrv_qed_co_block_status(BlockDriverState *bs,
> + bool want_zero,
> + int64_t pos, int64_t bytes,
> + int64_t *pnum, int64_t *map,
> BlockDriverState **file)
> {
> BDRVQEDState *s = bs->opaque;
> - size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
> - QEDIsAllocatedCB cb = {
> - .bs = bs,
> - .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
> - .status = BDRV_BLOCK_OFFSET_MASK,
> - .pnum = pnum,
> - .file = file,
> - };
> + size_t len;
size_t len = bytes;
with that:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + int status;
> QEDRequest request = { .l2_table = NULL };
> uint64_t offset;
> int ret;
>
> qemu_co_mutex_lock(&s->table_lock);
> - ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
> - qed_is_allocated_cb(&cb, ret, offset, len);
> + ret = qed_find_cluster(s, &request, pos, &len, &offset);
len is in-out parameter, you can't use it uninitialized.
>
> - /* The callback was invoked immediately */
> - assert(cb.status != BDRV_BLOCK_OFFSET_MASK);
> + *pnum = len;
> + switch (ret) {
> + case QED_CLUSTER_FOUND:
> + *map = offset | qed_offset_into_cluster(s, pos);
> + status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
> + *file = bs->file->bs;
> + break;
> + case QED_CLUSTER_ZERO:
> + status = BDRV_BLOCK_ZERO;
> + break;
> + case QED_CLUSTER_L2:
> + case QED_CLUSTER_L1:
> + status = 0;
> + break;
> + default:
> + assert(ret < 0);
> + status = ret;
> + break;
> + }
>
> qed_unref_l2_cache_entry(request.l2_table);
> qemu_co_mutex_unlock(&s->table_lock);
>
> - return cb.status;
> + return status;
> }
>
> static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
> @@ -1595,7 +1567,7 @@ static BlockDriver bdrv_qed = {
> .bdrv_child_perm = bdrv_format_default_perms,
> .bdrv_create = bdrv_qed_create,
> .bdrv_has_zero_init = bdrv_has_zero_init_1,
> - .bdrv_co_get_block_status = bdrv_qed_co_get_block_status,
> + .bdrv_co_block_status = bdrv_qed_co_block_status,
> .bdrv_co_readv = bdrv_qed_co_readv,
> .bdrv_co_writev = bdrv_qed_co_writev,
> .bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes,
--
Best regards,
Vladimir