On 1/8/19 1:45 PM, Vladimir Sementsov-Ogievskiy wrote: > bdrv_co_block_status digs bs->file for additional, more accurate search > for hole inside region, reported as DATA by bs since 5daa74a6ebc. > > This accuracy is not free: assume we have qcow2 disk. Actually, qcow2 > knows, where are holes and where is data.
Not when it is has been preallocated (at which point the qcow2 metadata says the entire file is data, even though much of it may still be zeroes). > But every block_status > request calls lseek additionally. Assume a big disk, full of > data, in any iterative copying block job (or img convert) we'll call > lseek(HOLE) on every iteration, and each of these lseeks will have to > iterate through all metadata up to the end of file. It's obviously > ineffective behavior. And for many scenarios we don't need this lseek > at all. This part is true, and we have had several complaints from various places about the time spent doing lseek()s where the block layer fails to cache what was learned on the previous iteration, coupled with inefficient kernel/filesystem implementations where lseek is not as cheap as it should be. > > So, add an option to omit calling lseek. The commit message would do well with an example showing the usage of the parameter via a typical qemu-img command line that is currently negatively impacted. Or even better, where is the iotests change that serves as a unit test of the new knob? > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > > Hi all! > > This is a continuation of "do not lseek in qcow2 block_status" thread > (https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05749.html) > > qapi/block-core.json | 6 +++++- > block/file-posix.c | 13 ++++++++++++- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 762000f31f..ec5549e5c2 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2645,6 +2645,9 @@ > # migration. May cause noticeable delays if the > image > # file is large, do not use in production. > # (default: off) (since: 3.0) Pre-existing: "off" isn't a valid QAPI bool; better would be fixing all instances of this to "default: false". > +# @rough-block-status: If set, on block-status querying assume that the whole > +# file is data and do not try to find holes through > system > +# calls (default: off) (since 4.0) You merely copied and pasted bad wording for the default section, but you might as well fix it, whether or not we also fix the other instances in a separate patch. Possible better wording: @rough-block-status: True to skip system calls that probe for holes when querying for block status (default: false) Bike-shedding: Since the point of this knob is to skip zero probing, could we instead name it '@probe-zeroes: True to use lseek to probe for holes (default: true)'? > # > # Since: 2.9 > ## > @@ -2653,7 +2656,8 @@ > '*pr-manager': 'str', > '*locking': 'OnOffAuto', > '*aio': 'BlockdevAioOptions', > - '*x-check-cache-dropped': 'bool' } } > + '*x-check-cache-dropped': 'bool', > + '*rough-block-status': 'bool' } } > > ## > # @BlockdevOptionsNull: > diff --git a/block/file-posix.c b/block/file-posix.c > index d8f0b93752..3f6d76a5dc 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -167,6 +167,7 @@ typedef struct BDRVRawState { > bool has_fallocate; > bool needs_alignment; > bool check_cache_dropped; > + bool rough_block_status; > > PRManager *pr_mgr; > } BDRVRawState; > @@ -439,6 +440,13 @@ static QemuOptsList raw_runtime_opts = { > .type = QEMU_OPT_BOOL, > .help = "check that page cache was dropped on live migration > (default: off)" > }, > + { > + .name = "rough-block-status", > + .type = QEMU_OPT_BOOL, > + .help = "If set, on block-status querying assume that the whole " > + "file is data and do not try to find holes through > system " > + "calls (default: off)" Whatever wording changes we make to QAPI should be copied here. The idea makes sense, but without unit test coverage, I'm reluctant to leave R-b yet. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
