Re: [Qemu-devel] [PATCH v9 01/13] qcow2: Unallocate unmapped zero clusters if no backing file

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

On 04/12/2017 04:49 AM, Kevin Wolf wrote:
>> Furthermore, this matches the behavior of discard_single_l2(), in
>> favoring an unallocated cluster over a zero cluster when full
>> discard is requested.
> 
> The only use for "full discard" is qcow2_make_empty(). It explicitly
> requests that the backing file becomes visible again. This is a
> completely different case.

No, let's look at that code - I'm not referring to the
qcow2_make_empty() case, but to the bdrv_pdiscard() case:


        /*
         * If full_discard is false, make sure that a discarded area
reads back
         * as zeroes for v3 images (we cannot do it for v2 without actually
         * writing a zero-filled buffer). We can skip the operation if the
         * cluster is already marked as zero, or if it's unallocated and we
         * don't have a backing file.
         *
         * TODO We might want to use bdrv_get_block_status(bs) here, but
we're
         * holding s->lock, so that doesn't work today.
         *
         * If full_discard is true, the sector should not read back as
zeroes,
         * but rather fall through to the backing file.
         */
        switch (qcow2_get_cluster_type(old_l2_entry)) {
            case QCOW2_CLUSTER_UNALLOCATED:
                if (full_discard || !bs->backing) {
                    continue;
                }
                break;


That says: if our cluster is currently unallocated, and we have no
backing file, then LEAVE it unallocated (rather than marking it
QCOW2_CLUSTER_ZERO), even through bdrv_pdiscard.

Meanwhile, the next lines:

            case QCOW2_CLUSTER_ZERO:
                if (!full_discard) {
                    continue;
                }

state that if we are doing bdrv_pdiscard(), and the cluster already
reads as zeroes, then minimize the churn by not changing the cluster
(and that in turn means we do NOT lose a preallocation).

> 
> In other words, in order to stay consistent between discard and
> write_zeroes from a guest POV, we need to leave this code alone.

Again, my argument is that if we have no backing file, and the cluster
is previously unallocated, then leaving it unallocated is equivalent to
changing it to QCOW2_CLUSTER_ZERO.  There is no difference to the guest
POV by this change.

> 
>> Meanwhile, version 2 qcow2 files (compat=0.10) lack support for an
>> explicit zero cluster.  This minor tweak therefore allows us to turn
>> write zeroes with unmap into an actual unallocation on those files,
>> where they used to return -ENOTSUP and cause an allocation due to
>> the fallback to explicitly written zeroes.
> 
> Okay, this is true.
> 
> But I doubt that making write_zeroes more efficient on v2 images without
> a backing file is really worth any extra complexity at this point...

My argument is that it is not much complexity, and that making this
change coupled with the new qemu-iotest 179 in patch 2/13 (which fails
without this change) enabled me to test qemu-io -c 'w -z -u' (which
nothing else was testing) and ensure that I'm not regression when the
rest of the series further tweaks things to be byte-based.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]