From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34691) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bItzc-0004um-9I for qemu-devel@nongnu.org; Fri, 01 Jul 2016 04:40:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bItzY-0006CI-3m for qemu-devel@nongnu.org; Fri, 01 Jul 2016 04:40:07 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:30299 helo=relay.sw.ru) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bItzX-0006Bp-KM for qemu-devel@nongnu.org; Fri, 01 Jul 2016 04:40:04 -0400 References: <1467272042-5195-1-git-send-email-vsementsov@virtuozzo.com> <5774E27D.60003@openvz.org> <9367c471-3cdd-ac6f-eb84-6aa3f4764dc3@redhat.com> <5775558F.4080705@openvz.org> <20160701081202.GA5838@noname.redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <57762C56.3010608@virtuozzo.com> Date: Fri, 1 Jul 2016 11:39:50 +0300 MIME-Version: 1.0 In-Reply-To: <20160701081202.GA5838@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , "Denis V. Lunev" Cc: John Snow , qemu-devel@nongnu.org, famz@redhat.com, qemu-block@nongnu.org, mreitz@redhat.com, stefanha@redhat.com On 01.07.2016 11:12, Kevin Wolf wrote: > Am 30.06.2016 um 19:23 hat Denis V. Lunev geschrieben: >> On 06/30/2016 07:40 PM, John Snow wrote: >>> On 06/30/2016 05:12 AM, Denis V. Lunev wrote: >>>> On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote: >>>>> After loading bitmap from image and setting IN_USE flag in it's header, >>>>> corresponding data (bitmap table and data clusters) becomes inconsistent >>>>> and is no longer needed. It is better to free bitmap table and >>>>> corresponding clusters from the image immediately after loading the >>>>> bitmap than free them when the bitmap is saved, or deleted or set >>>>> non-persistent. >>>>> >>>>> For now it is impossible to store only bitmap header without bitmap >>>>> table, as specification requires it. Storing zeroed bitmap table (one or >>>>> more clusters) is the only option to implement the behaviour similar to >>>>> specified above. >>>>> >>>>> The same problem is for just storing empty bitmaps. >>>>> >>>>> This patch allows storing only bitmap header for empty bitmaps. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>> --- >>>>> >>>>> Additional note. Should we also allow here bitmap_table_offset = 1, like >>>>> in bitmap table, for the bitmap with all bits set? I am not sure that it >>>>> is needed at all, but just to keep the company.. >>>>> >>>>> docs/specs/qcow2.txt | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt >>>>> index 80cdfd0..9826222 100644 >>>>> --- a/docs/specs/qcow2.txt >>>>> +++ b/docs/specs/qcow2.txt >>>>> @@ -435,9 +435,12 @@ Structure of a bitmap directory entry: >>>>> Offset into the image file at which the bitmap >>>>> table >>>>> (described below) for the bitmap starts. Must be >>>>> aligned to >>>>> a cluster boundary. >>>>> + Zero value means that bitmap table is not >>>>> allocated and the >>>>> + bitmap should be considered as empty (all bits >>>>> are zero). >>>>> 8 - 11: bitmap_table_size >>>>> - Number of entries in the bitmap table of the bitmap. >>>>> + Number of entries in the bitmap table of the >>>>> bitmap. It >>>>> + must be zero if bitmap_table_offset is zero. >>>>> 12 - 15: flags >>>>> Bit >>>> NACK >>>> >>>> no guys, we can not make this change at the moment. >>>> We do have QEMU available in the field which is working >>>> with the currently specified format. >>>> >>>> Den >>>> >>> But I think the new format is a /compatible/ change. Under the old spec, >>> I think this field is *NEVER* zero. >>> >>> Am I wrong? >> yes >> >> but as far as I can understand this breaks backward compatibility, >> i.e. software working with the current revision of the specification >> will not be able to load new images. > This is okay if it means that it cleanly errors out. I'm a bit worried > that it might take the 0 literally, though, and use the image header as > the bitmap table. Can you check that, Den? > > If that's the case, we can make the change only with an incompatible > feature bit, and that's probably not worth it. > > Kevin Current version will error out with "Bitmap doesn't satisfy the constraints.", because I check that int check_constraints(...) uint64_t phys_bitmap_bytes = (uint64_t)h->bitmap_table_size * cluster_size; uint64_t max_virtual_bits = (phys_bitmap_bytes * 8) << h->granularity_bits; int fail = .... || (disk_size > max_virtual_bits) || ... return fail ? -EINVAL : 0; } (max_virtual_bits will be zero if bitmap_table_size is zero) -- Best regards, Vladimir