qemu-devel
[Top][All Lists]
Advanced

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

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


From: Lucian Petrut
Subject: Re: [Qemu-devel] [[PATCH v2] 1/1] vpc.c: Add VHD resize support
Date: Mon, 22 Sep 2014 13:24:16 +0000

Thanks for reviewing this patch! I’ll send a new one, fixing the issues you’ve pointed out, along with some tests.

Please see my inline comments bellow.

Regards,
Lucian Petrut​

 >From: Stefan Hajnoczi
 >Sent: ‎Monday‎, ‎September‎ ‎22‎, ‎2014 ‎3‎:‎17‎ ‎PM
 >To: Petrut Lucian
 >Cc: address@hidden, Lucian Petrut, address@hidden, address@hidden, address@hidden, Alessandro Pilotti
 >
 >
 >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.

You are right, a check should be added here. I’m not aware of parsers
that may have this behavior but better be on the safe side.

 >> +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()

My bad, I’ll fix the BE related issues.

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

This was intended, it may just add an extra entry. Also, the number
of entries is calculated in the same way in the existing code which
creates dynamic VHDs, so I wanted to keep consistency with that
as well.

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

Whops, my bad. You are right, I should just use !buf. At first, I did not free
this at the end of iteration so I forgot to change this as well.

 >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

Thanks for the link, I’ll add a test for this.

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

You are right, this would be much better.


 >> +            }
 >> +
 >> +            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;

You are right here as well. I’ve actually done it this way but forgot to amend
the commit, thanks for pointing this out!

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

Done.

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

Fixed VHDs, unlike the other VHD types, don’t have a copy of the footer in the
file header, nor do they have the “dynamic vhd header”.

 >> +    }
 >> +
 >> +    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()

reply via email to

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