| Message ID | 20171012185916.22776-3-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | add byte-based block_status driver callbacks | expand |
12.10.2017 21:58, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. Update the generic helpers, and all passthrough clients > (blkdebug, commit, mirror, throttle) accordingly. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v4: rebase to interface tweak > v3: rebase to addition of throttle driver > v2: rebase to master, retitle while merging blkdebug, commit, and mirror > --- > include/block/block_int.h | 28 ++++++++++++++++------------ > block/io.c | 36 ++++++++++++++++++++---------------- > block/blkdebug.c | 20 +++++++++++--------- > block/commit.c | 2 +- > block/mirror.c | 2 +- > block/throttle.c | 2 +- > 6 files changed, 50 insertions(+), 40 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 4153cd646d..7c8503f693 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1018,23 +1018,27 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, > uint64_t *nperm, uint64_t *nshared); > > /* > - * Default implementation for drivers to pass bdrv_co_get_block_status() to > + * Default implementation for drivers to pass bdrv_co_block_status() to > * their file. > */ > -int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs, > - int64_t sector_num, > - int nb_sectors, > - int *pnum, > - BlockDriverState **file); > +int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file); > /* > - * Default implementation for drivers to pass bdrv_co_get_block_status() to > + * Default implementation for drivers to pass bdrv_co_block_status() to > * their backing file. > */ > -int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs, > - int64_t sector_num, > - int nb_sectors, > - int *pnum, > - BlockDriverState **file); > +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file); > const char *bdrv_get_parent_name(const BlockDriverState *bs); > void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp); > bool blk_dev_has_removable_media(BlockBackend *blk); > diff --git a/block/io.c b/block/io.c > index ef9ea44667..6a2a2e1484 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1754,30 +1754,34 @@ typedef struct BdrvCoBlockStatusData { > bool done; > } BdrvCoBlockStatusData; > > -int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs, > - int64_t sector_num, > - int nb_sectors, > - int *pnum, > - BlockDriverState **file) > +int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file) > { > assert(bs->file && bs->file->bs); > - *pnum = nb_sectors; > + *pnum = bytes; > + *map = offset; > *file = bs->file->bs; > - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > - (sector_num << BDRV_SECTOR_BITS); > + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; > } > > -int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs, > - int64_t sector_num, > - int nb_sectors, > - int *pnum, > - BlockDriverState **file) > +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file) > { > assert(bs->backing && bs->backing->bs); > - *pnum = nb_sectors; > + *pnum = bytes; > + *map = offset; > *file = bs->backing->bs; > - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > - (sector_num << BDRV_SECTOR_BITS); > + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; > } hmm. not related question, but can you please explain, why we use such stubs instead of really call bdrv_co_block_status on bs->file->bs or bs->backing->bs ? > > /* > diff --git a/block/blkdebug.c b/block/blkdebug.c > index e21669979d..82c30357f8 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -627,15 +627,17 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs, > return bdrv_co_pdiscard(bs->file->bs, offset, bytes); > } > > -static int64_t coroutine_fn blkdebug_co_get_block_status( > - BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, > - BlockDriverState **file) > +static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file) > { > - assert(QEMU_IS_ALIGNED(sector_num | nb_sectors, > - DIV_ROUND_UP(bs->bl.request_alignment, > - BDRV_SECTOR_SIZE))); > - return bdrv_co_get_block_status_from_file(bs, sector_num, nb_sectors, > - pnum, file); > + assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment)); looks like this is obviously guaranteed by block layer, so no needs for special function. and we can use .bdrv_co_block_status = bdrv_co_block_status_from_backing like for other drivers. > + return bdrv_co_block_status_from_file(bs, want_zero, offset, bytes, > + pnum, map, file); > } > > static void blkdebug_close(BlockDriverState *bs) > @@ -907,7 +909,7 @@ static BlockDriver bdrv_blkdebug = { > .bdrv_co_flush_to_disk = blkdebug_co_flush, > .bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes, > .bdrv_co_pdiscard = blkdebug_co_pdiscard, > - .bdrv_co_get_block_status = blkdebug_co_get_block_status, > + .bdrv_co_block_status = blkdebug_co_block_status, > > .bdrv_debug_event = blkdebug_debug_event, > .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, > diff --git a/block/commit.c b/block/commit.c > index 5036eec434..ec0511bac7 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -265,7 +265,7 @@ static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c, > static BlockDriver bdrv_commit_top = { > .format_name = "commit_top", > .bdrv_co_preadv = bdrv_commit_top_preadv, > - .bdrv_co_get_block_status = bdrv_co_get_block_status_from_backing, > + .bdrv_co_block_status = bdrv_co_block_status_from_backing, > .bdrv_refresh_filename = bdrv_commit_top_refresh_filename, > .bdrv_close = bdrv_commit_top_close, > .bdrv_child_perm = bdrv_commit_top_child_perm, > diff --git a/block/mirror.c b/block/mirror.c > index 307b6391a8..79739d063e 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1094,7 +1094,7 @@ static BlockDriver bdrv_mirror_top = { > .bdrv_co_pwrite_zeroes = bdrv_mirror_top_pwrite_zeroes, > .bdrv_co_pdiscard = bdrv_mirror_top_pdiscard, > .bdrv_co_flush = bdrv_mirror_top_flush, > - .bdrv_co_get_block_status = bdrv_co_get_block_status_from_backing, > + .bdrv_co_block_status = bdrv_co_block_status_from_backing, > .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, > .bdrv_close = bdrv_mirror_top_close, > .bdrv_child_perm = bdrv_mirror_top_child_perm, > diff --git a/block/throttle.c b/block/throttle.c > index 5bca76300f..76cd963a8c 100644 > --- a/block/throttle.c > +++ b/block/throttle.c > @@ -224,7 +224,7 @@ static BlockDriver bdrv_throttle = { > .bdrv_reopen_prepare = throttle_reopen_prepare, > .bdrv_reopen_commit = throttle_reopen_commit, > .bdrv_reopen_abort = throttle_reopen_abort, > - .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file, > + .bdrv_co_block_status = bdrv_co_block_status_from_file, > > .is_filter = true, > }; conversion looks ok, Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 11/28/2017 02:19 AM, Vladimir Sementsov-Ogievskiy wrote: > 12.10.2017 21:58, Eric Blake wrote: >> We are gradually moving away from sector-based interfaces, towards >> byte-based. Update the generic helpers, and all passthrough clients >> (blkdebug, commit, mirror, throttle) accordingly. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> BlockDriverState **file) >> +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs, >> + bool want_zero, >> + int64_t offset, >> + int64_t bytes, >> + int64_t *pnum, >> + int64_t *map, >> + BlockDriverState >> **file) >> { >> assert(bs->backing && bs->backing->bs); >> - *pnum = nb_sectors; >> + *pnum = bytes; >> + *map = offset; >> *file = bs->backing->bs; >> - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | >> - (sector_num << BDRV_SECTOR_BITS); >> + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; >> } > > hmm. not related question, but can you please explain, why we use such > stubs > instead of really call bdrv_co_block_status on bs->file->bs or > bs->backing->bs ? > Returning BDRV_BLOCK_RAW is what tells io.c to call bdrv_co_block_status() on whatever got assigned into *file. The two stubs make it so there are fewer conditionals in io.c; perhaps it could be reworked to avoid the stubs and have io.c have all the smarts when .bdrv_co_block_status is NULL, but as you say, that would be a separate patch (although it was Manos' work earlier this year that even created the common helper stubs rather than duplicating code in each callback in the first place). >> +static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs, >> + assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment)); > > looks like this is obviously guaranteed by block layer, so no needs for > special > function. and we can use > > .bdrv_co_block_status = bdrv_co_block_status_from_backing > > like for other drivers. > We could, but we don't want to. The point of the blkdebug driver is to explicitly test that certain preconditions are being satisfied, so that even if the block layer in io.c changes, our iotests make sure that drivers are provided with certain guarantees. In other words, the assert() added here is added value that we cannot stick in the common helper, but something that we want to keep associated with the blkdebug driver.
diff --git a/include/block/block_int.h b/include/block/block_int.h index 4153cd646d..7c8503f693 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1018,23 +1018,27 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, uint64_t *nperm, uint64_t *nshared); /* - * Default implementation for drivers to pass bdrv_co_get_block_status() to + * Default implementation for drivers to pass bdrv_co_block_status() to * their file. */ -int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, - int *pnum, - BlockDriverState **file); +int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs, + bool want_zero, + int64_t offset, + int64_t bytes, + int64_t *pnum, + int64_t *map, + BlockDriverState **file); /* - * Default implementation for drivers to pass bdrv_co_get_block_status() to + * Default implementation for drivers to pass bdrv_co_block_status() to * their backing file. */ -int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, - int *pnum, - BlockDriverState **file); +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs, + bool want_zero, + int64_t offset, + int64_t bytes, + int64_t *pnum, + int64_t *map, + BlockDriverState **file); const char *bdrv_get_parent_name(const BlockDriverState *bs); void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp); bool blk_dev_has_removable_media(BlockBackend *blk); diff --git a/block/io.c b/block/io.c index ef9ea44667..6a2a2e1484 100644 --- a/block/io.c +++ b/block/io.c @@ -1754,30 +1754,34 @@ typedef struct BdrvCoBlockStatusData { bool done; } BdrvCoBlockStatusData; -int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, - int *pnum, - BlockDriverState **file) +int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs, + bool want_zero, + int64_t offset, + int64_t bytes, + int64_t *pnum, + int64_t *map, + BlockDriverState **file) { assert(bs->file && bs->file->bs); - *pnum = nb_sectors; + *pnum = bytes; + *map = offset; *file = bs->file->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | - (sector_num << BDRV_SECTOR_BITS); + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; } -int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs, - int64_t sector_num, - int nb_sectors, - int *pnum, - BlockDriverState **file) +int coroutine_fn bdrv_co_block_status_from_backing(BlockDriverState *bs, + bool want_zero, + int64_t offset, + int64_t bytes, + int64_t *pnum, + int64_t *map, + BlockDriverState **file) { assert(bs->backing && bs->backing->bs); - *pnum = nb_sectors; + *pnum = bytes; + *map = offset; *file = bs->backing->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | - (sector_num << BDRV_SECTOR_BITS); + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; } /* diff --git a/block/blkdebug.c b/block/blkdebug.c index e21669979d..82c30357f8 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -627,15 +627,17 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs, return bdrv_co_pdiscard(bs->file->bs, offset, bytes); } -static int64_t coroutine_fn blkdebug_co_get_block_status( - BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, - BlockDriverState **file) +static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs, + bool want_zero, + int64_t offset, + int64_t bytes, + int64_t *pnum, + int64_t *map, + BlockDriverState **file) { - assert(QEMU_IS_ALIGNED(sector_num | nb_sectors, - DIV_ROUND_UP(bs->bl.request_alignment, - BDRV_SECTOR_SIZE))); - return bdrv_co_get_block_status_from_file(bs, sector_num, nb_sectors, - pnum, file); + assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment)); + return bdrv_co_block_status_from_file(bs, want_zero, offset, bytes, + pnum, map, file); } static void blkdebug_close(BlockDriverState *bs) @@ -907,7 +909,7 @@ static BlockDriver bdrv_blkdebug = { .bdrv_co_flush_to_disk = blkdebug_co_flush, .bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes, .bdrv_co_pdiscard = blkdebug_co_pdiscard, - .bdrv_co_get_block_status = blkdebug_co_get_block_status, + .bdrv_co_block_status = blkdebug_co_block_status, .bdrv_debug_event = blkdebug_debug_event, .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, diff --git a/block/commit.c b/block/commit.c index 5036eec434..ec0511bac7 100644 --- a/block/commit.c +++ b/block/commit.c @@ -265,7 +265,7 @@ static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c, static BlockDriver bdrv_commit_top = { .format_name = "commit_top", .bdrv_co_preadv = bdrv_commit_top_preadv, - .bdrv_co_get_block_status = bdrv_co_get_block_status_from_backing, + .bdrv_co_block_status = bdrv_co_block_status_from_backing, .bdrv_refresh_filename = bdrv_commit_top_refresh_filename, .bdrv_close = bdrv_commit_top_close, .bdrv_child_perm = bdrv_commit_top_child_perm, diff --git a/block/mirror.c b/block/mirror.c index 307b6391a8..79739d063e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1094,7 +1094,7 @@ static BlockDriver bdrv_mirror_top = { .bdrv_co_pwrite_zeroes = bdrv_mirror_top_pwrite_zeroes, .bdrv_co_pdiscard = bdrv_mirror_top_pdiscard, .bdrv_co_flush = bdrv_mirror_top_flush, - .bdrv_co_get_block_status = bdrv_co_get_block_status_from_backing, + .bdrv_co_block_status = bdrv_co_block_status_from_backing, .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, .bdrv_close = bdrv_mirror_top_close, .bdrv_child_perm = bdrv_mirror_top_child_perm, diff --git a/block/throttle.c b/block/throttle.c index 5bca76300f..76cd963a8c 100644 --- a/block/throttle.c +++ b/block/throttle.c @@ -224,7 +224,7 @@ static BlockDriver bdrv_throttle = { .bdrv_reopen_prepare = throttle_reopen_prepare, .bdrv_reopen_commit = throttle_reopen_commit, .bdrv_reopen_abort = throttle_reopen_abort, - .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file, + .bdrv_co_block_status = bdrv_co_block_status_from_file, .is_filter = true, };
We are gradually moving away from sector-based interfaces, towards byte-based. Update the generic helpers, and all passthrough clients (blkdebug, commit, mirror, throttle) accordingly. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: rebase to interface tweak v3: rebase to addition of throttle driver v2: rebase to master, retitle while merging blkdebug, commit, and mirror --- include/block/block_int.h | 28 ++++++++++++++++------------ block/io.c | 36 ++++++++++++++++++++---------------- block/blkdebug.c | 20 +++++++++++--------- block/commit.c | 2 +- block/mirror.c | 2 +- block/throttle.c | 2 +- 6 files changed, 50 insertions(+), 40 deletions(-)