On Tue, Oct 01, 2013 at 03:29:17PM +0200, Stefan Hajnoczi wrote:
> On Wed, Sep 25, 2013 at 05:02:56PM -0400, Jeff Cody wrote:
> > @@ -1070,7 +1070,43 @@ exit:
> > return ret;
> > }
> >
> > +/*
> > + * Allocate a new payload block at the end of the file.
> > + *
> > + * Allocation will happen at 1MB alignment inside the file
> > + *
> > + * Returns the file offset start of the new payload block
> > + */
> > +static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
> > + uint64_t *new_offset)
> > +{
> > + *new_offset = bdrv_getlength(bs->file);
> > +
> > + /* per the spec, the address for a block is in units of 1MB */
> > + *new_offset = ROUND_UP(*new_offset, 1024*1024);
> > +
> > + return bdrv_truncate(bs->file, *new_offset + s->block_size);
> > +}
> > +
> > +/*
> > + * Update the BAT tablet entry with the new file offset, and the new entry
>
> s/tablet/table/
>
> > + * state */
> > +static void vhdx_update_bat_table_entry(BlockDriverState *bs,
> > BDRVVHDXState *s,
> > + VHDXSectorInfo *sinfo,
> > + uint64_t *bat_entry,
> > + uint64_t *bat_offset, int state)
> > +{
> > + /* The BAT entry is a uint64, with 44 bits for the file offset in
> > units of
> > + * 1MB, and 3 bits for the block state. */
> > + s->bat[sinfo->bat_idx] = ((sinfo->file_offset>>20) <<
>
> QEMU coding style requires space around the '>>'. Please run
> scripts/checkpatch.pl.
I missed that - but I did run checkpatch.pl, it did not catch it:
vhdx-v6/0011-block-vhdx-write-support.patch has no obvious style problems and
is ready for submission.
total: 0 errors, 0 warnings, 31 lines checked
I think the parenthesis hid it.
>
> > + VHDX_BAT_FILE_OFF_BITS);
> >
> > + s->bat[sinfo->bat_idx] |= state & VHDX_BAT_STATE_BIT_MASK;
> > +
> > + *bat_entry = cpu_to_le64(s->bat[sinfo->bat_idx]);
>
> Call this argument bat_entry_le so callers know now to manipulate the
> little-endian value directly?
>
Yes, good idea.
> > + *bat_offset = s->bat_offset + sinfo->bat_idx * sizeof(VHDXBatEntry);
> > +
> > +}
> >
> > /* Per the spec, on the first write of guest-visible data to the file the
> > * data write guid must be updated in the header */
> > @@ -1087,7 +1123,117 @@ int vhdx_user_visible_write(BlockDriverState *bs,
> > BDRVVHDXState *s)
> > static coroutine_fn int vhdx_co_writev(BlockDriverState *bs, int64_t
> > sector_num,
> > int nb_sectors, QEMUIOVector *qiov)
> > {
> > - return -ENOTSUP;
> > + int ret = -ENOTSUP;
> > + BDRVVHDXState *s = bs->opaque;
> > + VHDXSectorInfo sinfo;
> > + uint64_t bytes_done = 0;
> > + uint64_t bat_entry = 0;
> > + uint64_t bat_entry_offset = 0;
> > + bool bat_update;
> > + QEMUIOVector hd_qiov;
> > +
> > + qemu_iovec_init(&hd_qiov, qiov->niov);
> > +
> > + qemu_co_mutex_lock(&s->lock);
> > +
> > + ret = vhdx_user_visible_write(bs, s);
> > + if (ret < 0) {
> > + goto exit;
> > + }
> > +
> > + while (nb_sectors > 0) {
> > + if (s->params.data_bits & VHDX_PARAMS_HAS_PARENT) {
> > + /* not supported yet */
> > + ret = -ENOTSUP;
> > + goto exit;
> > + } else {
> > + bat_update = false;
> > + vhdx_block_translate(s, sector_num, nb_sectors, &sinfo);
> > +
> > + qemu_iovec_reset(&hd_qiov);
> > + qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
> > sinfo.bytes_avail);
> > + /* check the payload block state */
> > + switch (s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) {
> > + case PAYLOAD_BLOCK_ZERO:
> > + /* in this case, we need to preserve zero writes for
> > + * data that is not part of this write, so we must pad
> > + * the rest of the buffer to zeroes */
> > +
> > + /* if we are on a posix system with ftruncate() that
> > extends
> > + * a file, then it is zero-filled for us. On Win32, the
> > raw
> > + * layer uses SetFilePointer and SetFileEnd, which does not
> > + * zero fill AFAIK */
> > +
> > + /* TODO: queue another write of zero buffers if the host
> > OS does
> > + * not zero-fill on file extension */
>
> This is never addressed in the series.
>
I will address it in v7
> > +
> > + /* fall through */
> > + case PAYLOAD_BLOCK_NOT_PRESENT: /* fall through */
> > + case PAYLOAD_BLOCK_UNMAPPED: /* fall through */
> > + case PAYLOAD_BLOCK_UNDEFINED: /* fall through */
> > + ret = vhdx_allocate_block(bs, s, &sinfo.file_offset);
> > + if (ret < 0) {
> > + goto exit;
> > + }
> > + /* once we support differencing files, this may also be
> > + * partially present */
> > + /* update block state to the newly specified state */
> > + vhdx_update_bat_table_entry(bs, s, &sinfo, &bat_entry,
> > + &bat_entry_offset,
> > + PAYLOAD_BLOCK_FULL_PRESENT);
>
> In the specification the constant is called PAYLOAD_BLOCK_FULLY_PRESENT.
> Is s/FULLY/FULL/ intentional and can you match the spec for consistency?
>
That was unintentional, and yes I will change it to match the spec
naming.
> > + bat_update = true;
> > + /* since we just allocated a block, file_offset is the
> > + * beginning of the payload block. It needs to be the
> > + * write address, which includes the offset into the block
> > */
> > + sinfo.file_offset += sinfo.block_offset;
> > + /* fall through */
> > + case PAYLOAD_BLOCK_FULL_PRESENT:
> > + /* if the file offset address is in the header zone,
> > + * there is a problem */
> > + if (sinfo.file_offset < (1024*1024)) {
> > + ret = -EFAULT;
> > + goto exit;
> > + }
> > + /* block exists, so we can just overwrite it */
> > + qemu_co_mutex_unlock(&s->lock);
> > + ret = bdrv_co_writev(bs->file,
> > + sinfo.file_offset>>BDRV_SECTOR_BITS,
> > + sinfo.sectors_avail, &hd_qiov);
> > + qemu_co_mutex_lock(&s->lock);
> > + if (ret < 0) {
> > + goto exit;
> > + }
> > + break;
> > + case PAYLOAD_BLOCK_PARTIALLY_PRESENT:
> > + /* we don't yet support difference files, fall through
> > + * to error */
> > + default:
> > + ret = -EIO;
> > + goto exit;
> > + break;
> > + }
> > +
> > + if (bat_update) {
>
> Data must be stable on disk before we can expose the updated BAT entry.
> Missing bdrv_co_flush().
>
Good point, I will add that in for v7.
Thanks,
Jeff
