| Message ID | 20171012185916.22776-13-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | add byte-based block_status driver callbacks | expand |
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,
On 11/30/2017 04:27 AM, Vladimir Sementsov-Ogievskiy wrote: > 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> >> >> { >> 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; > Or rather, size_t len = MIN(bytes, SIZE_MAX); thanks to 32-bit platforms. > 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. Good catch.
01.12.2017 02:17, Eric Blake wrote: > On 11/30/2017 04:27 AM, Vladimir Sementsov-Ogievskiy wrote: >> 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> >>> > >>> { >>> 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; >> > > Or rather, size_t len = MIN(bytes, SIZE_MAX); thanks to 32-bit platforms. ok, this is even better > >> 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. > > Good catch. >
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; + 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); - /* 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,
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(-)