From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:46900)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1ZziZP-0003Rk-Fi
	for qemu-devel@nongnu.org; Fri, 20 Nov 2015 05:05:32 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <famz@redhat.com>) id 1ZziZO-0004rz-LA
	for qemu-devel@nongnu.org; Fri, 20 Nov 2015 05:05:31 -0500
Date: Fri, 20 Nov 2015 18:05:19 +0800
From: Fam Zheng <famz@redhat.com>
Message-ID: <20151120100519.GA23987@ad.usersys.redhat.com>
References: <1447760483-21094-1-git-send-email-famz@redhat.com>
	<1447760483-21094-2-git-send-email-famz@redhat.com>
	<564C550A.8010408@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <564C550A.8010408@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/2] mirror: Rewrite mirror_iteration
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Jeff Cody <jcody@redhat.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com

On Wed, 11/18 11:38, Paolo Bonzini wrote:
> 
> 
> On 17/11/2015 12:41, Fam Zheng wrote:
> > +        /* Wait for I/O to this cluster (from a previous iteration) to be
> > +         * done.*/
> > +        while (test_bit(next_chunk, s->in_flight_bitmap)) {
> > +            trace_mirror_yield_in_flight(s, next_sector, s->in_flight);
> > +            s->waiting_for_io = true;
> > +            qemu_coroutine_yield();
> > +            s->waiting_for_io = false;
> > +        }
> > +
> 
> I think this could just "break" if nb_chunks > 0, like
> 
> 	if (test_bit(next_chunk, s->in_flight_bitmap)) {
> 		if (nb_chunks > 0) {
> 			break;
> 		}
> 		mirror_wait_for_io(s);
> 		/* Now retry.  */
> 	} else {
> 		hbitmap_next = hbitmap_iter_next(&s->hbi);
> 	        assert(hbitmap_next == next_sector);
> 	        nb_chunks++;
> 	}
> 
> but it can be done later (the usage of mirror_wait_for_io is a hint :)).
> 
> There's a typo though:
> 
> > +    /* Clear dirty bits before querying the block status, because
> > +     * calling bdrv_reset_dirty_bitmap could yield - if some blocks are marked
> 
> That's bdrv_get_block_status_above.
> 
> > +     * dirty in this window, we need to know.
> > +     */
> 

Thanks. I'll adopt both suggestions and resend.

Fam