| Message ID | 20171012185916.22776-10-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 parallels driver accordingly. Note that > the internal function block_status() is still sector-based, because > it is still in use by other sector-based functions; but that's okay > because request_alignment is 512 as a result of those functions. > For now, no optimizations are added based on the mapping hint. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v4: rebase to interface tweak, R-b dropped > v3: no change > v2: rebase to mapping parameter; it is ignored, so R-b kept > --- > block/parallels.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index 2b6c6e5709..4a086f29e9 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -278,23 +278,31 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs) > } > > > -static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) > +static int coroutine_fn parallels_co_block_status(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file) > { > BDRVParallelsState *s = bs->opaque; > - int64_t offset; > + int count; > > + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); > qemu_co_mutex_lock(&s->lock); > - offset = block_status(s, sector_num, nb_sectors, pnum); > + offset = block_status(s, offset >> BDRV_SECTOR_BITS, > + bytes >> BDRV_SECTOR_BITS, &count); > qemu_co_mutex_unlock(&s->lock); > > if (offset < 0) { pnum=count*BDRV_SECTOR_SIZE and map=0 setting missed here. > return 0; > } > > + *pnum = count * BDRV_SECTOR_SIZE; > + *map = offset; > *file = bs->file->bs; > - return (offset << BDRV_SECTOR_BITS) | > - BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > + return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > } > > static coroutine_fn int parallels_co_writev(BlockDriverState *bs, > @@ -781,7 +789,7 @@ static BlockDriver bdrv_parallels = { > .bdrv_open = parallels_open, > .bdrv_close = parallels_close, > .bdrv_child_perm = bdrv_format_default_perms, > - .bdrv_co_get_block_status = parallels_co_get_block_status, > + .bdrv_co_block_status = parallels_co_block_status, > .bdrv_has_zero_init = bdrv_has_zero_init_1, > .bdrv_co_flush_to_os = parallels_co_flush_to_os, > .bdrv_co_readv = parallels_co_readv,
On 11/30/2017 03:03 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 parallels driver accordingly. Note that >> the internal function block_status() is still sector-based, because >> it is still in use by other sector-based functions; but that's okay >> because request_alignment is 512 as a result of those functions. >> For now, no optimizations are added based on the mapping hint. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> +static int coroutine_fn parallels_co_block_status(BlockDriverState *bs, >> + bool want_zero, >> + int64_t offset, >> + int64_t bytes, >> + int64_t *pnum, >> + int64_t *map, >> + BlockDriverState >> **file) >> { >> BDRVParallelsState *s = bs->opaque; >> - int64_t offset; >> + int count; >> >> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); >> qemu_co_mutex_lock(&s->lock); >> - offset = block_status(s, sector_num, nb_sectors, pnum); >> + offset = block_status(s, offset >> BDRV_SECTOR_BITS, >> + bytes >> BDRV_SECTOR_BITS, &count); >> qemu_co_mutex_unlock(&s->lock); >> >> if (offset < 0) { > > pnum=count*BDRV_SECTOR_SIZE and map=0 setting missed here. > >> return 0; >> } Setting *map is only required when return value includes BDRV_BLOCK_OFFSET_VALID, so that one was not necessary. But you do raise an interesting point about a pre-existing bug with pnum not being set. Commit 298a1665a guarantees that *pnum is 0 (and not uninitialized garbage) - but that still violates the contract that we (want to) have that all drivers either make progress or return an error (setting pnum to 0 should only be done at EOF or on error).
On 11/30/2017 04:12 PM, Eric Blake wrote: >>> BDRVParallelsState *s = bs->opaque; >>> - int64_t offset; >>> + int count; >>> >>> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); >>> qemu_co_mutex_lock(&s->lock); >>> - offset = block_status(s, sector_num, nb_sectors, pnum); >>> + offset = block_status(s, offset >> BDRV_SECTOR_BITS, >>> + bytes >> BDRV_SECTOR_BITS, &count); >>> qemu_co_mutex_unlock(&s->lock); >>> >>> if (offset < 0) { >> >> pnum=count*BDRV_SECTOR_SIZE and map=0 setting missed here. >> >>> return 0; >>> } > > Setting *map is only required when return value includes > BDRV_BLOCK_OFFSET_VALID, so that one was not necessary. But you do > raise an interesting point about a pre-existing bug with pnum not being > set. Commit 298a1665a guarantees that *pnum is 0 (and not uninitialized > garbage) - but that still violates the contract that we (want to) have > that all drivers either make progress or return an error (setting pnum > to 0 should only be done at EOF or on error). Oh. The pre-existing code DID set pnum to a non-zero value, as a side effect of block_status(); the new code fails to do so. So it is not pre-existing; good catch!
diff --git a/block/parallels.c b/block/parallels.c index 2b6c6e5709..4a086f29e9 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -278,23 +278,31 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs) } -static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) +static int coroutine_fn parallels_co_block_status(BlockDriverState *bs, + bool want_zero, + int64_t offset, + int64_t bytes, + int64_t *pnum, + int64_t *map, + BlockDriverState **file) { BDRVParallelsState *s = bs->opaque; - int64_t offset; + int count; + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); qemu_co_mutex_lock(&s->lock); - offset = block_status(s, sector_num, nb_sectors, pnum); + offset = block_status(s, offset >> BDRV_SECTOR_BITS, + bytes >> BDRV_SECTOR_BITS, &count); qemu_co_mutex_unlock(&s->lock); if (offset < 0) { return 0; } + *pnum = count * BDRV_SECTOR_SIZE; + *map = offset; *file = bs->file->bs; - return (offset << BDRV_SECTOR_BITS) | - BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; + return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; } static coroutine_fn int parallels_co_writev(BlockDriverState *bs, @@ -781,7 +789,7 @@ static BlockDriver bdrv_parallels = { .bdrv_open = parallels_open, .bdrv_close = parallels_close, .bdrv_child_perm = bdrv_format_default_perms, - .bdrv_co_get_block_status = parallels_co_get_block_status, + .bdrv_co_block_status = parallels_co_block_status, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_flush_to_os = parallels_co_flush_to_os, .bdrv_co_readv = parallels_co_readv,
We are gradually moving away from sector-based interfaces, towards byte-based. Update the parallels driver accordingly. Note that the internal function block_status() is still sector-based, because it is still in use by other sector-based functions; but that's okay because request_alignment is 512 as a result of those functions. For now, no optimizations are added based on the mapping hint. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: rebase to interface tweak, R-b dropped v3: no change v2: rebase to mapping parameter; it is ignored, so R-b kept --- block/parallels.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)