From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48642) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8DdO-00027G-Td for qemu-devel@nongnu.org; Mon, 05 Jan 2015 14:48:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y8DdK-0008Np-Tw for qemu-devel@nongnu.org; Mon, 05 Jan 2015 14:48:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39426) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8DdK-0008Nj-Mh for qemu-devel@nongnu.org; Mon, 05 Jan 2015 14:48:10 -0500 Message-ID: <54AAEA73.30904@redhat.com> Date: Mon, 05 Jan 2015 14:48:03 -0500 From: John Snow MIME-Version: 1.0 References: <1419692504-29373-1-git-send-email-peter@lekensteyn.nl> <1419692504-29373-11-git-send-email-peter@lekensteyn.nl> In-Reply-To: <1419692504-29373-11-git-send-email-peter@lekensteyn.nl> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/10] block/dmg: improve zeroes handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Wu , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 12/27/2014 10:01 AM, Peter Wu wrote: > Disk images may contain large all-zeroes gaps (1.66k sectors or 812 MiB > is seen in the real world). These blocks (type 2) do not need to be > extracted into a temporary buffer, there is no need to allocate memory > for these blocks nor to check its length. > > (For the test image, the maximum uncompressed size is 1054371 bytes, > probably for a bzip2-compressed block.) > > Signed-off-by: Peter Wu > --- > block/dmg.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/block/dmg.c b/block/dmg.c > index 67d4e2b..b84ba7e 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -137,7 +137,9 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk, > uncompressed_sectors = (s->lengths[chunk] + 511) / 512; > break; > case 2: /* zero */ > - uncompressed_sectors = s->sectorcounts[chunk]; > + /* as the all-zeroes block may be large, it is treated specially: the > + * sector is not copied from a large buffer, a simple memset is used > + * instead. Therefore uncompressed_sectors does not need to be set. */ > break; > } > > @@ -260,7 +262,9 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds, > s->sectorcounts[i] = buff_read_uint64(buffer, offset); > offset += 8; > > - if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) { > + /* all-zeroes sector (type 2) does not need to be "uncompressed" and can > + * therefore be unbounded. */ > + if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) { > error_report("sector count %" PRIu64 " for chunk %" PRIu32 > " is larger than max (%u)", > s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX); > @@ -621,9 +625,6 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num) > return -1; > } > break; > - case 2: /* zero */ > - memset(s->uncompressed_chunk, 0, 512 * s->sectorcounts[chunk]); > - break; This is just a personal preference, but I might actually prefer a little comment here that guards against anyone helpfully re-adding this case statement again in the future, if you really want to split it out. > } > s->current_chunk = chunk; > } > @@ -641,6 +642,13 @@ static int dmg_read(BlockDriverState *bs, int64_t sector_num, > if (dmg_read_chunk(bs, sector_num + i) != 0) { > return -1; > } > + /* Special case: current chunk is all zeroes. Do not perform a memcpy as > + * s->uncompressed_chunk may be too small to cover the large all-zeroes > + * section. */ > + if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */ > + memset(buf + i * 512, 0, 512); > + continue; > + } Why even call dmg_read_chunk first if we just ignore it? You can probably move this special case forward and only do the dmg_read_chunk if it fails. Or maybe we can keep this from becoming too fragmented by giving the read_chunk function access to the destination buffer somehow (parameter, structure member) and keeping all the logic for re-inflating the chunks together at the same layer. > sector_offset_in_chunk = sector_num + i - s->sectors[s->current_chunk]; > memcpy(buf + i * 512, > s->uncompressed_chunk + sector_offset_in_chunk * 512, 512); > Looks good, otherwise. Thank you :) --j