Patchwork [RFC,14/16] qcow2: Execute run_dependent_requests() without lock

login
register
mail settings
Submitter Kevin Wolf
Date Sept. 18, 2012, 11:40 a.m.
Message ID <1347968442-8860-15-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/184685/
State New
Headers show

Comments

Kevin Wolf - Sept. 18, 2012, 11:40 a.m.
There's no reason for run_dependent_requests() to hold s->lock, and a
later patch will require that in fact the lock is not held.

Also, before this patch, run_dependent_requests() not only does what its
name suggests, but also removes the l2meta from the list of in-flight
requests. Change this, while we're touching it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)
Paolo Bonzini - Sept. 18, 2012, 2:33 p.m.
Il 18/09/2012 13:40, Kevin Wolf ha scritto:
>  static void run_dependent_requests(BDRVQcowState *s, QCowL2Meta *m)
>  {
> -    /* Take the request off the list of running requests */
> -    if (m->nb_clusters != 0) {
> -        QLIST_REMOVE(m, next_in_flight);
> -    }
> -
>      /* Restart all dependent requests */
>      if (!qemu_co_queue_empty(&m->dependent_requests)) {
> -        qemu_co_mutex_unlock(&s->lock);
>          qemu_co_queue_restart_all(&m->dependent_requests);
> -        qemu_co_mutex_lock(&s->lock);
>      }

The comment and if can go away.

Perhaps this patch could be moved earlier in the series?  (Just asking,
in case the rebase is not too painful).

Paolo

>  }
>  
> @@ -800,10 +793,18 @@ again:
>          goto again;
>      }
>  
> +    qemu_co_mutex_unlock(&s->lock);
> +
> +    /* Take the request off the list of running requests */
> +    if (m->nb_clusters != 0) {
> +        QLIST_REMOVE(m, next_in_flight);
> +    }
> +
> +    /* Meanwhile some new dependencies could have accumulated */
Kevin Wolf - Sept. 18, 2012, 2:54 p.m.
Am 18.09.2012 16:33, schrieb Paolo Bonzini:
> Il 18/09/2012 13:40, Kevin Wolf ha scritto:
>>  static void run_dependent_requests(BDRVQcowState *s, QCowL2Meta *m)
>>  {
>> -    /* Take the request off the list of running requests */
>> -    if (m->nb_clusters != 0) {
>> -        QLIST_REMOVE(m, next_in_flight);
>> -    }
>> -
>>      /* Restart all dependent requests */
>>      if (!qemu_co_queue_empty(&m->dependent_requests)) {
>> -        qemu_co_mutex_unlock(&s->lock);
>>          qemu_co_queue_restart_all(&m->dependent_requests);
>> -        qemu_co_mutex_lock(&s->lock);
>>      }
> 
> The comment and if can go away.

Ah, yes. And the one remaining line can be open coded in the two callers.

> Perhaps this patch could be moved earlier in the series?  (Just asking,
> in case the rebase is not too painful).

I'll check how painful it becomes. I'd expect it's doable.

Kevin

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index e001436..88a2020 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -739,16 +739,9 @@  fail:
 
 static void run_dependent_requests(BDRVQcowState *s, QCowL2Meta *m)
 {
-    /* Take the request off the list of running requests */
-    if (m->nb_clusters != 0) {
-        QLIST_REMOVE(m, next_in_flight);
-    }
-
     /* Restart all dependent requests */
     if (!qemu_co_queue_empty(&m->dependent_requests)) {
-        qemu_co_mutex_unlock(&s->lock);
         qemu_co_queue_restart_all(&m->dependent_requests);
-        qemu_co_mutex_lock(&s->lock);
     }
 }
 
@@ -800,10 +793,18 @@  again:
         goto again;
     }
 
+    qemu_co_mutex_unlock(&s->lock);
+
+    /* Take the request off the list of running requests */
+    if (m->nb_clusters != 0) {
+        QLIST_REMOVE(m, next_in_flight);
+    }
+
+    /* Meanwhile some new dependencies could have accumulated */
     run_dependent_requests(s, m);
+
     g_free(m);
 
-    qemu_co_mutex_unlock(&s->lock);
     qemu_co_rwlock_unlock(&s->l2meta_flush);
 }
 
@@ -961,13 +962,16 @@  static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
     ret = 0;
 
 fail:
+    qemu_co_mutex_unlock(&s->lock);
+
     if (l2meta != NULL) {
+        if (l2meta->nb_clusters != 0) {
+            QLIST_REMOVE(l2meta, next_in_flight);
+        }
         run_dependent_requests(s, l2meta);
         g_free(l2meta);
     }
 
-    qemu_co_mutex_unlock(&s->lock);
-
     qemu_iovec_destroy(&hd_qiov);
     qemu_vfree(cluster_data);
     trace_qcow2_writev_done_req(qemu_coroutine_self(), ret);
@@ -1259,7 +1263,7 @@  static int preallocate(BlockDriverState *bs)
         /* There are no dependent requests, but we need to remove our request
          * from the list of in-flight requests */
         if (meta != NULL) {
-            run_dependent_requests(bs->opaque, meta);
+            QLIST_REMOVE(meta, next_in_flight);
         }
 
         /* TODO Preallocate data if requested */