From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEqeO-0004sw-EX for qemu-devel@nongnu.org; Mon, 13 Jul 2015 23:12:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEqeJ-0000im-AR for qemu-devel@nongnu.org; Mon, 13 Jul 2015 23:12:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41300) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEqeJ-0000ia-2r for qemu-devel@nongnu.org; Mon, 13 Jul 2015 23:12:51 -0400 Date: Tue, 14 Jul 2015 11:12:46 +0800 From: Fam Zheng Message-ID: <20150714031246.GB24907@ad.nay.redhat.com> References: <1436500012-32593-1-git-send-email-famz@redhat.com> <1436500012-32593-13-git-send-email-famz@redhat.com> <55A44644.1090604@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55A44644.1090604@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 12/15] block/backup: support block job transactions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: Kevin Wolf , Markus Armbruster , Jeff Cody , qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, stefanha@redhat.com On Mon, 07/13 19:14, John Snow wrote: > > +static void backup_txn_commit(BlockJob *job) > > +{ > > + BackupBlockJob *s = container_of(job, BackupBlockJob, common); > > + if (s->sync_bitmap) { > > + backup_handle_dirty_bitmap(s, 0); > > + } > > +} > > + > > +static void backup_txn_abort(BlockJob *job) > > +{ > > + BackupBlockJob *s = container_of(job, BackupBlockJob, common); > > + if (s->sync_bitmap) { > > + backup_handle_dirty_bitmap(s, -1); > > + } > > +} > > + > > If you're going to check for sync_bitmap out here in these two functions > instead of inside backup_handle_dirty_bitmap, add an assertion into the > called function that job->sync_bitmap *will* be present. The assertion is not really necessary because we'll get a seg fault if it's NULL. > > > static const BlockJobDriver backup_job_driver = { > > .instance_size = sizeof(BackupBlockJob), > > .job_type = BLOCK_JOB_TYPE_BACKUP, > > .set_speed = backup_set_speed, > > .iostatus_reset = backup_iostatus_reset, > > + .txn_commit = backup_txn_commit, > > + .txn_abort = backup_txn_abort, > > }; > > > > static BlockErrorAction backup_error_action(BackupBlockJob *job, > > @@ -444,7 +462,7 @@ static void coroutine_fn backup_run(void *opaque) > > qemu_co_rwlock_wrlock(&job->flush_rwlock); > > qemu_co_rwlock_unlock(&job->flush_rwlock); > > > > - if (job->sync_bitmap) { > > + if (!job->common.txn && job->sync_bitmap) { > > backup_handle_dirty_bitmap(job, ret); > > } > > Can we add a little call to blockjobs, like: > > bool block_cleanup_needed(BlockJob *job) > { > return job->common.txn == NULL; > } > > To make this status check look less magical? > Then, if the sync bitmap check is pushed into backup_handle, you can > just simply say: > > if (blockjob_cleanup_needed(job)) { > backup_handle_dirty_bitmap(job, ret); > } > > which IMO is nicer. Good idea. > > > hbitmap_free(job->bitmap); > > @@ -463,7 +481,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > > BlockdevOnError on_source_error, > > BlockdevOnError on_target_error, > > BlockCompletionFunc *cb, void *opaque, > > - Error **errp) > > + BlockJobTxn *txn, Error **errp) > > { > > int64_t len; > > > > @@ -545,6 +563,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, > > sync_bitmap : NULL; > > job->common.len = len; > > job->common.co = qemu_coroutine_create(backup_run); > > + block_job_txn_add_job(txn, &job->common); > > OK, so all backup jobs will participate in the transaction -- but they > can control this based on whether or not they pass forward the txn > parameter. It's a NOP if txn is NULL. > > > qemu_coroutine_enter(job->common.co, job); > > return; > >