Re: [Qemu-devel] [PATCH] file-posix: add rough-block-status parameter

[Kevin Wolf]

Am 08.01.2019 um 20:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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. 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.
> 
> So, add an option to omit calling lseek.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

I don't think adding an option is a good idea, people will not know this
option or forget to add it. Also, it's an option that you want to
specify almost always, so if we do add an option, maybe it should be the
default and an option should be added to enable recursive mode?

I haven't checked the full list of bdrv_co_block_status() callers yet.
So far we're talking about 'qemu-img convert' or the mirror job. Maybe
we need to check other callers, too, but for now I'll stick to these.

The interesting case seems to be preallocated qcow2 images, where the
clusters are allocated on the qcow2 level, but not in the image file.
If we don't do the lseek there, we'll have to read in the data and will
only then detect that it is zero (qemu-img only, mirror doesn't do this
yet - should it have an option to enable a zero-detecting mode? Can
detect-zeroes on the target node achieve the same thing?). So at least
in qemu-img, we don't get a different result, it's just a bit slower.

Also note that this is only metadata preallocation; full preallocation
will still return allocated for the protocol layer and so it will always
be slow.

Now, if this isn't about correctness, but about performance, maybe we
can do some heuristics instead of adding a user option? A very simple
approach could be, look at the lower layers only if we got a large
allocated area. If the image was preallocated, everything is allocated,
so if bdrv_co_block_status() returns a short range, it's a hint that
qcow2 knows _something_ at least. (That something could just be that
someone discarded something from a preallocated image, so we can't jump
to the conclusion that it's not preallocated - but it's certainly more
likely). And for small buffer, just reading it in and checking whether
it is zero can't be that much slower than finding a hole first.

So how about we look at the protocol layer only if we got an allocated
range of at least a megabyte or something? Or maybe make it depend on
the cluster size because with 2 MB clusters that will be always the
case, even if the image isn't preallocated.

Kevin