From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46215) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeUUg-0005Um-6X for qemu-devel@nongnu.org; Tue, 22 Sep 2015 16:48:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZeUUc-0005ox-2e for qemu-devel@nongnu.org; Tue, 22 Sep 2015 16:48:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35820) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeUUb-0005oh-R8 for qemu-devel@nongnu.org; Tue, 22 Sep 2015 16:48:50 -0400 References: <1442889976-8733-1-git-send-email-famz@redhat.com> From: John Snow Message-ID: <5601BEAF.9060706@redhat.com> Date: Tue, 22 Sep 2015 16:48:47 -0400 MIME-Version: 1.0 In-Reply-To: <1442889976-8733-1-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Jeff Cody , vsementsov@parallels.com, stefanha@redhat.com, Max Reitz On 09/21/2015 10:46 PM, Fam Zheng wrote: > v7: Add Eric's rev-by in 1, 11. > Add Max's rev-by in 4, 5, 9, 10, 11. > Add John's rev-by in 5, 6, 8. > Fix wording for 6. [John] > Fix comment of block_job_txn_add_job() in 9. [Max] > Remove superfluous hunks, and document default value in 11. [Eric] > Update Makefile dep in 14. [Max] > > v6: Address comments from Max and Eric (many thanks for reviewing!): > Add Max's reviews. > Don't leak txn. [Max] > Unusual comment ending "**/" -> "*/". [Eric] > Fix the stale block_job_txn_prepare_to_complete comment. [Max] > Move "block_job_txn_unref" to below FOREACH block. [Max] > Split "base" and "txn" in qapi schema, so that transactional-cancel is only > visible in transaction. [Eric] > > v5: Address comments from Max Reitz: > 2.4 -> 2.5 in qapi docs. > Don't leak txn object. > English syntax fixes. > Really leave the "cancelled" status of failed job. > Remove a superfluous added line. > > This is based on top of the work by Stefan Hajnoczi and John Snow. > > Recap: motivation for block job transactions > -------------------------------------------- > If an incremental backup block job fails then we reclaim the bitmap so > the job can be retried. The problem comes when multiple jobs are started as > part of a qmp 'transaction' command. We need to group these jobs in a > transaction so that either all jobs complete successfully or all bitmaps are > reclaimed. > > Without transactions, there is a case where some jobs complete successfully and > throw away their bitmaps, making it impossible to retry the backup by rerunning > the command if one of the jobs fails. > > How does this implementation work? > ---------------------------------- > These patches add a BlockJobTxn object with the following API: > > txn = block_job_txn_new(); > block_job_txn_add_job(txn, job1); > block_job_txn_add_job(txn, job2); > > The jobs either both complete successfully or they both fail/cancel. If the > user cancels job1 then job2 will also be cancelled and vice versa. > > Jobs objects stay alive waiting for other jobs to complete, even if the > coroutines have returned. They can be cancelled by the user during this time. > Job blockers are still in effect and no other block job can run on this device > in the meantime (since QEMU currently only allows 1 job per device). This is > the main drawback to this approach but reasonable since you probably don't want > to run other jobs/operations until you're sure the backup was successful (you > won't be able to retry a failed backup if there's a new job running). > > > Fam Zheng (6): > backup: Extract dirty bitmap handling as a separate function > blockjob: Introduce reference count > blockjob: Add .commit and .abort block job actions > blockjob: Add "completed" and "ret" in BlockJob > blockjob: Simplify block_job_finish_sync > block: Add block job transactions > > John Snow (4): > qapi: Add transaction support to block-dirty-bitmap operations > iotests: add transactional incremental backup test > block: rename BlkTransactionState and BdrvActionOps > iotests: 124 - transactional failure test > > Kashyap Chamarthy (1): > qmp-commands.hx: Update the supported 'transaction' operations > > Stefan Hajnoczi (3): > blockdev: make BlockJobTxn available to qmp 'transaction' > block/backup: support block job transactions > tests: add BlockJobTxn unit test > > block.c | 19 ++- > block/backup.c | 50 +++++-- > block/mirror.c | 2 +- > blockdev.c | 354 +++++++++++++++++++++++++++++++++++---------- > blockjob.c | 185 +++++++++++++++++++---- > docs/bitmaps.md | 6 +- > include/block/block.h | 2 +- > include/block/block_int.h | 6 +- > include/block/blockjob.h | 85 ++++++++++- > qapi-schema.json | 10 +- > qapi/block-core.json | 27 +++- > qmp-commands.hx | 21 ++- > tests/Makefile | 3 + > tests/qemu-iotests/124 | 182 ++++++++++++++++++++++- > tests/qemu-iotests/124.out | 4 +- > tests/test-blockjob-txn.c | 244 +++++++++++++++++++++++++++++++ > 16 files changed, 1056 insertions(+), 144 deletions(-) > create mode 100644 tests/test-blockjob-txn.c > Reminder: needed as a new patch -- an update to the documentation concerning the transactional-cancel argument added in patch 11/14 for docs/bitmaps.md. --js