| Message ID | 20170629184320.7151-4-el13635@mail.ntua.gr |
|---|---|
| State | New |
| Headers | show |
On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote: > bdrv_co_get_block_status_from_file() and > bdrv_co_get_block_status_from_backing() set *file to bs->file and > bs->backing respectively, so that bdrv_co_get_block_status() can recurse > to them. Future block drivers won't have to duplicate code to implement > this. > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > block/blkdebug.c | 12 +----------- > block/commit.c | 12 +----------- > block/io.c | 24 ++++++++++++++++++++++++ > block/mirror.c | 12 +----------- > include/block/block_int.h | 16 ++++++++++++++++ > 5 files changed, 43 insertions(+), 33 deletions(-) My [still-needs-rebasing] part 3 series converting bdrv_co_get_block_status to byte-based will be impacted by this, but I'd rather yours go in first and I can deal with the rebase fallout. > +++ b/include/block/block_int.h > @@ -945,6 +945,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, > uint64_t perm, uint64_t shared, > uint64_t *nperm, uint64_t *nshared); > > +/* > + * Default implementation for drivers to pass bdrv_co_get_block_status() to > + * their file I would have ended with '.' > + * */ Looks odd. Just use ' */', not ' * */' > +int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs, > + int64_t sector_num, > + int nb_sectors, int *pnum, > + BlockDriverState **file); Indentation is unusual (not necessarily bad, since you are up against 80-column limits). > +/* > + * Default implementation for drivers to pass bdrv_co_get_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); Ditto. Making changes according to what I mentioned is minor, so you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
On 07/03/2017 11:12 AM, Eric Blake wrote: > On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote: >> bdrv_co_get_block_status_from_file() and >> bdrv_co_get_block_status_from_backing() set *file to bs->file and >> bs->backing respectively, so that bdrv_co_get_block_status() can recurse >> to them. Future block drivers won't have to duplicate code to implement >> this. >> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> >> --- >> block/blkdebug.c | 12 +----------- >> block/commit.c | 12 +----------- >> block/io.c | 24 ++++++++++++++++++++++++ >> block/mirror.c | 12 +----------- >> include/block/block_int.h | 16 ++++++++++++++++ >> 5 files changed, 43 insertions(+), 33 deletions(-) > > My [still-needs-rebasing] part 3 series converting > bdrv_co_get_block_status to byte-based will be impacted by this, but I'd > rather yours go in first and I can deal with the rebase fallout. In fact, my rebase fallout is to completely delete bdrv_co_get_block_status_from_file(). Why? Because blkdebug is the only client, but I want to enhance blkdebug to add in-place assert()ions that the values passed by the block layer are properly aligned to bs->bl.request_alignment (matching what blkdebug does for read and write - after all, if a device cannot access smaller than a given alignment for a read, it should not be able to report different statuses half-way through that granularity). However, in-place assertions are no longer generic, so we no longer have a client of a generic helper function referring to bs->file. Which means you are REALLY left only with commit and mirror as the two functions that have a common implementation of pointing back to backing.
On Mon, Jul 03, 2017 at 12:51:45PM -0500, Eric Blake wrote: >On 07/03/2017 11:12 AM, Eric Blake wrote: >> On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote: >>> bdrv_co_get_block_status_from_file() and >>> bdrv_co_get_block_status_from_backing() set *file to bs->file and >>> bs->backing respectively, so that bdrv_co_get_block_status() can recurse >>> to them. Future block drivers won't have to duplicate code to implement >>> this. >>> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> >>> --- >>> block/blkdebug.c | 12 +----------- >>> block/commit.c | 12 +----------- >>> block/io.c | 24 ++++++++++++++++++++++++ >>> block/mirror.c | 12 +----------- >>> include/block/block_int.h | 16 ++++++++++++++++ >>> 5 files changed, 43 insertions(+), 33 deletions(-) >> >> My [still-needs-rebasing] part 3 series converting >> bdrv_co_get_block_status to byte-based will be impacted by this, but I'd >> rather yours go in first and I can deal with the rebase fallout. > >In fact, my rebase fallout is to completely delete >bdrv_co_get_block_status_from_file(). Why? Because blkdebug is the only >client, but I want to enhance blkdebug to add in-place assert()ions that >the values passed by the block layer are properly aligned to >bs->bl.request_alignment (matching what blkdebug does for read and write >- after all, if a device cannot access smaller than a given alignment >for a read, it should not be able to report different statuses half-way >through that granularity). However, in-place assertions are no longer >generic, so we no longer have a client of a generic helper function >referring to bs->file. > >Which means you are REALLY left only with commit and mirror as the two >functions that have a common implementation of pointing back to >backing. Hello Eric, thanks for taking the time to review my patches. The throttle driver I'm working on passes bdrv_co_get_block_status() to bs->file. If there is a problem with an unused default function (it's not static so will compile, but it might not be up to standard), you can just remove it and I will reintroduce it when it's needed. CC me on those patches when you send them if you can.
On 07/03/2017 01:24 PM, Manos Pitsidianakis wrote: >> In fact, my rebase fallout is to completely delete >> bdrv_co_get_block_status_from_file(). Why? Because blkdebug is the only >> client, but I want to enhance blkdebug to add in-place assert()ions that >> the values passed by the block layer are properly aligned to >> bs->bl.request_alignment (matching what blkdebug does for read and write >> - after all, if a device cannot access smaller than a given alignment >> for a read, it should not be able to report different statuses half-way >> through that granularity). However, in-place assertions are no longer >> generic, so we no longer have a client of a generic helper function >> referring to bs->file. >> >> Which means you are REALLY left only with commit and mirror as the two >> functions that have a common implementation of pointing back to backing. > > > Hello Eric, thanks for taking the time to review my patches. > > The throttle driver I'm working on passes bdrv_co_get_block_status() to > bs->file. If there is a problem with an unused default function (it's > not static so will compile, but it might not be up to standard), you can > just remove it and I will reintroduce it when it's needed. CC me on > those patches when you send them if you can. Sure. The other thing I can do is have: blkdebug_version() { assert(...); return common version; } so that the common version is still there for use by the throttling code. I'm adjusting my rebase accordingly, now that I know you have another intended use in mind.
On 07/03/2017 01:31 PM, Eric Blake wrote: >> The throttle driver I'm working on passes bdrv_co_get_block_status() to >> bs->file. If there is a problem with an unused default function (it's >> not static so will compile, but it might not be up to standard), you can >> just remove it and I will reintroduce it when it's needed. CC me on >> those patches when you send them if you can. > > Sure. Now posted: https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00427.html > The other thing I can do is have: > > blkdebug_version() { > assert(...); > return common version; > } That's the approach I went with in 14/15 of that series.
diff --git a/block/blkdebug.c b/block/blkdebug.c index b25856c4..f1539db6 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -641,16 +641,6 @@ 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) -{ - *pnum = nb_sectors; - *file = bs->file->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | - (sector_num << BDRV_SECTOR_BITS); -} - static void blkdebug_close(BlockDriverState *bs) { BDRVBlkdebugState *s = bs->opaque; @@ -925,7 +915,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_get_block_status = bdrv_co_get_block_status_from_file, .bdrv_debug_event = blkdebug_debug_event, .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, diff --git a/block/commit.c b/block/commit.c index 524bd549..5b04f832 100644 --- a/block/commit.c +++ b/block/commit.c @@ -247,16 +247,6 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs, return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); } -static int64_t coroutine_fn bdrv_commit_top_get_block_status( - BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, - BlockDriverState **file) -{ - *pnum = nb_sectors; - *file = bs->backing->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | - (sector_num << BDRV_SECTOR_BITS); -} - static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts) { bdrv_refresh_filename(bs->backing->bs); @@ -282,7 +272,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_commit_top_get_block_status, + .bdrv_co_get_block_status = bdrv_co_get_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/io.c b/block/io.c index a1aee01d..2487cc03 100644 --- a/block/io.c +++ b/block/io.c @@ -1706,6 +1706,30 @@ typedef struct BdrvCoGetBlockStatusData { bool done; } BdrvCoGetBlockStatusData; +int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs, + int64_t sector_num, + int nb_sectors, int *pnum, + BlockDriverState **file) +{ + assert(bs->file && bs->file->bs); + *pnum = nb_sectors; + *file = bs->file->bs; + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | + (sector_num << BDRV_SECTOR_BITS); +} + +int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs, + int64_t sector_num, + int nb_sectors, int *pnum, + BlockDriverState **file) +{ + assert(bs->backing && bs->backing->bs); + *pnum = nb_sectors; + *file = bs->backing->bs; + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | + (sector_num << BDRV_SECTOR_BITS); +} + /* * Returns the allocation status of the specified sectors. * Drivers not implementing the functionality are assumed to not support diff --git a/block/mirror.c b/block/mirror.c index 61a862dc..e8bf5f40 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1052,16 +1052,6 @@ static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs) return bdrv_co_flush(bs->backing->bs); } -static int64_t coroutine_fn bdrv_mirror_top_get_block_status( - BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, - BlockDriverState **file) -{ - *pnum = nb_sectors; - *file = bs->backing->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | - (sector_num << BDRV_SECTOR_BITS); -} - static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes, BdrvRequestFlags flags) { @@ -1108,7 +1098,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_mirror_top_get_block_status, + .bdrv_co_get_block_status = bdrv_co_get_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/include/block/block_int.h b/include/block/block_int.h index 15fa6021..a42c2a6f 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -945,6 +945,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c, uint64_t perm, uint64_t shared, uint64_t *nperm, uint64_t *nshared); +/* + * Default implementation for drivers to pass bdrv_co_get_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); +/* + * Default implementation for drivers to pass bdrv_co_get_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); 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);