1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/3] qcow2: Fix preallocated truncation
  4. View patch

[Qemu-devel] [PATCH 2/3] qcow2: Always execute preallocate() in a coroutine

Max Reitz posted 3 patches 8 years, 4 months ago
[Qemu-devel] [PATCH 2/3] qcow2: Always execute preallocate() in a coroutine
Posted by Max Reitz 8 years, 4 months ago 
Some qcow2 functions (at least perform_cow()) expect s->lock to be
taken.  Therefore, if we want to make use of them, we should execute
preallocate() (as "preallocate_co") in a coroutine so that we can use
the qemu_co_mutex_* functions.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c3b312cdef..cc75a5167f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
 }
 
 
+typedef struct PreallocCo {
+    BlockDriverState *bs;
+    uint64_t offset;
+    uint64_t new_length;
+
+    int ret;
+} PreallocCo;
+
 /**
  * Preallocates metadata structures for data clusters between @offset (in the
  * guest disk) and @new_length (which is thus generally the new guest disk
@@ -2467,9 +2475,12 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
  *
  * Returns: 0 on success, -errno on failure.
  */
-static int preallocate(BlockDriverState *bs,
-                       uint64_t offset, uint64_t new_length)
+static void coroutine_fn preallocate_co(void *opaque)
 {
+    struct PreallocCo *params = opaque;
+    BlockDriverState *bs = params->bs;
+    uint64_t offset = params->offset;
+    uint64_t new_length = params->new_length;
     BDRVQcow2State *s = bs->opaque;
     uint64_t bytes;
     uint64_t host_offset = 0;
@@ -2477,9 +2488,7 @@ static int preallocate(BlockDriverState *bs,
     int ret;
     QCowL2Meta *meta;
 
-    if (qemu_in_coroutine()) {
-        qemu_co_mutex_lock(&s->lock);
-    }
+    qemu_co_mutex_lock(&s->lock);
 
     assert(offset <= new_length);
     bytes = new_length - offset;
@@ -2533,10 +2542,28 @@ static int preallocate(BlockDriverState *bs,
     ret = 0;
 
 done:
+    qemu_co_mutex_unlock(&s->lock);
+    params->ret = ret;
+}
+
+static int preallocate(BlockDriverState *bs,
+                       uint64_t offset, uint64_t new_length)
+{
+    PreallocCo params = {
+        .bs         = bs,
+        .offset     = offset,
+        .new_length = new_length,
+        .ret        = -EINPROGRESS,
+    };
+
     if (qemu_in_coroutine()) {
-        qemu_co_mutex_unlock(&s->lock);
+        preallocate_co(&params);
+    } else {
+        Coroutine *co = qemu_coroutine_create(preallocate_co, &params);
+        bdrv_coroutine_enter(bs, co);
+        BDRV_POLL_WHILE(bs, params.ret == -EINPROGRESS);
     }
-    return ret;
+    return params.ret;
 }
 
 /* qcow2_refcount_metadata_size:
-- 
2.13.6
Re: [Qemu-devel] [PATCH 2/3] qcow2: Always execute preallocate() in a coroutine
Posted by Max Reitz 8 years, 4 months ago 
On 2017-10-09 23:55, Max Reitz wrote:
> Some qcow2 functions (at least perform_cow()) expect s->lock to be
> taken.  Therefore, if we want to make use of them, we should execute
> preallocate() (as "preallocate_co") in a coroutine so that we can use
> the qemu_co_mutex_* functions.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)

Cc: qemu-stable@nongnu.org
Re: [Qemu-devel] [PATCH 2/3] qcow2: Always execute preallocate() in a coroutine
Posted by Eric Blake 8 years, 4 months ago 
On 10/09/2017 04:55 PM, Max Reitz wrote:
> Some qcow2 functions (at least perform_cow()) expect s->lock to be
> taken.  Therefore, if we want to make use of them, we should execute
> preallocate() (as "preallocate_co") in a coroutine so that we can use
> the qemu_co_mutex_* functions.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 

> +++ b/block/qcow2.c
> @@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
>  }
>  
>  
> +typedef struct PreallocCo {
> +    BlockDriverState *bs;
> +    uint64_t offset;
> +    uint64_t new_length;
> +
> +    int ret;
> +} PreallocCo;

Because you create a typedef here...


> +static void coroutine_fn preallocate_co(void *opaque)
>  {
> +    struct PreallocCo *params = opaque;

you could drop 'struct' here, the way you already dropped it...

> +static int preallocate(BlockDriverState *bs,
> +                       uint64_t offset, uint64_t new_length)
> +{
> +    PreallocCo params = {
> +        .bs         = bs,
> +        .offset     = offset,
> +        .new_length = new_length,
> +        .ret        = -EINPROGRESS,
> +    };

...here.  But that doesn't change semantics, so either way,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/3] qcow2: Always execute preallocate() in a coroutine
Posted by Max Reitz 8 years, 4 months ago 
On 2017-10-10 00:28, Eric Blake wrote:
> On 10/09/2017 04:55 PM, Max Reitz wrote:
>> Some qcow2 functions (at least perform_cow()) expect s->lock to be
>> taken.  Therefore, if we want to make use of them, we should execute
>> preallocate() (as "preallocate_co") in a coroutine so that we can use
>> the qemu_co_mutex_* functions.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 34 insertions(+), 7 deletions(-)
>>
> 
>> +++ b/block/qcow2.c
>> @@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
>>  }
>>  
>>  
>> +typedef struct PreallocCo {
>> +    BlockDriverState *bs;
>> +    uint64_t offset;
>> +    uint64_t new_length;
>> +
>> +    int ret;
>> +} PreallocCo;
> 
> Because you create a typedef here...
> 
> 
>> +static void coroutine_fn preallocate_co(void *opaque)
>>  {
>> +    struct PreallocCo *params = opaque;
> 
> you could drop 'struct' here, the way you already dropped it...

Ah, yes, that was from before I added the typedef...  Will fix.

Max

> 
>> +static int preallocate(BlockDriverState *bs,
>> +                       uint64_t offset, uint64_t new_length)
>> +{
>> +    PreallocCo params = {
>> +        .bs         = bs,
>> +        .offset     = offset,
>> +        .new_length = new_length,
>> +        .ret        = -EINPROGRESS,
>> +    };
> 
> ...here.  But that doesn't change semantics, so either way,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Re: [Qemu-devel] [Qemu-block] [PATCH 2/3] qcow2: Always execute preallocate() in a coroutine
Posted by Jeff Cody 8 years, 4 months ago 
On Mon, Oct 09, 2017 at 11:55:32PM +0200, Max Reitz wrote:
> Some qcow2 functions (at least perform_cow()) expect s->lock to be
> taken.  Therefore, if we want to make use of them, we should execute
> preallocate() (as "preallocate_co") in a coroutine so that we can use
> the qemu_co_mutex_* functions.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c3b312cdef..cc75a5167f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
>  }
>  
>  
> +typedef struct PreallocCo {
> +    BlockDriverState *bs;
> +    uint64_t offset;
> +    uint64_t new_length;
> +
> +    int ret;
> +} PreallocCo;
> +
>  /**
>   * Preallocates metadata structures for data clusters between @offset (in the
>   * guest disk) and @new_length (which is thus generally the new guest disk
> @@ -2467,9 +2475,12 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
>   *
>   * Returns: 0 on success, -errno on failure.
>   */
> -static int preallocate(BlockDriverState *bs,
> -                       uint64_t offset, uint64_t new_length)
> +static void coroutine_fn preallocate_co(void *opaque)
>  {
> +    struct PreallocCo *params = opaque;
> +    BlockDriverState *bs = params->bs;
> +    uint64_t offset = params->offset;
> +    uint64_t new_length = params->new_length;
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t bytes;
>      uint64_t host_offset = 0;
> @@ -2477,9 +2488,7 @@ static int preallocate(BlockDriverState *bs,
>      int ret;
>      QCowL2Meta *meta;
>  
> -    if (qemu_in_coroutine()) {
> -        qemu_co_mutex_lock(&s->lock);
> -    }
> +    qemu_co_mutex_lock(&s->lock);
>  
>      assert(offset <= new_length);
>      bytes = new_length - offset;
> @@ -2533,10 +2542,28 @@ static int preallocate(BlockDriverState *bs,
>      ret = 0;
>  
>  done:
> +    qemu_co_mutex_unlock(&s->lock);
> +    params->ret = ret;
> +}
> +
> +static int preallocate(BlockDriverState *bs,
> +                       uint64_t offset, uint64_t new_length)
> +{
> +    PreallocCo params = {
> +        .bs         = bs,
> +        .offset     = offset,
> +        .new_length = new_length,
> +        .ret        = -EINPROGRESS,
> +    };
> +
>      if (qemu_in_coroutine()) {
> -        qemu_co_mutex_unlock(&s->lock);
> +        preallocate_co(&params);
> +    } else {
> +        Coroutine *co = qemu_coroutine_create(preallocate_co, &params);
> +        bdrv_coroutine_enter(bs, co);
> +        BDRV_POLL_WHILE(bs, params.ret == -EINPROGRESS);
>      }
> -    return ret;
> +    return params.ret;
>  }
>  
>  /* qcow2_refcount_metadata_size:
> -- 
> 2.13.6
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.