From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44387) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBhiZ-0001pm-ID for qemu-devel@nongnu.org; Wed, 12 Sep 2012 03:50:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TBhiT-0003m5-Bd for qemu-devel@nongnu.org; Wed, 12 Sep 2012 03:50:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36269) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBhiT-0003m0-33 for qemu-devel@nongnu.org; Wed, 12 Sep 2012 03:50:33 -0400 Message-ID: <50503EC3.30403@redhat.com> Date: Wed, 12 Sep 2012 09:50:27 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1344613185-12308-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1344613185-12308-6-git-send-email-wdongxu@linux.vnet.ibm.com> <504F0725.7000104@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V12 5/6] add-cow file format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dong Xu Wang Cc: qemu-devel@nongnu.org Am 12.09.2012 09:28, schrieb Dong Xu Wang: >>> +static bool is_allocated(BlockDriverState *bs, int64_t sector_num) >>> +{ >>> + BDRVAddCowState *s = bs->opaque; >>> + BlockCache *c = s->bitmap_cache; >>> + int64_t cluster_num = sector_num / SECTORS_PER_CLUSTER; >>> + uint8_t *table = NULL; >>> + uint64_t offset = ADD_COW_PAGE_SIZE * s->header.header_pages_size >>> + + (offset_in_bitmap(sector_num) & (~(c->entry_size - 1))); >>> + int ret = block_cache_get(bs, s->bitmap_cache, offset, >>> + (void **)&table, BLOCK_TABLE_BITMAP, ADD_COW_CACHE_ENTRY_SIZE); >> >> No matching block_cache_put? >> >>> + >>> + if (ret < 0) { >>> + return ret; >>> + } >>> + return table[cluster_num / 8 % ADD_COW_CACHE_ENTRY_SIZE] >>> + & (1 << (cluster_num % 8)); >>> +} >>> + >>> +static coroutine_fn int add_cow_is_allocated(BlockDriverState *bs, >>> + int64_t sector_num, int nb_sectors, int *num_same) >>> +{ >>> + BDRVAddCowState *s = bs->opaque; >>> + int changed; >>> + >>> + if (nb_sectors == 0) { >>> + *num_same = 0; >>> + return 0; >>> + } >>> + >>> + if (s->header.features & ADD_COW_F_All_ALLOCATED) { >>> + *num_same = nb_sectors - 1; >> >> Why - 1? >> >>> + return 1; >>> + } >>> + changed = is_allocated(bs, sector_num); >>> + >>> + for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) { >>> + if (is_allocated(bs, sector_num + *num_same) != changed) { >>> + break; >>> + } >>> + } >>> + return changed; >>> +} >>> +static int bdrv_add_cow_truncate(BlockDriverState *bs, int64_t size) >>> +{ >>> + BDRVAddCowState *s = bs->opaque; >>> + int sector_per_byte = SECTORS_PER_CLUSTER * 8; >>> + int ret; >>> + uint32_t bitmap_pos = s->header.header_pages_size * ADD_COW_PAGE_SIZE; >>> + int64_t bitmap_size = >>> + (size / BDRV_SECTOR_SIZE + sector_per_byte - 1) / sector_per_byte; >>> + bitmap_size = (bitmap_size + ADD_COW_CACHE_ENTRY_SIZE - 1) >>> + & (~(ADD_COW_CACHE_ENTRY_SIZE - 1)); >>> + >>> + ret = bdrv_truncate(bs->file, bitmap_pos + bitmap_size); >>> + if (ret < 0) { >>> + return ret; >>> + } >>> + return 0; >>> +} >> >> So you don't truncate s->image_file? Does this work? > > s->image_file should be truncated? Image file can have a larger virtual size > than backing_file, my understanding is we should not truncate image file. I'm talking about s->image_hd, not bs->backing_hd. You are right that the backing file should not be changed. But the associated raw image should be resized, shouldn't it? >>> +static coroutine_fn int add_cow_co_flush(BlockDriverState *bs) >>> +{ >>> + BDRVAddCowState *s = bs->opaque; >>> + int ret; >>> + >>> + qemu_co_mutex_lock(&s->lock); >>> + ret = block_cache_flush(bs, s->bitmap_cache, BLOCK_TABLE_BITMAP, >>> + ADD_COW_CACHE_ENTRY_SIZE); >>> + qemu_co_mutex_unlock(&s->lock); >>> + return ret; >>> +} >> >> What about flushing s->image_file? >>> +typedef struct AddCowHeader { >>> + uint64_t magic; >>> + uint32_t version; >>> + >>> + uint32_t backing_filename_offset; >>> + uint32_t backing_filename_size; >>> + >>> + uint32_t image_filename_offset; >>> + uint32_t image_filename_size; >>> + >>> + uint64_t features; >>> + uint64_t optional_features; >>> + uint32_t header_pages_size; >>> +} QEMU_PACKED AddCowHeader; >> >> Why aren't backing/image_file_format part of the header here? They are >> in the spec. It would also simplify some offset calculation code. >> > > Anthony said "It's far better to shrink the size of the header and use > an offset/len > pointer to the backing file string. Limiting backing files to 1023 is > unacceptable" > > http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg04110.html > > So I use offset and length instead of using string directly. I'm talking about the format, not the path. Kevin