From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34569) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeSE7-0000GL-0b for qemu-devel@nongnu.org; Tue, 12 Jun 2012 10:37:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SeSE0-0007bY-Lh for qemu-devel@nongnu.org; Tue, 12 Jun 2012 10:37:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18903) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeSE0-0007au-DN for qemu-devel@nongnu.org; Tue, 12 Jun 2012 10:37:40 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q5CEbcAx014140 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 12 Jun 2012 10:37:38 -0400 Message-ID: <4FD75430.3090902@redhat.com> Date: Tue, 12 Jun 2012 16:37:36 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1339508835-15108-1-git-send-email-kwolf@redhat.com> <4FD74B8D.9090800@redhat.com> <4FD7506C.1030606@redhat.com> <4FD752AA.6000705@redhat.com> In-Reply-To: <4FD752AA.6000705@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qcow2: Simplify calculation for COW area at the end List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org Am 12.06.2012 16:31, schrieb Paolo Bonzini: > Il 12/06/2012 16:21, Kevin Wolf ha scritto: >>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >>>> index 9aee9fc..763b724 100644 >>>> --- a/block/qcow2-cluster.c >>>> +++ b/block/qcow2-cluster.c >>>> @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) >>>> } >>>> >>>> if (m->nb_available & (s->cluster_sectors - 1)) { >>>> - uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1); >>>> cow = true; >>>> qemu_co_mutex_unlock(&s->lock); >>>> - ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9), >>>> - m->nb_available - end, s->cluster_sectors); >>>> + ret = copy_sectors(bs, start_sect, cluster_offset, >>>> + m->nb_available, s->cluster_sectors); >>> >>> Do you need to add end to s->cluster_sectors too, so that "start_sect + >>> n_end" and "n_end - n_start" remain the same? >> >> You mean because n_end is now relative to start_sect instead of >> start_sect + end, right? > > Yes. Or more simply, because I was expecting no other uses of > start_sect, cluster_offset and n_start after reading your commit message. :) > >> I thought about it and I find this code is a bit confusing, but I think >> you're right that I need to replace n_end as well because it would be >> wrong for an allocating request than spans multiple clusters. I think >> this one should be right, would you agree? >> >> ret = copy_sectors(bs, start_sect, cluster_offset, >> m->nb_available, align_offset(m->nb_available, s->cluster_sectors)); > > The obvious expression would be > > s->cluster_sectors > + (m->nb_available & ~(uint64_t)(s->cluster_sectors - 1)) > > which is a bit different from align_offset. If m->nb_available is > already aligned, it returns the *next* aligned value rather than > m->nb_available itself. > > So the equivalent expression using align_offset would be this one: > > align_offset(m->nb_available+1, s->cluster_sectors) Heh, yes, you're thinking about equivalence of the very formula (which is what I would do in unknown code as well), whereas I think about a COW operation that ranges from a given sector to the end of the same cluster (and not the next one). It's eventually the same, because this statement is only executed when it's not aligned (i.e. the COW range isn't empty): if (m->nb_available & (s->cluster_sectors - 1)) { ... } Kevin