From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44215) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7pYY-0008Ba-4G for qemu-devel@nongnu.org; Wed, 24 Jun 2015 14:37:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7pYS-0006D5-9H for qemu-devel@nongnu.org; Wed, 24 Jun 2015 14:37:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55011) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7pYS-0006Cu-25 for qemu-devel@nongnu.org; Wed, 24 Jun 2015 14:37:48 -0400 Message-ID: <558AF8F7.7010803@redhat.com> Date: Wed, 24 Jun 2015 20:37:43 +0200 From: Max Reitz MIME-Version: 1.0 References: <1434103761-29871-1-git-send-email-stefanha@redhat.com> <1434103761-29871-6-git-send-email-stefanha@redhat.com> In-Reply-To: <1434103761-29871-6-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 5/9] block: add block job transactions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , qemu-devel@nongnu.org Cc: Kevin Wolf , famz@redhat.com, Jeff Cody , vsementsov@parallels.com, John Snow On 12.06.2015 12:09, Stefan Hajnoczi wrote: > Sometimes block jobs must execute as a transaction group. Finishing > jobs wait until all other jobs are ready to complete successfully. > Failure or cancellation of one job cancels the other jobs in the group. > > Signed-off-by: Stefan Hajnoczi > --- > blockjob.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 1 + > include/block/block_int.h | 3 +- > include/block/blockjob.h | 49 ++++++++++++++ > trace-events | 4 ++ > 5 files changed, 216 insertions(+), 1 deletion(-) > > diff --git a/blockjob.c b/blockjob.c > index 2755465..ff622f5 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -399,3 +399,163 @@ void block_job_defer_to_main_loop(BlockJob *job, > > qemu_bh_schedule(data->bh); > } > + > +/* Transactional group of block jobs */ > +struct BlockJobTxn { > + /* Jobs may be in different AioContexts so protect all fields */ > + QemuMutex lock; > + > + /* Reference count for txn object */ > + unsigned int ref; > + > + /* Is this txn cancelling its jobs? */ > + bool aborting; > + > + /* Number of jobs still running */ > + unsigned int jobs_pending; > + > + /* List of jobs */ > + QLIST_HEAD(, BlockJob) jobs; > +}; > + > +BlockJobTxn *block_job_txn_new(void) > +{ > + BlockJobTxn *txn = g_new(BlockJobTxn, 1); > + qemu_mutex_init(&txn->lock); > + txn->ref = 1; /* dropped by block_job_txn_begin() */ > + txn->aborting = false; > + txn->jobs_pending = 0; > + QLIST_INIT(&txn->jobs); > + return txn; > +} > + > +static void block_job_txn_unref(BlockJobTxn *txn) > +{ > + qemu_mutex_lock(&txn->lock); > + > + if (--txn->ref > 0) { > + qemu_mutex_unlock(&txn->lock); > + return; > + } > + > + qemu_mutex_unlock(&txn->lock); > + qemu_mutex_destroy(&txn->lock); > + g_free(txn); > +} > + > +/* The purpose of this is to keep txn alive until all jobs have been added */ > +void block_job_txn_begin(BlockJobTxn *txn) > +{ > + block_job_txn_unref(txn); > +} > + > +void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job) > +{ > + if (!txn) { > + return; > + } Do you plan on making use of this case? I'm asking because while I'm usually in favor of handling everything gracefully as long as it's easy to implement, here I can't think of a case where using NULL with this function makes sense, that is to me it would seem like the caller made some bad mistake. This in turn would mean that dereferencing a NULL pointer or failing an assertion were preferable to hiding that mistake. Other than this small thing and that it doesn't compile (until patch 7, I presume), looks good. Max