From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56784) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1askyp-0002AN-Se for qemu-devel@nongnu.org; Wed, 20 Apr 2016 01:47:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1askyo-000797-SC for qemu-devel@nongnu.org; Wed, 20 Apr 2016 01:47:15 -0400 Date: Wed, 20 Apr 2016 13:47:12 +0800 From: Fam Zheng Message-ID: <20160420054712.GA1404@ad-mail.usersys.redhat.com> References: <1461106788-14285-1-git-send-email-mreitz@redhat.com> <1461106788-14285-2-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1461106788-14285-2-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-2.6 1/2] block/mirror: Revive dead yielding code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Kevin Wolf , Jeff Cody 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 > --- > 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