[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
>
signature.asc
Description: PGP signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 3/7] dmg: Refactor and prepare dmg_read_chunk() to cache random access points,
Stefan Hajnoczi <=