From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YutiY-0002vx-Uz for qemu-devel@nongnu.org; Tue, 19 May 2015 22:26:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yutfp-0003P0-33 for qemu-devel@nongnu.org; Tue, 19 May 2015 22:24:24 -0400 Date: Wed, 20 May 2015 10:23:06 +0800 From: Fam Zheng Message-ID: <20150520022306.GB6219@ad.nay.redhat.com> References: <1432036186-29903-1-git-send-email-famz@redhat.com> <1432036186-29903-14-git-send-email-famz@redhat.com> <555AEEA3.4080407@redhat.com> <20150519164816.GA5375@cpc-pc.redhat.com> <555AF925.6050809@redhat.com> <20150519183752.GA2986@cpc-pc.redhat.com> <555B1718.7010501@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <555B1718.7010501@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 13/13] block/mirror: Block "device IO" during mirror exit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-block@nongnu.org, Juan Quintela , "Michael S. Tsirkin" , qemu-devel@nongnu.org, mreitz@redhat.com, Stefan Hajnoczi , Amit Shah On Tue, 05/19 12:57, Paolo Bonzini wrote: > > > On 19/05/2015 20:37, Fam Zheng wrote: > > On Tue, 05/19 10:49, Paolo Bonzini wrote: > >> > >> > >> On 19/05/2015 18:48, Fam Zheng wrote: > >>>>> > >>>>> This is too late. As a rule, the blocker must be established before > >>>>> calling bdrv_drain, and removed on the next yield (in this case, before > >>>>> the assignment to last_pause_ns). > >>> I don't understand. If the blocker is removed before mirror_run returns, > >>> wouldn't more device IO already hit source image by the time mirror_exit runs? > >> > >> If you go to mirror_exit, you won't reach the assignment (so you have to > >> remove the blocker in mirror_exit too). > >> > >> But if you don't go to mirror_exit because cnt != 0, you must remove the > >> blocker before the next I/O. > >> > > > > OK, but I'm still not clear how is it too late in this patch? Although the > > blocker is set after bdrv_drain, we know there is no dirty data because cnt is > > 0, and we'll be holding a blocker when releasing the AioContext, no new IO is > > allowed. > > So you rely on the caller of mirror_run not calling aio_context_release > between bdrv_drain and block_job_defer_to_main_loop? That indeed should > work, but why not stick to a common pattern of blocking I/O before > bdrv_drain? That's how bdrv_drain is almost always used in the code, so > it's a safe thing to do and the preemption points are then documented > more clearly. > > I think it would be nice to have all bdrv_drain calls: > > - either preceded by a device I/O blocker > > - or preceded by a comment explaining why the call is there and why it > doesn't need the blocker > > This is not a NACK, but I would like to understand the disadvantages of > what I am suggesting here. That makes sense now, I was just trying to get your idea. Of course the patten as you suggested is more advanced! Fam