Re: [Qemu-devel] [PATCH 10/10] block/dmg: improve zeroes handling

[John Snow <jsnow@redhat.com>]

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 <peter@lekensteyn.nl>
> ---
>   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