| Message ID | 20190612221004.2317-35-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Deal with filters | expand |
13.06.2019 1:09, 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 cfefb00104..431fa38ea0 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1203,28 +1203,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 ec5a8c8edf..a5b58eadeb 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -257,7 +257,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 88e1c1f538..5a292de000 100644 > --- a/block/copy-on-read.c > +++ b/block/copy-on-read.c > @@ -161,8 +161,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 14f99e1c00..0a832e30a3 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1998,36 +1998,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 > @@ -2068,6 +2038,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; > @@ -2093,7 +2064,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) { > @@ -2114,9 +2086,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; I now in a little doubt: What is real difference between RAW for filters and UNALLOCATED for qcow2 (when we should look at backing) ? > + } > if (ret < 0) { > *pnum = 0; > goto out; > diff --git a/block/mirror.c b/block/mirror.c > index 3d767e3030..71bd7f7625 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1484,7 +1484,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 de1b6bd7e8..32ec56db0f 100644 > --- a/block/throttle.c > +++ b/block/throttle.c > @@ -269,7 +269,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 19.06.19 11:34, Vladimir Sementsov-Ogievskiy wrote: > 13.06.2019 1:09, 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 14f99e1c00..0a832e30a3 100644 >> --- a/block/io.c >> +++ b/block/io.c [...] >> @@ -2114,9 +2086,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; > > I now in a little doubt: > > What is real difference between RAW for filters and UNALLOCATED for qcow2 (when we > should look at backing) ? Maybe none, but I don’t think diving down that rabbit hole is going to make this seres shorter. Max
On 19.06.19 11:34, Vladimir Sementsov-Ogievskiy wrote: > 13.06.2019 1:09, 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 14f99e1c00..0a832e30a3 100644 >> --- a/block/io.c >> +++ b/block/io.c [...] >> @@ -2114,9 +2086,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; > > I now in a little doubt: > > What is real difference between RAW for filters and UNALLOCATED for qcow2 (when we > should look at backing) ? Maybe none, but I don’t think diving down that rabbit hole is going to make this series shorter. Max
13.06.2019 1:09, 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> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
21.06.2019 16:39, Vladimir Sementsov-Ogievskiy wrote: > 13.06.2019 1:09, 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> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Strange, I have this mail automatically returned back. Did you receive it?
diff --git a/include/block/block_int.h b/include/block/block_int.h index cfefb00104..431fa38ea0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1203,28 +1203,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 ec5a8c8edf..a5b58eadeb 100644 --- a/block/commit.c +++ b/block/commit.c @@ -257,7 +257,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 88e1c1f538..5a292de000 100644 --- a/block/copy-on-read.c +++ b/block/copy-on-read.c @@ -161,8 +161,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 14f99e1c00..0a832e30a3 100644 --- a/block/io.c +++ b/block/io.c @@ -1998,36 +1998,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 @@ -2068,6 +2038,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; @@ -2093,7 +2064,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) { @@ -2114,9 +2086,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 3d767e3030..71bd7f7625 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1484,7 +1484,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 de1b6bd7e8..32ec56db0f 100644 --- a/block/throttle.c +++ b/block/throttle.c @@ -269,7 +269,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(-)