Re: [Qemu-devel] [PATCH] qcow2: Rewrite qcow2_alloc_bytes()

[Max Reitz <mreitz@redhat.com>]

On 2015-02-04 at 16:59, Eric Blake wrote:
> On 02/04/2015 12:53 PM, Max Reitz wrote:
>
> My review jumps around a bit; try to follow the numbers to learn my
> stream of consciousness on reviewing it.
>
> tl;dr:
> It looks like this is functionally correct (you won't break behavior),
> but that you have a pessimization bug (see [7]), so you ought to spin v2.
>
>> qcow2_alloc_bytes() is a function with insufficient error handling and
>> an unnecessary goto. This patch rewrites it.
> Not clear what the added error handling is from just the commit message. [1]

Well, there are two cases and I didn't really mind writing it into the 
commit message since this patch completely rewrites the function anyway.

[snip]

>> +
>> +        cluster_end = start_of_cluster(s, s->free_byte_offset) +
>> +            s->cluster_size;
> [6] cluster_end is now either s->cluster_size (s->free_byte_offset was
> 0) or the first cluster-aligned address after s->free_byte_offset (will
> tell us if new_cluster is contiguous). If I'm not mistaken, you could
> also write this as:
>
> cluster_end = ROUND_UP(s->free_byte_offset, s->cluster_size);
>
> which would give the same value for a non-zero s->free_byte_offset, but
> the value 0 instead of s->cluster_size if s->free_byte_offset is 0.
> I'll analyze at point [7] what semantic difference that would make.

Indeed. I was wondering about s->free_byte_offset maybe being aligned to 
a cluster boundary and thus breaking this, but that can never happen (if 
it is aligned, it will be set to 0 at the end of this function). If it 
did happen, the free_in_cluster calculation would break, too.

I'll add an assert(!s->free_byte_offset || offset_into_cluster(s, 
s->free_byte_offset)); at the start of this function (which might have 
avoided "[0] ... I finally figured it out").

>> +
>> +        if (!s->free_cluster_index || cluster_end != new_cluster) {
>> +            s->free_byte_offset = new_cluster;
>> +        }
> [7] This was the only mention of s->free_cluster_index in the patch.

Oops, that's because I meant to use s->free_byte_offset.

> I
> had to hunt for it's use in the code base;

Sorry. :-/

> there are only two places in
> the code base that set it to non-zero: alloc_clusters_noref() and
> update_refcount().  But alloc_clusters_noref() is called by
> qcow2_alloc_clusters(), which we just called.  Therefore, the left half
> of the || is always true, and you always send up changing
> s->free_byte_offset (that is, when you allocate a new cluster, you never
> reuse the tail of an existing cluster, even if the two were contiguous).
> [11]
>
> At one point in my review, I wondered if point [0] should assert that
> !s->free_byte_offset should imply !s->free_cluster_index (more on that
> at [10], but I convinced myself that would be wrong).
>
> Let's ignore the left half of the ||, and focus on the right half.  If
> s->free_byte_offset was non-zero, you now know whether the new cluster
> is contiguous (use the existing tail, and the overflow into the new
> cluster is safe) or not (use only the new cluster); either way,
> s->free_byte_offset is now the point where the compressed data will be
> written.  If s->free_byte_offset was 0, you want to unconditionally
> change it to the start of the just-allocated cluster.
>
> Note that if you used my suggestion above at making cluster_end == 0
> rather than s->cluster_size for the !s->free_byte_offset case, then you
> are guaranteed that cluster_end != new_cluster is a sufficient test for
> when to correctly rewrite s->free_byte_offset (since new_cluster will be
> non-zero, because qcow2_alloc_clusters() will never return the header
> cluster); whereas with your code, there is a very minute chance that
> qcow2_alloc_clusters() could be equal to s->cluster_size (that is, the
> very first cluster after the header, perhaps possible if earlier actions
> on the file moved the L1 and refcount table to later in the file and
> freed up cluster index 1).  Using just the right half of the ||, if that
> rare chance under your code happened, then we would NOT rewrite
> s->free_byte_offset, and actually have a bug of returning 0 to the caller.

That silly small mistake made your whole review so much more 
difficult... I'm sorry, again.

>> +    }
>> +
>> +    if (offset_into_cluster(s, s->free_byte_offset)) {
> I'd feel a bit better if you added
> assert(s->free_byte_offset);
> just before this if (which happens to be the case with your
> pessimization on the left half of || [7], and would also be the case
> with my proposed simplification of [6]).

Yes, why not.

Thanks for your review!

Max