Patchwork [RFC,V6,25/33] qcow2: Serialize write requests when deduplication is activated.

login
register
mail settings
Submitter Benoît Canet
Date Feb. 6, 2013, 12:31 p.m.
Message ID <1360153926-9492-26-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/218651/
State New
Headers show

Comments

Benoît Canet - Feb. 6, 2013, 12:31 p.m.
This fix the sub cluster sized writes race conditions while waiting
for a more faster solution.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block/qcow2.c |   14 +++++++++++++-
 block/qcow2.h |    1 +
 2 files changed, 14 insertions(+), 1 deletion(-)
Eric Blake - Feb. 7, 2013, 6:02 p.m.
On 02/06/2013 05:31 AM, Benoît Canet wrote:
> This fix the sub cluster sized writes race conditions while waiting

s/fix/fixes/

> for a more faster solution.

s/more//

Would it be worth describing the race condition in more detail?
Stefan Hajnoczi - Feb. 8, 2013, 10:51 a.m.
On Wed, Feb 06, 2013 at 01:31:58PM +0100, Benoît Canet wrote:
> This fix the sub cluster sized writes race conditions while waiting
> for a more faster solution.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block/qcow2.c |   14 +++++++++++++-
>  block/qcow2.h |    1 +
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f39b6d5..5cfffd2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -524,6 +524,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
>  
>      /* Initialise locks */
>      qemu_co_mutex_init(&s->lock);
> +    qemu_co_mutex_init(&s->dedup_lock);
>  
>      /* Repair image if dirty */
>      if (!(flags & BDRV_O_CHECK) && !bs->read_only &&
> @@ -815,8 +816,15 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>      s->cluster_cache_offset = -1; /* disable compressed cache */
>  
>      qemu_co_mutex_lock(&s->lock);
> -
>      atomic_dedup_is_running = qcow2_dedup_is_running(bs);
> +    qemu_co_mutex_unlock(&s->lock);

No need to lock/unlock around qcow2_dedup_is_running(bs) because it
doesn't yield.  No other coroutine can run while our thread of control
is active here.

> +
> +    if (atomic_dedup_is_running) {
> +        qemu_co_mutex_lock(&s->dedup_lock);
> +    }

What exactly does dedup_lock protect?  Please document it.

> +
> +    qemu_co_mutex_lock(&s->lock);
> +
>      if (atomic_dedup_is_running) {
>          QTAILQ_INIT(&ds.undedupables);
>          ds.phash.reuse = false;
> @@ -983,6 +991,10 @@ fail:
>          g_free(l2meta);
>      }
>  
> +    if (atomic_dedup_is_running) {
> +        qemu_co_mutex_unlock(&s->dedup_lock);
> +    }
> +
>      qemu_iovec_destroy(&hd_qiov);
>      qemu_vfree(cluster_data);
>      qemu_vfree(dedup_cluster_data);
> diff --git a/block/qcow2.h b/block/qcow2.h
> index a13ec75..81dc0ea 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -236,6 +236,7 @@ typedef struct BDRVQcowState {
>      GTree *dedup_tree_by_sect;
>  
>      CoMutex lock;
> +    CoMutex dedup_lock;
>  
>      uint32_t crypt_method; /* current crypt method, 0 if no key yet */
>      uint32_t crypt_method_header;
> -- 
> 1.7.10.4
>

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index f39b6d5..5cfffd2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -524,6 +524,7 @@  static int qcow2_open(BlockDriverState *bs, int flags)
 
     /* Initialise locks */
     qemu_co_mutex_init(&s->lock);
+    qemu_co_mutex_init(&s->dedup_lock);
 
     /* Repair image if dirty */
     if (!(flags & BDRV_O_CHECK) && !bs->read_only &&
@@ -815,8 +816,15 @@  static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
     s->cluster_cache_offset = -1; /* disable compressed cache */
 
     qemu_co_mutex_lock(&s->lock);
-
     atomic_dedup_is_running = qcow2_dedup_is_running(bs);
+    qemu_co_mutex_unlock(&s->lock);
+
+    if (atomic_dedup_is_running) {
+        qemu_co_mutex_lock(&s->dedup_lock);
+    }
+
+    qemu_co_mutex_lock(&s->lock);
+
     if (atomic_dedup_is_running) {
         QTAILQ_INIT(&ds.undedupables);
         ds.phash.reuse = false;
@@ -983,6 +991,10 @@  fail:
         g_free(l2meta);
     }
 
+    if (atomic_dedup_is_running) {
+        qemu_co_mutex_unlock(&s->dedup_lock);
+    }
+
     qemu_iovec_destroy(&hd_qiov);
     qemu_vfree(cluster_data);
     qemu_vfree(dedup_cluster_data);
diff --git a/block/qcow2.h b/block/qcow2.h
index a13ec75..81dc0ea 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -236,6 +236,7 @@  typedef struct BDRVQcowState {
     GTree *dedup_tree_by_sect;
 
     CoMutex lock;
+    CoMutex dedup_lock;
 
     uint32_t crypt_method; /* current crypt method, 0 if no key yet */
     uint32_t crypt_method_header;