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 <[email protected]> >>> --- >>> 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
