[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/7] qcow2: split out reading normal clusters fr
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 3/7] qcow2: split out reading normal clusters from qcow2_co_preadv |
Date: |
Mon, 1 Oct 2018 17:39:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
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 <address@hidden>
>>> ---
>>> 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