| Message ID | 20190410202033.28617-5-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Deal with filters | expand |
10.04.2019 23:20, Max Reitz wrote: > With bdrv_filtered_rw_bs(), we can easily handle this default filter > behavior in bdrv_co_block_status(). > > blkdebug wants to have an additional assertion, so it keeps its own > implementation, except bdrv_co_block_status_from_file() needs to be > inlined there. > > Suggested-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/block/block_int.h | 22 ----------------- > block/blkdebug.c | 7 ++++-- > block/blklogwrites.c | 1 - > block/commit.c | 1 - > block/copy-on-read.c | 2 -- > block/io.c | 51 +++++++++++++-------------------------- > block/mirror.c | 1 - > block/throttle.c | 1 - > 8 files changed, 22 insertions(+), 64 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index d0309e6307..76c7c0a111 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1187,28 +1187,6 @@ 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_block_status() to > - * their 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_block_status() to > - * their backing 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/blkdebug.c b/block/blkdebug.c > index efd9441625..7950ae729c 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -637,8 +637,11 @@ static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs, > BlockDriverState **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); > + assert(bs->file && bs->file->bs); > + *pnum = bytes; > + *map = offset; > + *file = bs->file->bs; > + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; directly inlined, OK > } > > static void blkdebug_close(BlockDriverState *bs) > diff --git a/block/blklogwrites.c b/block/blklogwrites.c > index eb2b4901a5..1eb4a5c613 100644 > --- a/block/blklogwrites.c > +++ b/block/blklogwrites.c > @@ -518,7 +518,6 @@ static BlockDriver bdrv_blk_log_writes = { > .bdrv_co_pwrite_zeroes = blk_log_writes_co_pwrite_zeroes, > .bdrv_co_flush_to_disk = blk_log_writes_co_flush_to_disk, > .bdrv_co_pdiscard = blk_log_writes_co_pdiscard, > - .bdrv_co_block_status = bdrv_co_block_status_from_file, > > .is_filter = true, > .strong_runtime_opts = blk_log_writes_strong_runtime_opts, > diff --git a/block/commit.c b/block/commit.c > index 252007fd57..c366ee9655 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -254,7 +254,6 @@ 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_block_status = bdrv_co_block_status_from_backing, > .bdrv_refresh_filename = bdrv_commit_top_refresh_filename, > .bdrv_child_perm = bdrv_commit_top_child_perm, > > diff --git a/block/copy-on-read.c b/block/copy-on-read.c > index 53972b1da3..fe9260163c 100644 > --- a/block/copy-on-read.c > +++ b/block/copy-on-read.c > @@ -150,8 +150,6 @@ static BlockDriver bdrv_copy_on_read = { > .bdrv_eject = cor_eject, > .bdrv_lock_medium = cor_lock_medium, > > - .bdrv_co_block_status = bdrv_co_block_status_from_file, > - > .bdrv_recurse_is_first_non_filter = cor_recurse_is_first_non_filter, > > .has_variable_length = true, > diff --git a/block/io.c b/block/io.c > index 5c33ecc080..8d124bae5c 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1993,36 +1993,6 @@ typedef struct BdrvCoBlockStatusData { > bool done; > } BdrvCoBlockStatusData; > > -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 = bytes; > - *map = offset; > - *file = bs->file->bs; > - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; > -} > - > -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 = bytes; > - *map = offset; > - *file = bs->backing->bs; > - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; > -} > - > /* > * Returns the allocation status of the specified sectors. > * Drivers not implementing the functionality are assumed to not support > @@ -2063,6 +2033,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, > BlockDriverState *local_file = NULL; > int64_t aligned_offset, aligned_bytes; > uint32_t align; > + bool has_filtered_child; > > assert(pnum); > *pnum = 0; > @@ -2088,7 +2059,8 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, > > /* Must be non-NULL or bdrv_getlength() would have failed */ > assert(bs->drv); > - if (!bs->drv->bdrv_co_block_status) { > + has_filtered_child = bs->drv->is_filter && bdrv_filtered_rw_child(bs); > + if (!bs->drv->bdrv_co_block_status && !has_filtered_child) { > *pnum = bytes; > ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED; > if (offset + bytes == total_size) { > @@ -2109,9 +2081,20 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, > aligned_offset = QEMU_ALIGN_DOWN(offset, align); > aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset; > > - ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset, > - aligned_bytes, pnum, &local_map, > - &local_file); > + if (bs->drv->bdrv_co_block_status) { > + ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset, > + aligned_bytes, pnum, &local_map, > + &local_file); > + } else { > + /* Default code for filters */ > + > + local_file = bdrv_filtered_rw_bs(bs); > + assert(local_file); > + > + *pnum = aligned_bytes; > + local_map = aligned_offset; > + ret = BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; > + } preexistent, but why default for filters is aligned and for other nodes is not? > if (ret < 0) { > *pnum = 0; > goto out; > diff --git a/block/mirror.c b/block/mirror.c > index 80cef587f0..2e521c726a 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1487,7 +1487,6 @@ 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_block_status = bdrv_co_block_status_from_backing, > .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, > .bdrv_child_perm = bdrv_mirror_top_child_perm, > > diff --git a/block/throttle.c b/block/throttle.c > index f64dcc27b9..b6922e734f 100644 > --- a/block/throttle.c > +++ b/block/throttle.c > @@ -259,7 +259,6 @@ static BlockDriver bdrv_throttle = { > .bdrv_reopen_prepare = throttle_reopen_prepare, > .bdrv_reopen_commit = throttle_reopen_commit, > .bdrv_reopen_abort = throttle_reopen_abort, > - .bdrv_co_block_status = bdrv_co_block_status_from_file, > > .bdrv_co_drain_begin = throttle_co_drain_begin, > .bdrv_co_drain_end = throttle_co_drain_end, >
On 21.05.19 10:57, Vladimir Sementsov-Ogievskiy wrote: > 10.04.2019 23:20, Max Reitz wrote: >> With bdrv_filtered_rw_bs(), we can easily handle this default filter >> behavior in bdrv_co_block_status(). >> >> blkdebug wants to have an additional assertion, so it keeps its own >> implementation, except bdrv_co_block_status_from_file() needs to be >> inlined there. >> >> Suggested-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> include/block/block_int.h | 22 ----------------- >> block/blkdebug.c | 7 ++++-- >> block/blklogwrites.c | 1 - >> block/commit.c | 1 - >> block/copy-on-read.c | 2 -- >> block/io.c | 51 +++++++++++++-------------------------- >> block/mirror.c | 1 - >> block/throttle.c | 1 - >> 8 files changed, 22 insertions(+), 64 deletions(-) [...] >> diff --git a/block/io.c b/block/io.c >> index 5c33ecc080..8d124bae5c 100644 >> --- a/block/io.c >> +++ b/block/io.c [...] >> @@ -2088,7 +2059,8 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, >> >> /* Must be non-NULL or bdrv_getlength() would have failed */ >> assert(bs->drv); >> - if (!bs->drv->bdrv_co_block_status) { >> + has_filtered_child = bs->drv->is_filter && bdrv_filtered_rw_child(bs); >> + if (!bs->drv->bdrv_co_block_status && !has_filtered_child) { >> *pnum = bytes; >> ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED; >> if (offset + bytes == total_size) { >> @@ -2109,9 +2081,20 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, >> aligned_offset = QEMU_ALIGN_DOWN(offset, align); >> aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset; >> >> - ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset, >> - aligned_bytes, pnum, &local_map, >> - &local_file); >> + if (bs->drv->bdrv_co_block_status) { >> + ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset, >> + aligned_bytes, pnum, &local_map, >> + &local_file); >> + } else { >> + /* Default code for filters */ >> + >> + local_file = bdrv_filtered_rw_bs(bs); >> + assert(local_file); >> + >> + *pnum = aligned_bytes; >> + local_map = aligned_offset; >> + ret = BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; >> + } > > > preexistent, but why default for filters is aligned and for other nodes is not? I suppose because the default code for other nodes has been written before the aligning code was introduced. I guess there is no good reason to enforce alignment in either case. It is important to do so when issuing a request to the driver because the driver is not required to be able to handle unaligned requests. If we completely forgo the driver and just go through to the next layer, it doesn’t really matter, I think. Well, I just kept it as it was before. O:-) Max
diff --git a/include/block/block_int.h b/include/block/block_int.h index d0309e6307..76c7c0a111 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1187,28 +1187,6 @@ 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_block_status() to - * their 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_block_status() to - * their backing 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/blkdebug.c b/block/blkdebug.c index efd9441625..7950ae729c 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -637,8 +637,11 @@ static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs, BlockDriverState **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); + assert(bs->file && bs->file->bs); + *pnum = bytes; + *map = offset; + *file = bs->file->bs; + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; } static void blkdebug_close(BlockDriverState *bs) diff --git a/block/blklogwrites.c b/block/blklogwrites.c index eb2b4901a5..1eb4a5c613 100644 --- a/block/blklogwrites.c +++ b/block/blklogwrites.c @@ -518,7 +518,6 @@ static BlockDriver bdrv_blk_log_writes = { .bdrv_co_pwrite_zeroes = blk_log_writes_co_pwrite_zeroes, .bdrv_co_flush_to_disk = blk_log_writes_co_flush_to_disk, .bdrv_co_pdiscard = blk_log_writes_co_pdiscard, - .bdrv_co_block_status = bdrv_co_block_status_from_file, .is_filter = true, .strong_runtime_opts = blk_log_writes_strong_runtime_opts, diff --git a/block/commit.c b/block/commit.c index 252007fd57..c366ee9655 100644 --- a/block/commit.c +++ b/block/commit.c @@ -254,7 +254,6 @@ 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_block_status = bdrv_co_block_status_from_backing, .bdrv_refresh_filename = bdrv_commit_top_refresh_filename, .bdrv_child_perm = bdrv_commit_top_child_perm, diff --git a/block/copy-on-read.c b/block/copy-on-read.c index 53972b1da3..fe9260163c 100644 --- a/block/copy-on-read.c +++ b/block/copy-on-read.c @@ -150,8 +150,6 @@ static BlockDriver bdrv_copy_on_read = { .bdrv_eject = cor_eject, .bdrv_lock_medium = cor_lock_medium, - .bdrv_co_block_status = bdrv_co_block_status_from_file, - .bdrv_recurse_is_first_non_filter = cor_recurse_is_first_non_filter, .has_variable_length = true, diff --git a/block/io.c b/block/io.c index 5c33ecc080..8d124bae5c 100644 --- a/block/io.c +++ b/block/io.c @@ -1993,36 +1993,6 @@ typedef struct BdrvCoBlockStatusData { bool done; } BdrvCoBlockStatusData; -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 = bytes; - *map = offset; - *file = bs->file->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; -} - -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 = bytes; - *map = offset; - *file = bs->backing->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; -} - /* * Returns the allocation status of the specified sectors. * Drivers not implementing the functionality are assumed to not support @@ -2063,6 +2033,7 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, BlockDriverState *local_file = NULL; int64_t aligned_offset, aligned_bytes; uint32_t align; + bool has_filtered_child; assert(pnum); *pnum = 0; @@ -2088,7 +2059,8 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, /* Must be non-NULL or bdrv_getlength() would have failed */ assert(bs->drv); - if (!bs->drv->bdrv_co_block_status) { + has_filtered_child = bs->drv->is_filter && bdrv_filtered_rw_child(bs); + if (!bs->drv->bdrv_co_block_status && !has_filtered_child) { *pnum = bytes; ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED; if (offset + bytes == total_size) { @@ -2109,9 +2081,20 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs, aligned_offset = QEMU_ALIGN_DOWN(offset, align); aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset; - ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset, - aligned_bytes, pnum, &local_map, - &local_file); + if (bs->drv->bdrv_co_block_status) { + ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset, + aligned_bytes, pnum, &local_map, + &local_file); + } else { + /* Default code for filters */ + + local_file = bdrv_filtered_rw_bs(bs); + assert(local_file); + + *pnum = aligned_bytes; + local_map = aligned_offset; + ret = BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; + } if (ret < 0) { *pnum = 0; goto out; diff --git a/block/mirror.c b/block/mirror.c index 80cef587f0..2e521c726a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1487,7 +1487,6 @@ 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_block_status = bdrv_co_block_status_from_backing, .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, .bdrv_child_perm = bdrv_mirror_top_child_perm, diff --git a/block/throttle.c b/block/throttle.c index f64dcc27b9..b6922e734f 100644 --- a/block/throttle.c +++ b/block/throttle.c @@ -259,7 +259,6 @@ static BlockDriver bdrv_throttle = { .bdrv_reopen_prepare = throttle_reopen_prepare, .bdrv_reopen_commit = throttle_reopen_commit, .bdrv_reopen_abort = throttle_reopen_abort, - .bdrv_co_block_status = bdrv_co_block_status_from_file, .bdrv_co_drain_begin = throttle_co_drain_begin, .bdrv_co_drain_end = throttle_co_drain_end,
With bdrv_filtered_rw_bs(), we can easily handle this default filter behavior in bdrv_co_block_status(). blkdebug wants to have an additional assertion, so it keeps its own implementation, except bdrv_co_block_status_from_file() needs to be inlined there. Suggested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> --- include/block/block_int.h | 22 ----------------- block/blkdebug.c | 7 ++++-- block/blklogwrites.c | 1 - block/commit.c | 1 - block/copy-on-read.c | 2 -- block/io.c | 51 +++++++++++++-------------------------- block/mirror.c | 1 - block/throttle.c | 1 - 8 files changed, 22 insertions(+), 64 deletions(-)