From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:43589) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUcMb-00022a-G2 for qemu-devel@nongnu.org; Thu, 09 Jun 2011 06:21:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QUcMa-0003nL-3U for qemu-devel@nongnu.org; Thu, 09 Jun 2011 06:21:21 -0400 Received: from mail-pv0-f173.google.com ([74.125.83.173]:38849) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QUcMZ-0003n4-LQ for qemu-devel@nongnu.org; Thu, 09 Jun 2011 06:21:19 -0400 Received: by pvg3 with SMTP id 3so695387pvg.4 for ; Thu, 09 Jun 2011 03:21:18 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20110609075828.GA8285@stefanha-thinkpad.localdomain> References: <20110609075828.GA8285@stefanha-thinkpad.localdomain> From: Fam Zheng Date: Thu, 9 Jun 2011 18:20:58 +0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 01/12] VMDK: Introduced VmdkExtent List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , qemu-devel@nongnu.org, Christoph Hellwig On Thu, Jun 9, 2011 at 3:58 PM, Stefan Hajnoczi wrote: > On Sat, Jun 04, 2011 at 08:40:22AM +0800, Fam Zheng wrote: >> =C2=A0 fail: >> - =C2=A0 =C2=A0qemu_free(s->l1_backup_table); >> - =C2=A0 =C2=A0qemu_free(s->l1_table); >> - =C2=A0 =C2=A0qemu_free(s->l2_cache); >> + =C2=A0 =C2=A0if(s->extents) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_free(s->extents[0].l1_backup_table); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_free(s->extents[0].l1_table); >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_free(s->extents[0].l2_cache); >> + =C2=A0 =C2=A0} > > for (i =3D 0; i < s->num_extents; i++) { > =C2=A0 =C2=A0qemu_free(s->extents[i].l1_backup_table); > =C2=A0 =C2=A0qemu_free(s->extents[i].l1_table); > =C2=A0 =C2=A0qemu_free(s->extents[i].l2_cache); > } > qemu_free(s->extents); > This looks better, although num_extents is 1 at most here. >> +static int find_extent(BDRVVmdkState *s, int64_t sector_num, int start_= idx) >> +{ >> + =C2=A0 =C2=A0int ext_idx =3D start_idx; >> + =C2=A0 =C2=A0while (ext_idx < s->num_extents >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&& sector_num >=3D s->extents= [ext_idx].sectors) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0sector_num -=3D s->extents[ext_idx].sectors= ; >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0ext_idx++; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0if (ext_idx =3D=3D s->num_extents) return -1; >> + =C2=A0 =C2=A0return ext_idx; >> +} > > Callers don't really need the index, they just want the extent and an > optimized way of repeated calls to avoid O(N^2) find_extent() times: > > static VmdkExtent *find_extent(BDRVVmdkState *s, int64_t sector_num, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 VmdkExtent *start_extent) > { > =C2=A0 =C2=A0VmdkExtent *extent =3D start_extent; > > =C2=A0 =C2=A0if (!start_extent) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0extent =3D &s->extent[0]; > =C2=A0 =C2=A0} > > =C2=A0 =C2=A0while (extent < &s->extents[s->num_extents]) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (sector_num < extent->end) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return extent; > =C2=A0 =C2=A0 =C2=A0 =C2=A0} > =C2=A0 =C2=A0 =C2=A0 =C2=A0extent++; > =C2=A0 =C2=A0} > =C2=A0 =C2=A0return NULL; > } > > I added the VmdkExtent.end field so that this function can be called > repeatedly for an increasing series of sector_num values. =C2=A0It seems = like > your code would fail when called with a non-0 index since sector_num -=3D > s->extents[ext_idx].sectors is incorrect when starting from an arbitrary > extent_idx. > Parameter sector_num which I meant was relative to start_idx, so caller passes a relative sector_num when calling with a non-0 index. >> + >> =C2=A0static int vmdk_is_allocated(BlockDriverState *bs, int64_t sector_= num, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int nb_sectors, int *pnum) >> =C2=A0{ >> =C2=A0 =C2=A0 =C2=A0BDRVVmdkState *s =3D bs->opaque; >> - =C2=A0 =C2=A0int index_in_cluster, n; >> - =C2=A0 =C2=A0uint64_t cluster_offset; >> >> - =C2=A0 =C2=A0cluster_offset =3D get_cluster_offset(bs, NULL, sector_nu= m << 9, 0); >> - =C2=A0 =C2=A0index_in_cluster =3D sector_num % s->cluster_sectors; >> - =C2=A0 =C2=A0n =3D s->cluster_sectors - index_in_cluster; >> + =C2=A0 =C2=A0int index_in_cluster, n, ret; >> + =C2=A0 =C2=A0uint64_t offset; >> + =C2=A0 =C2=A0VmdkExtent *extent; >> + =C2=A0 =C2=A0int ext_idx; >> + >> + =C2=A0 =C2=A0ext_idx =3D find_extent(s, sector_num, 0); >> + =C2=A0 =C2=A0if (ext_idx =3D=3D -1) return 0; >> + =C2=A0 =C2=A0extent =3D &s->extents[ext_idx]; >> + =C2=A0 =C2=A0if (s->extents[ext_idx].flat) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0n =3D extent->sectors - sector_num; > > If I have two flat extents: > Extent A: 0 =C2=A0 =C2=A0 - 1.5MB > Extent B: 1.5MB - 2MB > > And I call vmdk_is_allocated(sector_num=3D1.5MB), then n =3D 512KB - 1.5M= B > which is negative! =C2=A0Also n is an int but it should be an int64_t (or > uint64_t) which can hold sector units. You are right. I did this because I wasn't considering multi extent situations yet in this patch, and when introducing multi extents support this problem is fixed. --=20 Best regards! Fam Zheng