Re: [Qemu-devel] [PATCH v2 13/17] vmdk: Implement .bdrv_co_pwritev() interface

[Kevin Wolf <kwolf@redhat.com>]

Am 29.04.2016 um 05:08 hat Fam Zheng geschrieben:
> On Thu, 04/28 15:16, Kevin Wolf wrote:
> > +typedef struct VmdkWriteCompressedCo {
> > +    BlockDriverState *bs;
> > +    int64_t sector_num;
> > +    const uint8_t *buf;
> > +    int nb_sectors;
> > +    int ret;
> > +} VmdkWriteCompressedCo;
> > +
> > +static void vmdk_co_write_compressed(void *opaque)
> > +{
> > +    VmdkWriteCompressedCo *co = opaque;
> > +    QEMUIOVector local_qiov;
> > +    uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE;
> > +    uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE;
> > +
> > +    struct iovec iov = (struct iovec) {
> > +        .iov_base   = (uint8_t*) co->buf,
> > +        .iov_len    = bytes,
> > +    };
> > +    qemu_iovec_init_external(&local_qiov, &iov, 1);
> > +
> > +    co->ret = vmdk_pwritev(co->bs, offset, bytes, &local_qiov, false, false);
> 
> Should it acquire s->lock?

Yes, probably, if only for consistency.

It didn't acquire the lock before, so it didn't seem appropriate to add
it in this patch. Not taking the lock is fine because this function is
only called by qemu-img, where there is no concurrency.

There was a proposal recently to allow compressed writes during a normal
VM run. If we want to enable this, the locking would be needed. (The
same is true for qcow2 and qcow2.)

> > +}
> > +
> >  static int vmdk_write_compressed(BlockDriverState *bs,
> >                                   int64_t sector_num,
> >                                   const uint8_t *buf,
> >                                   int nb_sectors)
> >  {
> >      BDRVVmdkState *s = bs->opaque;
> > +
> >      if (s->num_extents == 1 && s->extents[0].compressed) {
> > -        return vmdk_write(bs, sector_num, buf, nb_sectors, false, false);
> > +        Coroutine *co;
> > +        AioContext *aio_context = bdrv_get_aio_context(bs);
> > +        VmdkWriteCompressedCo data = {
> > +            .bs         = bs,
> > +            .sector_num = sector_num,
> > +            .buf        = buf,
> > +            .nb_sectors = nb_sectors,
> > +            .ret        = -EINPROGRESS,
> > +        };
> > +        co = qemu_coroutine_create(vmdk_co_write_compressed);
> > +        qemu_coroutine_enter(co, &data);
> > +        while (data.ret == -EINPROGRESS) {
> > +            aio_poll(aio_context, true);
> > +        }
> > +        return data.ret;
> 
> Don't you have a plan to make the creation of coroutine for compressed write in
> in block layer?  Or will bdrv_co_pwritev gain a "compressed" flag
> (BDRV_REQ_COMPRESSED) in the future?

Well, there was this recent discussion, but personally I don't have any
plans. If we want to do compressed writes from running VMs, we will need
the conversion because we must not block the vcpu.

Or maybe the flag would actually be the nicer option, yes. I don't have
a strong opinion on this yet.

Kevin