qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 3/7] dmg: Refactor and prepare dmg_read_chunk


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [PATCH v2 3/7] dmg: Refactor and prepare dmg_read_chunk() to cache random access points
Date: Fri, 5 May 2017 14:33:36 +0100
User-agent: Mutt/1.8.0 (2017-02-23)

On Thu, Apr 27, 2017 at 01:36:33PM +0530, Ashijeet Acharya wrote:
> Refactor dmg_read_chunk() to start making use of the new DMGReadState
> structure and do chunk and sector related calculations based on it.
> Add a new argument "DMGReadState *drs" to it.
> 
> Also, rename DMG_SECTORCOUNTS_MAX to DMG_SECTOR_MAX to avoid indentaion
> problems at some places inside dmg_read_chunk()
> 
> Signed-off-by: Ashijeet Acharya <address@hidden>
> ---
>  block/dmg.c | 159 
> +++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 94 insertions(+), 65 deletions(-)

This patch makes dmg_read_chunk() reread the chunk even if it has
already been read into s->uncompressed_chunk.  This is inefficient.  If
this is necessary for some reason then the commit description should
include a justification.

> diff --git a/block/dmg.c b/block/dmg.c
> index c6fe8b0..32623e2 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -38,7 +38,7 @@ enum {
>       * or truncating when converting to 32-bit types
>       */
>      DMG_LENGTHS_MAX = 64 * 1024 * 1024, /* 64 MB */
> -    DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512,
> +    DMG_SECTOR_MAX = DMG_LENGTHS_MAX / 512,
>  };
>  
>  static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
> @@ -260,10 +260,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, 
> DmgHeaderState *ds,
>  
>          /* 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) {
> +        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTOR_MAX) {
>              error_report("sector count %" PRIu64 " for chunk %" PRIu32
>                           " is larger than max (%u)",
> -                         s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
> +                         s->sectorcounts[i], i, DMG_SECTOR_MAX);
>              ret = -EINVAL;
>              goto fail;
>          }
> @@ -578,78 +578,106 @@ static inline uint32_t search_chunk(BDRVDMGState *s, 
> uint64_t sector_num)
>      return s->n_chunks; /* error */
>  }
>  
> -static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
> +static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num,
> +                                 DMGReadState *drs)
>  {
>      BDRVDMGState *s = bs->opaque;
>  
> +    int ret;
> +    uint32_t sector_offset;
> +    uint64_t sectors_read;
> +    uint32_t chunk;
> +
>      if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) {
> -        int ret;
> -        uint32_t chunk = search_chunk(s, sector_num);
> +        chunk = search_chunk(s, sector_num);
> +    } else {
> +        chunk = drs->saved_chunk_type;

chunk is an index into s->sectors[] and s->sectorcounts[].  It is not a
chunk type, so drs->saved_chunk_type is an odd name for this new field.

But now this makes me wonder - how is s->current_chunk different from
drs->saved_chunk_type?

> +    }
>  
> -        if (chunk >= s->n_chunks) {
> +    if (chunk >= s->n_chunks) {
> +            return -1;
> +    }
> +
> +    /* reset our access point cache if we had a change in current chunk */
> +    if (chunk != drs->saved_chunk_type) {
> +        cache_access_point(drs, NULL, -1, -1, -1, -1);
> +    }
> +

The results of the computation from here...

> +    sector_offset = sector_num - s->sectors[chunk];
> +
> +    if ((s->sectorcounts[chunk] - sector_offset) > DMG_SECTOR_MAX) {
> +        sectors_read = DMG_SECTOR_MAX;
> +    } else {
> +        sectors_read = s->sectorcounts[chunk] - sector_offset;
> +    }
> +
> +    /* truncate sectors read if it exceeds the 2MB buffer of qemu-img
> +     * convert */
> +    if ((sector_num % DMG_SECTOR_MAX) + sectors_read > DMG_SECTOR_MAX) {
> +        sectors_read = DMG_SECTOR_MAX - (sector_num % DMG_SECTOR_MAX);
> +    }

...to here are never used.  Please remove the unused code and variables
from this patch.  It may be necessary to add them back in a later patch
(I haven't looked ahead), but each patch must be self-contained and make
sense without foreknowledge of future patches.

> +
> +    s->current_chunk = s->n_chunks;
> +
> +    switch (s->types[chunk]) { /* block entry type */
> +    case 0x80000005: { /* zlib compressed */
> +        /* we need to buffer, because only the chunk as whole can be
> +         * inflated. */
> +        ret = bdrv_pread(bs->file, s->offsets[chunk],
> +                         s->compressed_chunk, s->lengths[chunk]);
> +        if (ret != s->lengths[chunk]) {
>              return -1;
>          }
>  
> -        s->current_chunk = s->n_chunks;
> -        switch (s->types[chunk]) { /* block entry type */
> -        case 0x80000005: { /* zlib compressed */
> -            /* we need to buffer, because only the chunk as whole can be
> -             * inflated. */
> -            ret = bdrv_pread(bs->file, s->offsets[chunk],
> -                             s->compressed_chunk, s->lengths[chunk]);
> -            if (ret != s->lengths[chunk]) {
> -                return -1;
> -            }
> -
> -            s->zstream.next_in = s->compressed_chunk;
> -            s->zstream.avail_in = s->lengths[chunk];
> -            s->zstream.next_out = s->uncompressed_chunk;
> -            s->zstream.avail_out = 512 * s->sectorcounts[chunk];
> -            ret = inflateReset(&s->zstream);
> -            if (ret != Z_OK) {
> -                return -1;
> -            }
> -            ret = inflate(&s->zstream, Z_FINISH);
> -            if (ret != Z_STREAM_END ||
> -                s->zstream.total_out != 512 * s->sectorcounts[chunk]) {
> -                return -1;
> -            }
> -            break; }
> -        case 0x80000006: /* bzip2 compressed */
> -            if (!dmg_uncompress_bz2) {
> -                break;
> -            }
> -            /* we need to buffer, because only the chunk as whole can be
> -             * inflated. */
> -            ret = bdrv_pread(bs->file, s->offsets[chunk],
> -                             s->compressed_chunk, s->lengths[chunk]);
> -            if (ret != s->lengths[chunk]) {
> -                return -1;
> -            }
> -
> -            ret = dmg_uncompress_bz2((char *)s->compressed_chunk,
> -                                     (unsigned int) s->lengths[chunk],
> -                                     (char *)s->uncompressed_chunk,
> -                                     (unsigned int)
> -                                         (512 * s->sectorcounts[chunk]));
> -            if (ret < 0) {
> -                return ret;
> -            }
> -            break;
> -        case 1: /* copy */
> -            ret = bdrv_pread(bs->file, s->offsets[chunk],
> -                             s->uncompressed_chunk, s->lengths[chunk]);
> -            if (ret != s->lengths[chunk]) {
> -                return -1;
> -            }
> -            break;
> -        case 2: /* zero */
> -            /* see dmg_read, it is treated specially. No buffer needs to be
> -             * pre-filled, the zeroes can be set directly. */
> +        s->zstream.next_in = s->compressed_chunk;
> +        s->zstream.avail_in = s->lengths[chunk];
> +        s->zstream.next_out = s->uncompressed_chunk;
> +        s->zstream.avail_out = 512 * s->sectorcounts[chunk];
> +        ret = inflateReset(&s->zstream);
> +        if (ret != Z_OK) {
> +            return -1;
> +        }
> +        ret = inflate(&s->zstream, Z_FINISH);
> +        if (ret != Z_STREAM_END ||
> +            s->zstream.total_out != 512 * s->sectorcounts[chunk]) {
> +            return -1;
> +        }
> +        break; }
> +    case 0x80000006: /* bzip2 compressed */
> +        if (!dmg_uncompress_bz2) {
>              break;
>          }
> -        s->current_chunk = chunk;
> +        /* we need to buffer, because only the chunk as whole can be
> +         * inflated. */
> +        ret = bdrv_pread(bs->file, s->offsets[chunk],
> +                         s->compressed_chunk, s->lengths[chunk]);
> +        if (ret != s->lengths[chunk]) {
> +            return -1;
> +        }
> +
> +        ret = dmg_uncompress_bz2((char *)s->compressed_chunk,
> +                                 (unsigned int) s->lengths[chunk],
> +                                 (char *)s->uncompressed_chunk,
> +                                 (unsigned int)
> +                                    (512 * s->sectorcounts[chunk]));
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        break;
> +    case 1: /* copy */
> +        ret = bdrv_pread(bs->file, s->offsets[chunk],
> +                         s->uncompressed_chunk, s->lengths[chunk]);
> +        if (ret != s->lengths[chunk]) {
> +            return -1;
> +        }
> +        break;
> +    case 2: /* zero */
> +        /* see dmg_read, it is treated specially. No buffer needs to be
> +         * pre-filled, the zeroes can be set directly. */
> +        break;
>      }
> +    s->current_chunk = chunk;
> +
>      return 0;
>  }
>  
> @@ -661,6 +689,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, 
> uint64_t bytes,
>      uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
>      int nb_sectors = bytes >> BDRV_SECTOR_BITS;
>      int ret, i;
> +    DMGReadState *drs = s->drs;
>  
>      assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
>      assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> @@ -671,7 +700,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, 
> uint64_t bytes,
>          uint32_t sector_offset_in_chunk;
>          void *data;
>  
> -        if (dmg_read_chunk(bs, sector_num + i) != 0) {
> +        if (dmg_read_chunk(bs, sector_num + i, drs) != 0) {
>              ret = -EIO;
>              goto fail;
>          }
> -- 
> 2.6.2
> 

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]