On 01.10.18 17:14, Vladimir Sementsov-Ogievskiy wrote: > 27.09.2018 20:35, Max Reitz wrote: >> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote: >>> Memory allocation may become less efficient for encrypted case. It's a >>> payment for further asynchronous scheme. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>> --- >>> block/qcow2.c | 114 >>> ++++++++++++++++++++++++++++++++-------------------------- >>> 1 file changed, 64 insertions(+), 50 deletions(-) >>> >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 65e3ca00e2..5e7f2ee318 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -1808,6 +1808,67 @@ out: >>> return ret; >>> } >>> >>> +static coroutine_fn int qcow2_co_preadv_normal(BlockDriverState *bs, >>> + uint64_t >>> file_cluster_offset, >>> + uint64_t offset, >>> + uint64_t bytes, >>> + QEMUIOVector *qiov, >>> + uint64_t qiov_offset) >>> +{ >>> + int ret; >>> + BDRVQcow2State *s = bs->opaque; >>> + void *crypt_buf = NULL; >>> + QEMUIOVector hd_qiov; >>> + int offset_in_cluster = offset_into_cluster(s, offset); >>> + >>> + if ((file_cluster_offset & 511) != 0) { >>> + return -EIO; >>> + } >>> + >>> + qemu_iovec_init(&hd_qiov, qiov->niov); >> So you're not just re-allocating the bounce buffer for every single >> call, but also this I/O vector. Hm. >> >>> + if (bs->encrypted) { >>> + assert(s->crypto); >>> + assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size); >>> + >>> + crypt_buf = qemu_try_blockalign(bs->file->bs, bytes); >> 1. Why did you remove the comment? >> >> 2. The check for whether crypt_buf was successfully allocated is missing. >> >> 3. Do you have any benchmarks for encrypted images? Having to allocate >> the bounce buffer for potentially every single cluster sounds rather bad >> to me. > > Hmm, no excuses. 1- Will fix, 2 - will fix, 3 - will fix or at least > test the performance. > >>> + qemu_iovec_add(&hd_qiov, crypt_buf, bytes); >>> + } else { >>> + qemu_iovec_concat(&hd_qiov, qiov, qiov_offset, bytes); >> qcow2_co_preadv() continues to do this as well. That's superfluous now. > > hd_qiov is local here.. or what do you mean?
qcow2_co_preadv() continues to have its own hd_qiov and appends qiov to it (just like here), but it's unused for normal clusters. So it doesn't have to do that for normal clusters. >>> + } >>> + >>> + BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO); >>> + ret = bdrv_co_preadv(bs->file, >>> + file_cluster_offset + offset_in_cluster, >>> + bytes, &hd_qiov, 0); >>> + if (ret < 0) { >>> + goto out; >>> + } >>> + >>> + if (bs->encrypted) { >>> + assert(s->crypto); >>> + assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0); >>> + assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0); >>> + if (qcrypto_block_decrypt(s->crypto, >>> + (s->crypt_physical_offset ? >>> + file_cluster_offset + offset_in_cluster >>> : >>> + offset), >>> + crypt_buf, >>> + bytes, >>> + NULL) < 0) { >> What's the reason against decrypting this in-place in qiov so we could >> save the bounce buffer? We allow offsets in clusters, so why can't we >> just call this function once per involved I/O vector entry? > > Isn't it unsafe to do decryption in guest buffers? I don't know. Why would it be? Because... >> Max >> >>> + ret = -EIO; >>> + goto out; >>> + } >>> + qemu_iovec_from_buf(qiov, qiov_offset, crypt_buf, bytes); ...we're putting the decrypted content there anyway. The only issue I see is that the guest may see encrypted content for a short duration. Hm. Maybe we don't want that. In which case it probably has to stay as it is. Max
signature.asc
Description: OpenPGP digital signature