From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36529) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCee4-0003tF-Ed for qemu-devel@nongnu.org; Tue, 07 Jul 2015 21:59:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCedz-0002g2-Vx for qemu-devel@nongnu.org; Tue, 07 Jul 2015 21:59:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42974) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCedz-0002fH-OG for qemu-devel@nongnu.org; Tue, 07 Jul 2015 21:59:27 -0400 Date: Wed, 8 Jul 2015 09:59:24 +0800 From: Fam Zheng Message-ID: <20150708015924.GF10382@ad.nay.redhat.com> References: <1436192669-10062-1-git-send-email-stefanha@redhat.com> <1436192669-10062-6-git-send-email-stefanha@redhat.com> <20150707073245.GD9515@ad.nay.redhat.com> <20150707125949.GA28673@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150707125949.GA28673@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 05/10] block: add block job transactions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, John Snow On Tue, 07/07 13:59, Stefan Hajnoczi wrote: > On Tue, Jul 07, 2015 at 03:32:45PM +0800, Fam Zheng wrote: > > On Mon, 07/06 15:24, Stefan Hajnoczi wrote: > > > +/** > > > + * block_job_txn_add_job: > > > + * @txn: The transaction (may be NULL) > > > + * @job: Job to add to the transaction > > > + * > > > + * Add @job to the transaction. The @job must not already be in a transaction. > > > + * The block job driver must call block_job_txn_prepare_to_complete() before > > > > s/block_job_txn_prepare_to_complete/block_job_txn_job_done/ > > > > Reading this for a second time I start to feel it too complicated for the good. > > > > I have another idea: in block_job_completed, check if other jobs have failed, > > and call this job driver's (imaginary) "abort()" callback accordingly; if all > > jobs has succeeded, call a "commit" callback during last block_job_completed. > > > > Does that make sense? > > I think you've skipped the hard part: immediate cancellation. If a job > is cancelled by the user or a job fails, then all other jobs are > cancelled immediately. > > Immediate cancellation has the problem that jobs could be running in any > AioContext, so you need to handle concurrency. That's where the > locking, juggling AioContexts, and interaction between blockjobs comes > in. OK, let me try again: The idea is intercepting job->cb so we can handle jobs completely in block_job_completed (which is in main loop), rather than the coroutines that can be any AioContext. 1) If a job is cancelled or failed, it goes to block_job_completed immediately, with block_job_is_cancelled() == true. In this case, we call block_job_cancel_sync on all other jobs, and then call "abort()" callbacks to reclaim any bitmaps, then emit QMP events. Some other jobs may have already completed before this point, but it's not a problem because we always defer the actual completions (abort/commit and QMP) altogether. 2) If there is no job failed or canceled, in the last block_job_completed, we call "commit()" to abdicate bitmaps, and emit the QMP events. This would still require BlockJobTxn to track the block jobs in a group, but hopefully it could reduce the complexity of interactions between block jobs. I can prototype it if this isn't missing anything obvious. Fam > > If immediate cancellation is removed then this patch becomes simple: > block_job_txn_job_done() becomes a yield and until the jobs_pending > counter reaches 0. > > But now the user must handle cancellation and error cases manually by > invoking QMP block-job-cancel on all the other jobs. In other words, > we're requiring that the user implements their own BlockJobTxn struct > that keeps track of related jobs. If they don't keep track then they > may not be able to terminate jobs after failure. > > This is why QEMU should be the one to complete or fail all block jobs in > the transaction. It's simpler if we don't do that but the API then > requires more complexity in the user to be used correctly.