From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34068) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaSVi-0007tl-IQ for qemu-devel@nongnu.org; Fri, 11 Sep 2015 13:53:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZaSVh-0002H3-Iz for qemu-devel@nongnu.org; Fri, 11 Sep 2015 13:53:18 -0400 Date: Fri, 11 Sep 2015 19:53:09 +0200 From: Kevin Wolf Message-ID: <20150911175309.GD5164@noname.redhat.com> References: <1441990071-7205-1-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1441990071-7205-1-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH] qcow2: Make qcow2_alloc_bytes() more explicit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org Am 11.09.2015 um 18:47 hat Max Reitz geschrieben: > In case of -EAGAIN returned by update_refcount(), we should discard the > cluster offset we were trying to allocate and request a new one, because > in theory that old offset might now be taken by a refcount block. > > In practice, this was not the case due to update_refcount() generally > returning strictly monotonic increasing cluster offsets. However, this > behavior is not set in stone, and it is also not obvious when looking at > qcow2_alloc_bytes() alone, so we should not rely on it. > > Reported-by: Kevin Wolf > Signed-off-by: Max Reitz > --- > block/qcow2-refcount.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index e8430ec..c30bb14 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -949,11 +949,17 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) > > if (!offset || ROUND_UP(offset, s->cluster_size) != new_cluster) { > offset = new_cluster; > + free_in_cluster = s->cluster_size; > + } else { > + free_in_cluster += s->cluster_size; > } > } This doesn't actually do anything except confuse the reader, but as the value of free_in_cluster doesn't matter in the second iteration because offset == 0, this is correct. > assert(offset); > ret = update_refcount(bs, offset, size, 1, false, QCOW2_DISCARD_NEVER); > + if (ret < 0) { > + offset = 0; > + } > } while (ret == -EAGAIN); > if (ret < 0) { > return ret; Thanks, applied to the block branch. Kevin