From: "Denis V. Lunev" <den@openvz.org>
We are not working with a shared state data in the decruption code and
thus this operation is safe. On the other hand this significantly
reduces the scope of the lock.
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
block/qcow2.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..41def07c67 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1880,9 +1880,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
break;
case QCOW2_CLUSTER_NORMAL:
+ qemu_co_mutex_unlock(&s->lock);
+
if ((cluster_offset & 511) != 0) {
ret = -EIO;
- goto fail;
+ goto fail_nolock;
}
if (bs->encrypted) {
@@ -1899,7 +1901,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
* s->cluster_size);
if (cluster_data == NULL) {
ret = -ENOMEM;
- goto fail;
+ goto fail_nolock;
}
}
@@ -1909,13 +1911,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
}
BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
- qemu_co_mutex_unlock(&s->lock);
ret = bdrv_co_preadv(bs->file,
cluster_offset + offset_in_cluster,
cur_bytes, &hd_qiov, 0);
- qemu_co_mutex_lock(&s->lock);
if (ret < 0) {
- goto fail;
+ goto fail_nolock;
}
if (bs->encrypted) {
assert(s->crypto);
@@ -1929,10 +1929,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
cur_bytes,
NULL) < 0) {
ret = -EIO;
- goto fail;
+ goto fail_nolock;
}
qemu_iovec_from_buf(qiov, bytes_done, cluster_data, cur_bytes);
}
+ qemu_co_mutex_lock(&s->lock);
break;
default:
@@ -1950,6 +1951,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
fail:
qemu_co_mutex_unlock(&s->lock);
+fail_nolock:
qemu_iovec_destroy(&hd_qiov);
qemu_vfree(cluster_data);
--
2.11.1
On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
> From: "Denis V. Lunev" <den@openvz.org>
>
> We are not working with a shared state data in the decruption code and
> thus this operation is safe. On the other hand this significantly
> reduces the scope of the lock.
Sure, but does it have any effect? This is a coroutine lock and I don't
see any yield points besides the bdrv_co_preadv() that was executed
outside of the lock anyway.
Max
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
> block/qcow2.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ec9e6238a0..41def07c67 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1880,9 +1880,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
> break;
>
> case QCOW2_CLUSTER_NORMAL:
> + qemu_co_mutex_unlock(&s->lock);
> +
> if ((cluster_offset & 511) != 0) {
> ret = -EIO;
> - goto fail;
> + goto fail_nolock;
> }
>
> if (bs->encrypted) {
> @@ -1899,7 +1901,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
> * s->cluster_size);
> if (cluster_data == NULL) {
> ret = -ENOMEM;
> - goto fail;
> + goto fail_nolock;
> }
> }
>
> @@ -1909,13 +1911,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
> }
>
> BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
> - qemu_co_mutex_unlock(&s->lock);
> ret = bdrv_co_preadv(bs->file,
> cluster_offset + offset_in_cluster,
> cur_bytes, &hd_qiov, 0);
> - qemu_co_mutex_lock(&s->lock);
> if (ret < 0) {
> - goto fail;
> + goto fail_nolock;
> }
> if (bs->encrypted) {
> assert(s->crypto);
> @@ -1929,10 +1929,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
> cur_bytes,
> NULL) < 0) {
> ret = -EIO;
> - goto fail;
> + goto fail_nolock;
> }
> qemu_iovec_from_buf(qiov, bytes_done, cluster_data, cur_bytes);
> }
> + qemu_co_mutex_lock(&s->lock);
> break;
>
> default:
> @@ -1950,6 +1951,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
> fail:
> qemu_co_mutex_unlock(&s->lock);
>
> +fail_nolock:
> qemu_iovec_destroy(&hd_qiov);
> qemu_vfree(cluster_data);
>
>
On 27.09.18 18:58, Max Reitz wrote:
> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
>> From: "Denis V. Lunev" <den@openvz.org>
>>
>> We are not working with a shared state data in the decruption code and
(*decryption)
>> thus this operation is safe. On the other hand this significantly
>> reduces the scope of the lock.
>
> Sure, but does it have any effect? This is a coroutine lock and I don't
> see any yield points besides the bdrv_co_preadv() that was executed
> outside of the lock anyway.
I think it makes sense to move the qemu_co_mutex_lock() down below the
decryption, as the commit title suggests. Because then we can decrypt
while another coroutine that uses the lock is blocking.
But I don't see the point of moving the qemu_co_mutex_unlock() up.
(That is non-trivial movement because it means I'd have to find out
whether anything that could modify the cluster offset is serialized by
the generic block layer. Or, well, not really, because there is no
yield point, so it doesn't actually matter. But it doesn't give us any
benefit either, I think.)
Max
> Max
>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>> block/qcow2.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index ec9e6238a0..41def07c67 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1880,9 +1880,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>> break;
>>
>> case QCOW2_CLUSTER_NORMAL:
>> + qemu_co_mutex_unlock(&s->lock);
>> +
>> if ((cluster_offset & 511) != 0) {
>> ret = -EIO;
>> - goto fail;
>> + goto fail_nolock;
>> }
>>
>> if (bs->encrypted) {
>> @@ -1899,7 +1901,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>> * s->cluster_size);
>> if (cluster_data == NULL) {
>> ret = -ENOMEM;
>> - goto fail;
>> + goto fail_nolock;
>> }
>> }
>>
>> @@ -1909,13 +1911,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>> }
>>
>> BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
>> - qemu_co_mutex_unlock(&s->lock);
>> ret = bdrv_co_preadv(bs->file,
>> cluster_offset + offset_in_cluster,
>> cur_bytes, &hd_qiov, 0);
>> - qemu_co_mutex_lock(&s->lock);
>> if (ret < 0) {
>> - goto fail;
>> + goto fail_nolock;
>> }
>> if (bs->encrypted) {
>> assert(s->crypto);
>> @@ -1929,10 +1929,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>> cur_bytes,
>> NULL) < 0) {
>> ret = -EIO;
>> - goto fail;
>> + goto fail_nolock;
>> }
>> qemu_iovec_from_buf(qiov, bytes_done, cluster_data, cur_bytes);
>> }
>> + qemu_co_mutex_lock(&s->lock);
>> break;
>>
>> default:
>> @@ -1950,6 +1951,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>> fail:
>> qemu_co_mutex_unlock(&s->lock);
>>
>> +fail_nolock:
>> qemu_iovec_destroy(&hd_qiov);
>> qemu_vfree(cluster_data);
>>
>>
>
>
27.09.2018 20:02, Max Reitz wrote:
> On 27.09.18 18:58, Max Reitz wrote:
>> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
>>> From: "Denis V. Lunev" <den@openvz.org>
>>>
>>> We are not working with a shared state data in the decruption code and
> (*decryption)
>
>>> thus this operation is safe. On the other hand this significantly
>>> reduces the scope of the lock.
>> Sure, but does it have any effect? This is a coroutine lock and I don't
>> see any yield points besides the bdrv_co_preadv() that was executed
>> outside of the lock anyway.
> I think it makes sense to move the qemu_co_mutex_lock() down below the
> decryption, as the commit title suggests. Because then we can decrypt
> while another coroutine that uses the lock is blocking.
>
> But I don't see the point of moving the qemu_co_mutex_unlock() up.
> (That is non-trivial movement because it means I'd have to find out
> whether anything that could modify the cluster offset is serialized by
> the generic block layer. Or, well, not really, because there is no
> yield point, so it doesn't actually matter. But it doesn't give us any
> benefit either, I think.)
>
> Max
It don't make any performance benefit, but it allows to split part about
normal cluster reading to separate function.
Hmm, in future we can do decryption in threads (like compress threads)
and it should improve performance.
>
>> Max
>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> ---
>>> block/qcow2.c | 14 ++++++++------
>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index ec9e6238a0..41def07c67 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1880,9 +1880,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>> break;
>>>
>>> case QCOW2_CLUSTER_NORMAL:
>>> + qemu_co_mutex_unlock(&s->lock);
>>> +
>>> if ((cluster_offset & 511) != 0) {
>>> ret = -EIO;
>>> - goto fail;
>>> + goto fail_nolock;
>>> }
>>>
>>> if (bs->encrypted) {
>>> @@ -1899,7 +1901,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>> * s->cluster_size);
>>> if (cluster_data == NULL) {
>>> ret = -ENOMEM;
>>> - goto fail;
>>> + goto fail_nolock;
>>> }
>>> }
>>>
>>> @@ -1909,13 +1911,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>> }
>>>
>>> BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
>>> - qemu_co_mutex_unlock(&s->lock);
>>> ret = bdrv_co_preadv(bs->file,
>>> cluster_offset + offset_in_cluster,
>>> cur_bytes, &hd_qiov, 0);
>>> - qemu_co_mutex_lock(&s->lock);
>>> if (ret < 0) {
>>> - goto fail;
>>> + goto fail_nolock;
>>> }
>>> if (bs->encrypted) {
>>> assert(s->crypto);
>>> @@ -1929,10 +1929,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>> cur_bytes,
>>> NULL) < 0) {
>>> ret = -EIO;
>>> - goto fail;
>>> + goto fail_nolock;
>>> }
>>> qemu_iovec_from_buf(qiov, bytes_done, cluster_data, cur_bytes);
>>> }
>>> + qemu_co_mutex_lock(&s->lock);
>>> break;
>>>
>>> default:
>>> @@ -1950,6 +1951,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>> fail:
>>> qemu_co_mutex_unlock(&s->lock);
>>>
>>> +fail_nolock:
>>> qemu_iovec_destroy(&hd_qiov);
>>> qemu_vfree(cluster_data);
>>>
>>>
>>
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.