Re: [Qemu-devel] [[PATCH v2] 1/1] vpc.c: Add VHD resize support

[Stefan Hajnoczi]

On Fri, Sep 19, 2014 at 12:59:46PM +0300, Lucian Petrut wrote:
> Note that this patch assumes that all the data blocks are written
> right after the BAT.

Are there any known files out there where this is not the case?

Perhaps an error should be printed if a data block that requires moving
does not appear in pagetable[].  That's an indication that the image is
not laid out as expected.

> +static int vpc_truncate(BlockDriverState *bs, int64_t offset)
> +{
> +    BDRVVPCState *s = bs->opaque;
> +    VHDFooter *footer = (VHDFooter *) s->footer_buf;
> +    VHDDynDiskHeader *dyndisk_header;
> +    void *buf = NULL;
> +    int64_t new_total_sectors, old_bat_size, new_bat_size,
> +            block_offset, new_block_offset, bat_offset;
> +    int32_t bat_value, data_blocks_required;
> +    int ret = 0;
> +    uint16_t cyls = 0;
> +    uint8_t heads = 0;
> +    uint8_t secs_per_cyl = 0;
> +    uint32_t new_num_bat_entries;
> +    uint64_t index, block_index, new_bat_right_limit;
> +
> +    if (offset & 511) {
> +        error_report("The new size must be a multiple of 512.");
> +        return -EINVAL;
> +    }
> +
> +    if (offset < bs->total_sectors * 512) {
> +        error_report("Shrinking vhd images is not supported.");
> +        return -ENOTSUP;
> +    }
> +
> +    if (cpu_to_be32(footer->type) == VHD_DIFFERENCING) {

footer_buf is big-endian so this should be be32_to_cpu()

> +        error_report("Resizing differencing vhd images is not supported.");
> +        return -ENOTSUP;
> +    }
> +
> +    old_bat_size = (s->max_table_entries * 4 + 511) & ~511;
> +    new_total_sectors = offset / BDRV_SECTOR_SIZE;
> +
> +    for (index = 0; new_total_sectors > (int64_t)cyls * heads * secs_per_cyl;
> +            index++) {
> +        if (calculate_geometry(new_total_sectors + index, &cyls, &heads,
> +                               &secs_per_cyl)) {
> +            return -EFBIG;
> +        }
> +    }
> +    new_total_sectors = (int64_t) cyls * heads * secs_per_cyl;
> +    new_num_bat_entries = (new_total_sectors + s->block_size / 512) /
> +                          (s->block_size / 512);

This expression doesn't just round up, it always adds an extra block.
Is this intentional?

I expected the numerator for rounding up to be:
  new_total_sectors + s->block_size / 512 - 1

> +
> +    if (cpu_to_be32(footer->type) == VHD_DYNAMIC) {

be32_to_cpu()

> +        new_bat_size = (new_num_bat_entries * 4 + 511) & ~511;
> +        /* Number of blocks required for extending the BAT */
> +        data_blocks_required = (new_bat_size - old_bat_size +
> +                                s->block_size - 1) / s->block_size;
> +        new_bat_right_limit = s->bat_offset + old_bat_size +
> +                              data_blocks_required *
> +                              (s->block_size + s->bitmap_size);
> +
> +        for (block_index = 0; block_index <
> +                data_blocks_required; block_index++){
> +            /*
> +             * The BAT has to be extended. We'll have to move the first
> +             * data block(s) to the end of the file, making room for the
> +             * BAT to expand. Also, the BAT entries have to be updated for
> +             * the moved blocks.
> +             */
> +
> +            block_offset = s->bat_offset + old_bat_size +
> +                           block_index * (s->block_size + s->bitmap_size);
> +            if (block_offset >= s->free_data_block_offset) {
> +                /*
> +                * Do not allocate a new block for the BAT if no data blocks
> +                * were previously allocated to the vhd image.
> +                */
> +                s->free_data_block_offset += (new_bat_size - old_bat_size);
> +                break;
> +            }
> +
> +            if (block_index == 0) {

Is this condition correct?  I expected !buf.  What happens during
the second loop iteration when block_index == 1 (we freed buf at the end
of the first iteration).

If I'm reading the code correctly this results in a NULL pointer
dereference.  That would mean you never executed this code path.

Please create a qemu-iotests test case to exercise the code paths in
your patch.  There are many examples you can use as a starting point in
tests/qemu-iotests/.  The basic overview is here:
http://qemu-project.org/Documentation/QemuIoTests

> +                buf = g_malloc(s->block_size + s->bitmap_size);

Please use qemu_blockalign()/qemu_vfree() for disk data buffers.  If the
file is opened with O_DIRECT there are memory alignment constraints.
Using qemu_blockalign() avoids the need for a bounce buffer in
block/raw-posix.c.

(Even if there is no performance concern, it's a good habit to follow
consistently so that we do align in the cases where the performance does
matter.)

> +            }
> +
> +            ret = bdrv_pread(bs->file, block_offset, buf,
> +                             s->block_size + s->bitmap_size);
> +            if (ret < 0) {
> +                goto out;
> +            }
> +
> +            new_block_offset = s->free_data_block_offset < 
> new_bat_right_limit ?
> +                           new_bat_right_limit : s->free_data_block_offset;
> +            bdrv_pwrite_sync(bs->file, new_block_offset, buf,
> +                             s->block_size + s->bitmap_size);
> +            if (ret < 0) {
> +                goto out;
> +            }
> +
> +            for (index = 0; index < s->max_table_entries; index++) {
> +                if (s->pagetable[index] == block_offset / BDRV_SECTOR_SIZE) {
> +                    s->pagetable[index] = block_offset;

The if condition checks for block_offset / BDRV_SECTOR_SIZE, but the
assignment stores block_offset (without converting it to sectors).

Did you mean:

  if (s->pagetable[index] == block_offset / BDRV_SECTOR_SIZE) {
      s->pagetable[index] = new_block_offset / BDRV_SECTOR_SIZE;

> +                    bat_offset = s->bat_offset + (4 * index);
> +                    bat_value = be32_to_cpu(new_block_offset /
> +                                            BDRV_SECTOR_SIZE);

cpu_to_be32()

> +                    ret = bdrv_pwrite_sync(bs->file, bat_offset, &bat_value, 
> 4);
> +                    if (ret < 0) {
> +                        goto out;
> +                    }
> +                    break;
> +                }
> +            }
> +
> +            s->free_data_block_offset = new_block_offset + s->block_size +
> +                                        s->bitmap_size;
> +            g_free(buf);
> +            buf = NULL;
> +        }
> +
> +        buf = g_malloc(512);
> +        memset(buf, 0xFF, 512);
> +
> +        /* Extend the BAT */
> +        offset = s->bat_offset + old_bat_size;
> +        for (index = 0;
> +                index < (new_bat_size - old_bat_size) / 512;
> +                index++) {
> +            ret = bdrv_pwrite_sync(bs->file, offset, buf, 512);

You could just bdrv_pwrite() and bdrv_flush() after the loop to make
this faster.

> +            if (ret < 0) {
> +                goto out;
> +            }
> +            offset += 512;
> +        }
> +
> +        g_free(buf);
> +        buf = g_malloc(1024);
> +
> +        /* Update the Dynamic Disk Header */
> +        ret = bdrv_pread(bs->file, 512, buf,
> +                         1024);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +
> +        dyndisk_header = (VHDDynDiskHeader *) buf;
> +        dyndisk_header->max_table_entries = be32_to_cpu(new_num_bat_entries);

cpu_to_be32()

> +        dyndisk_header->checksum = 0;
> +        dyndisk_header->checksum = be32_to_cpu(vpc_checksum(buf, 1024));

cpu_to_be32()

> +        ret = bdrv_pwrite_sync(bs->file, 512, buf, 1024);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +
> +    } else {
> +        s->free_data_block_offset = new_total_sectors * BDRV_SECTOR_SIZE;

Don't we have to take into account the header for VHD_FIXED?

> +    }
> +
> +    footer->cyls = be16_to_cpu(cyls);

cpu_to_be16()

> +    footer->heads = heads;
> +    footer->secs_per_cyl = secs_per_cyl;
> +    footer->size = be64_to_cpu(new_total_sectors * BDRV_SECTOR_SIZE);

cpu_to_be64()

> +    footer->checksum = 0;
> +    footer->checksum = be32_to_cpu(vpc_checksum(s->footer_buf, HEADER_SIZE));

cpu_to_be32()

> +
> +    /*
> +     *  Rewrite the footer, copying to the image header in case of a
> +     *  dynamic vhd.
> +     */
> +    rewrite_footer(bs, (cpu_to_be32(footer->type) != VHD_FIXED));

be32_to_cpu()

pgpPjo0x_ck26.pgp
Description: PGP signature