From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJMRJ-0005ob-Si for qemu-devel@nongnu.org; Thu, 05 Feb 2015 08:25:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJMRE-0007o4-Du for qemu-devel@nongnu.org; Thu, 05 Feb 2015 08:25:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJMRE-0007nr-69 for qemu-devel@nongnu.org; Thu, 05 Feb 2015 08:25:44 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t15DPgww026257 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 5 Feb 2015 08:25:43 -0500 Message-ID: <54D36F55.3080904@redhat.com> Date: Thu, 05 Feb 2015 08:25:41 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423079623-4253-1-git-send-email-mreitz@redhat.com> <54D2965B.3040309@redhat.com> In-Reply-To: <54D2965B.3040309@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qcow2: Rewrite qcow2_alloc_bytes() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi 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