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

[Peter Wu <peter@lekensteyn.nl>]

On Monday 05 January 2015 14:48:03 John Snow wrote:
> 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.

Ok, makes sense. I'll add an appropriate comment here.

> >           }
> >           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.

dmg_read_chunk is needed to find the current sector (which requires a
binary search if the sector is not within the current chunk). I have
updated the comments to clarify this.

> 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.

dmg_read_chunk could be modified to return a pointer inside
s->uncompressed_chunk (adjusted to the sector number), but this just
moves the special case inside dmg_read_chunk. Alternatively, the whole
memcpy/memset stuff could be moved into dmg_read_chunk, but then the
function could be renamed to dmg_read_into_sector.

I treat dmg_read_chunk as a function which reads the current chunk (if
not already read), and returns the cached chunk otherwise. Then the
caller can decide on the part of the chunk it is interested in. As the
zero chunk does not really update the "current chunk contents", it'll
need special treatment.

The "continue" keyword is used while it actually means "either memset or
memcpy", this approach was chosen to avoid re-indenting the below memcpy
code.

Kind regards,
Peter

> >           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