[Qemu-devel] [PATCH for-2.6 0/2] block/mirror: Fix mirroring with guest I/O load

[Qemu-devel] [PATCH for-2.6 0/2] block/mirror: Fix mirroring with guest I/O load

[Max Reitz]

Bug report: https://bugs.launchpad.net/qemu/+bug/1570134

If you are doing a mirror operation (I just tested with block-commit on
the active layer, but I guess any mirroring will do) while the guest has
rather heavy I/O load (or light I/O also, you just need to be more
unlucky) will lead to the cache of mirror's bitmap iterator becoming
stale and not reflect all dirty bits which are set in the drive's dirty
bitmap.

Generally, this isn't bad because we just restart over once we are
through, and this will refresh the iterator's cache.

But it is bad for the code which tries to find a contiguous range of
dirty chunks. This code needs to clear the bits in the iterator, so it
invokes hbitmap_iter_next() for every contiguous dirty chunk found. But
then it has to make sure that this actually cleared that chunk's dirty
bit: And if the iterator's cache is stale, this may not be the case.
Then, we run into a failed assertion.

But detecting this discrepancy is easy and refreshing the iterator's
cache is too; and then, the assertion holds.

Besides this (patch 2), the code which is supposed to wait for
overlapping in-flight requests on the first chunk of a dirty range is
dead. I didn't produce any problems regarding that, but I'm sure it's
not good. Patch 1 fixes that.


Max Reitz (2):
  block/mirror: Revive dead yielding code
  block/mirror: Refresh stale bitmap iterator cache

 block/mirror.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

-- 
2.8.0

[Qemu-devel] [PATCH for-2.6 1/2] block/mirror: Revive dead yielding code

[Max Reitz]

mirror_iteration() is supposed to wait if the current chunk is subject
to a still in-flight mirroring operation. However, it mixed checking
this conflict situation with checking the dirty status of a chunk. A
simplification for the latter condition (the first chunk encountered is
always dirty) led to neglecting the former: We just skip the first chunk
and thus never test whether it conflicts with an in-flight operation.

To fix this, pull out the code which waits for in-flight operations on
the first chunk of the range to be mirrored to settle.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index c2cfc1a..2714a77 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -298,7 +298,7 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
 static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
 {
     BlockDriverState *source = s->common.bs;
-    int64_t sector_num;
+    int64_t sector_num, first_chunk;
     uint64_t delay_ns = 0;
     /* At least the first dirty chunk is mirrored in one iteration. */
     int nb_chunks = 1;
@@ -313,6 +313,12 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         assert(sector_num >= 0);
     }
 
+    first_chunk = sector_num / sectors_per_chunk;
+    while (test_bit(first_chunk, s->in_flight_bitmap)) {
+        trace_mirror_yield_in_flight(s, first_chunk, s->in_flight);
+        mirror_wait_for_io(s);
+    }
+
     /* Find the number of consective dirty chunks following the first dirty
      * one, and wait for in flight requests in them. */
     while (nb_chunks * sectors_per_chunk < (s->buf_size >> BDRV_SECTOR_BITS)) {
@@ -324,17 +330,12 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
             break;
         }
         if (test_bit(next_chunk, s->in_flight_bitmap)) {
-            if (nb_chunks > 0) {
-                break;
-            }
-            trace_mirror_yield_in_flight(s, next_sector, s->in_flight);
-            mirror_wait_for_io(s);
-            /* Now retry.  */
-        } else {
-            hbitmap_next = hbitmap_iter_next(&s->hbi);
-            assert(hbitmap_next == next_sector);
-            nb_chunks++;
+            break;
         }
+
+        hbitmap_next = hbitmap_iter_next(&s->hbi);
+        assert(hbitmap_next == next_sector);
+        nb_chunks++;
     }
 
     /* Clear dirty bits before querying the block status, because
-- 
2.8.0

[Qemu-devel] [PATCH for-2.6 2/2] block/mirror: Refresh stale bitmap iterator cache

[Max Reitz]

If the drive's dirty bitmap is dirtied while the mirror operation is
running, the cache of the iterator used by the mirror code may become
stale and not contain all dirty bits.

This only becomes an issue if we are looking for contiguously dirty
chunks on the drive. In that case, we can easily detect the discrepancy
and just refresh the iterator if one occurs.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 2714a77..9df1fae 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -334,6 +334,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         }
 
         hbitmap_next = hbitmap_iter_next(&s->hbi);
+        if (hbitmap_next > next_sector || hbitmap_next < 0) {
+            /* The bitmap iterator's cache is stale, refresh it */
+            bdrv_set_dirty_iter(&s->hbi, next_sector);
+            hbitmap_next = hbitmap_iter_next(&s->hbi);
+        }
         assert(hbitmap_next == next_sector);
         nb_chunks++;
     }
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.6 1/2] block/mirror: Revive dead yielding code

[Fam Zheng]

On Wed, 04/20 00:59, Max Reitz wrote:
> mirror_iteration() is supposed to wait if the current chunk is subject
> to a still in-flight mirroring operation. However, it mixed checking
> this conflict situation with checking the dirty status of a chunk. A
> simplification for the latter condition (the first chunk encountered is
> always dirty) led to neglecting the former: We just skip the first chunk
> and thus never test whether it conflicts with an in-flight operation.
> 
> To fix this, pull out the code which waits for in-flight operations on
> the first chunk of the range to be mirrored to settle.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index c2cfc1a..2714a77 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -298,7 +298,7 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
>  static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>  {
>      BlockDriverState *source = s->common.bs;
> -    int64_t sector_num;
> +    int64_t sector_num, first_chunk;
>      uint64_t delay_ns = 0;
>      /* At least the first dirty chunk is mirrored in one iteration. */
>      int nb_chunks = 1;
> @@ -313,6 +313,12 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          assert(sector_num >= 0);
>      }
>  
> +    first_chunk = sector_num / sectors_per_chunk;
> +    while (test_bit(first_chunk, s->in_flight_bitmap)) {
> +        trace_mirror_yield_in_flight(s, first_chunk, s->in_flight);
> +        mirror_wait_for_io(s);
> +    }
> +
>      /* Find the number of consective dirty chunks following the first dirty
>       * one, and wait for in flight requests in them. */
>      while (nb_chunks * sectors_per_chunk < (s->buf_size >> BDRV_SECTOR_BITS)) {
> @@ -324,17 +330,12 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>              break;
>          }
>          if (test_bit(next_chunk, s->in_flight_bitmap)) {

Now this can be combiled into the previous if block.

> -            if (nb_chunks > 0) {
> -                break;
> -            }
> -            trace_mirror_yield_in_flight(s, next_sector, s->in_flight);
> -            mirror_wait_for_io(s);
> -            /* Now retry.  */
> -        } else {
> -            hbitmap_next = hbitmap_iter_next(&s->hbi);
> -            assert(hbitmap_next == next_sector);
> -            nb_chunks++;
> +            break;
>          }
> +
> +        hbitmap_next = hbitmap_iter_next(&s->hbi);
> +        assert(hbitmap_next == next_sector);
> +        nb_chunks++;
>      }
>  
>      /* Clear dirty bits before querying the block status, because
> -- 
> 2.8.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.6 2/2] block/mirror: Refresh stale bitmap iterator cache

[Fam Zheng]

On Wed, 04/20 00:59, Max Reitz wrote:
> If the drive's dirty bitmap is dirtied while the mirror operation is
> running, the cache of the iterator used by the mirror code may become
> stale and not contain all dirty bits.
> 
> This only becomes an issue if we are looking for contiguously dirty
> chunks on the drive. In that case, we can easily detect the discrepancy
> and just refresh the iterator if one occurs.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 2714a77..9df1fae 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -334,6 +334,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          }
>  
>          hbitmap_next = hbitmap_iter_next(&s->hbi);
> +        if (hbitmap_next > next_sector || hbitmap_next < 0) {
> +            /* The bitmap iterator's cache is stale, refresh it */
> +            bdrv_set_dirty_iter(&s->hbi, next_sector);
> +            hbitmap_next = hbitmap_iter_next(&s->hbi);
> +        }
>          assert(hbitmap_next == next_sector);
>          nb_chunks++;
>      }
> -- 
> 2.8.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.6 2/2] block/mirror: Refresh stale bitmap iterator cache

[Kevin Wolf]

Am 20.04.2016 um 00:59 hat Max Reitz geschrieben:
> If the drive's dirty bitmap is dirtied while the mirror operation is
> running, the cache of the iterator used by the mirror code may become
> stale and not contain all dirty bits.
> 
> This only becomes an issue if we are looking for contiguously dirty
> chunks on the drive. In that case, we can easily detect the discrepancy
> and just refresh the iterator if one occurs.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 2714a77..9df1fae 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -334,6 +334,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          }
>  
>          hbitmap_next = hbitmap_iter_next(&s->hbi);
> +        if (hbitmap_next > next_sector || hbitmap_next < 0) {
> +            /* The bitmap iterator's cache is stale, refresh it */
> +            bdrv_set_dirty_iter(&s->hbi, next_sector);
> +            hbitmap_next = hbitmap_iter_next(&s->hbi);
> +        }
>          assert(hbitmap_next == next_sector);

The iterator doesn't seem to be used afterwards anyway, so why not just
use next_sector and stop using the iterator when we already know what
result we want to get?

Kevin

Re: [Qemu-devel] [PATCH for-2.6 1/2] block/mirror: Revive dead yielding code

[Kevin Wolf]

Am 20.04.2016 um 00:59 hat Max Reitz geschrieben:
> mirror_iteration() is supposed to wait if the current chunk is subject
> to a still in-flight mirroring operation. However, it mixed checking
> this conflict situation with checking the dirty status of a chunk. A
> simplification for the latter condition (the first chunk encountered is
> always dirty) led to neglecting the former: We just skip the first chunk
> and thus never test whether it conflicts with an in-flight operation.
> 
> To fix this, pull out the code which waits for in-flight operations on
> the first chunk of the range to be mirrored to settle.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 2/2] block/mirror: Refresh stale bitmap iterator cache

[Kevin Wolf]

Am 20.04.2016 um 14:13 hat Kevin Wolf geschrieben:
> Am 20.04.2016 um 00:59 hat Max Reitz geschrieben:
> > If the drive's dirty bitmap is dirtied while the mirror operation is
> > running, the cache of the iterator used by the mirror code may become
> > stale and not contain all dirty bits.
> > 
> > This only becomes an issue if we are looking for contiguously dirty
> > chunks on the drive. In that case, we can easily detect the discrepancy
> > and just refresh the iterator if one occurs.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/mirror.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 2714a77..9df1fae 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -334,6 +334,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> >          }
> >  
> >          hbitmap_next = hbitmap_iter_next(&s->hbi);
> > +        if (hbitmap_next > next_sector || hbitmap_next < 0) {
> > +            /* The bitmap iterator's cache is stale, refresh it */
> > +            bdrv_set_dirty_iter(&s->hbi, next_sector);
> > +            hbitmap_next = hbitmap_iter_next(&s->hbi);
> > +        }
> >          assert(hbitmap_next == next_sector);
> 
> The iterator doesn't seem to be used afterwards anyway, so why not just
> use next_sector and stop using the iterator when we already know what
> result we want to get?

And of course I read that code completely wrong because in reality
&s->hbi isn't local. Seems to be the right way to do things then.

Kevin

Re: [Qemu-devel] [PATCH for-2.6 0/2] block/mirror: Fix mirroring with guest I/O load

[Kevin Wolf]

Am 20.04.2016 um 00:59 hat Max Reitz geschrieben:
> Bug report: https://bugs.launchpad.net/qemu/+bug/1570134
> 
> If you are doing a mirror operation (I just tested with block-commit on
> the active layer, but I guess any mirroring will do) while the guest has
> rather heavy I/O load (or light I/O also, you just need to be more
> unlucky) will lead to the cache of mirror's bitmap iterator becoming
> stale and not reflect all dirty bits which are set in the drive's dirty
> bitmap.
> 
> Generally, this isn't bad because we just restart over once we are
> through, and this will refresh the iterator's cache.
> 
> But it is bad for the code which tries to find a contiguous range of
> dirty chunks. This code needs to clear the bits in the iterator, so it
> invokes hbitmap_iter_next() for every contiguous dirty chunk found. But
> then it has to make sure that this actually cleared that chunk's dirty
> bit: And if the iterator's cache is stale, this may not be the case.
> Then, we run into a failed assertion.
> 
> But detecting this discrepancy is easy and refreshing the iterator's
> cache is too; and then, the assertion holds.
> 
> Besides this (patch 2), the code which is supposed to wait for
> overlapping in-flight requests on the first chunk of a dirty range is
> dead. I didn't produce any problems regarding that, but I'm sure it's
> not good. Patch 1 fixes that.

Thanks, applied to the block branch.

Kevin