From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L4eW7-0001Q6-5a for qemu-devel@nongnu.org; Mon, 24 Nov 2008 11:42:31 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L4eW6-0001OW-8b for qemu-devel@nongnu.org; Mon, 24 Nov 2008 11:42:30 -0500 Received: from [199.232.76.173] (port=37319 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L4eW6-0001OB-2s for qemu-devel@nongnu.org; Mon, 24 Nov 2008 11:42:30 -0500 Received: from yx-out-1718.google.com ([74.125.44.157]:47408) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L4eW5-0008Tk-Im for qemu-devel@nongnu.org; Mon, 24 Nov 2008 11:42:29 -0500 Received: by yx-out-1718.google.com with SMTP id 3so802175yxi.82 for ; Mon, 24 Nov 2008 08:42:28 -0800 (PST) Message-ID: <492AD970.9050805@codemonkey.ws> Date: Mon, 24 Nov 2008 10:42:24 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 3/5] Write table offset and size in one syscall. References: <20081123145248.22178.36228.stgit@dhcp-1-237.tlv.redhat.com> <20081123145312.22178.68255.stgit@dhcp-1-237.tlv.redhat.com> In-Reply-To: <20081123145312.22178.68255.stgit@dhcp-1-237.tlv.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Gleb Natapov wrote: > Otherwise if VM is killed between two writes data may be lost. > But if offset and size fields are at the same disk block one > write should update them both simultaneously. > > Signed-off-by: Gleb Natapov > --- > > block-qcow2.c | 27 ++++++++++----------------- > 1 files changed, 10 insertions(+), 17 deletions(-) > > diff --git a/block-qcow2.c b/block-qcow2.c > index 69f6414..7f99921 100644 > --- a/block-qcow2.c > +++ b/block-qcow2.c > @@ -429,8 +429,7 @@ static int grow_l1_table(BlockDriverState *bs, int min_size) > int new_l1_size, new_l1_size2, ret, i; > uint64_t *new_l1_table; > uint64_t new_l1_table_offset; > - uint64_t data64; > - uint32_t data32; > + uint8_t data[12]; > This assumes packing will happen correctly. > > new_l1_size = s->l1_size; > if (min_size <= new_l1_size) > @@ -460,14 +459,12 @@ static int grow_l1_table(BlockDriverState *bs, int min_size) > new_l1_table[i] = be64_to_cpu(new_l1_table[i]); > > /* set new table */ > - data64 = cpu_to_be64(new_l1_table_offset); > - if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_table_offset), > - &data64, sizeof(data64)) != sizeof(data64)) > - goto fail; > - data32 = cpu_to_be32(new_l1_size); > - if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), > - &data32, sizeof(data32)) != sizeof(data32)) > + *(uint32_t*)data = cpu_to_be32(new_l1_size); > + *(uint64_t*)&data[4] = cpu_to_be64(new_l1_table_offset); > + if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data, > + sizeof(data)) != sizeof(data)) > goto fail; > > Why not just introduces a uint8_t data[12] in this function, memcpy to the right offsets, and do one brdv_pwrite? Then you don't need to do weird things with packing. > qemu_free(s->l1_table); > free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t)); > s->l1_table_offset = new_l1_table_offset; > @@ -2278,8 +2275,7 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size) > int new_table_size, new_table_size2, refcount_table_clusters, i, ret; > uint64_t *new_table; > int64_t table_offset; > - uint64_t data64; > - uint32_t data32; > + uint8_t data[12]; > int old_table_size; > int64_t old_table_offset; > > @@ -2318,13 +2314,10 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size) > for(i = 0; i < s->refcount_table_size; i++) > be64_to_cpus(&new_table[i]); > > - data64 = cpu_to_be64(table_offset); > + *(uint64_t*)data = cpu_to_be64(table_offset); > + *(uint32_t*)&data[8] = cpu_to_be32(refcount_table_clusters); > if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset), > - &data64, sizeof(data64)) != sizeof(data64)) > - goto fail; > - data32 = cpu_to_be32(refcount_table_clusters); > - if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_clusters), > - &data32, sizeof(data32)) != sizeof(data32)) > + data, sizeof(data)) != sizeof(data)) > goto fail; > Same here. Alternatively, you could use the cpu_to_beXXs() variants and just pass in pointer offsets. Regards, Anthony Liguori > qemu_free(s->refcount_table); > old_table_offset = s->refcount_table_offset; > > > >