From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aw4Gb-0004Dd-Jm for qemu-devel@nongnu.org; Fri, 29 Apr 2016 04:59:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aw4Ga-00039N-I7 for qemu-devel@nongnu.org; Fri, 29 Apr 2016 04:59:17 -0400 Date: Fri, 29 Apr 2016 09:41:28 +0200 From: Kevin Wolf Message-ID: <20160429074128.GA4350@noname.redhat.com> References: <1461849406-29743-1-git-send-email-kwolf@redhat.com> <1461849406-29743-14-git-send-email-kwolf@redhat.com> <20160429030824.GE1421@ad.usersys.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160429030824.GE1421@ad.usersys.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 13/17] vmdk: Implement .bdrv_co_pwritev() interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: qemu-block@nongnu.org, mreitz@redhat.com, stefanha@redhat.com, sw@weilnetz.de, qemu-devel@nongnu.org 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