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(-)
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);
--
2.11.0
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> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2026 Red Hat, Inc.