Re: [Qemu-block] [PATCH 1/7] qcow2: move qemu_co_mutex_lock below decryption procedure

[Max Reitz]

On 27.09.18 18:58, Max Reitz wrote:
> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
>> From: "Denis V. Lunev" <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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);
>>  
>>
> 
>

signature.asc
Description: OpenPGP digital signature