Re: [Qemu-block] [PATCH v4 09/11] block: drive_backup transaction callback support

2015-05-18 Thread Stefan Hajnoczi
On Mon, May 11, 2015 at 07:04:24PM -0400, John Snow wrote:
 +static void drive_backup_cb(BlkActionState *common)
 +{
 +BlkActionCallbackData *cb_data = common-cb_data;
 +BlockDriverState *bs = cb_data-opaque;
 +DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
 +
 +assert(state-bs == bs);
 +if (bs-job) {
 +assert(state-job == bs-job);
 +}

What is the purpose of the if statement?

Why is it not okay for a new job to have started?

 +
 +state-aio_context = bdrv_get_aio_context(bs);
 +aio_context_acquire(state-aio_context);

The bs-job access above should be protected by aio_context_acquire().


pgp7FkEx19Y09.pgp
Description: PGP signature


Re: [Qemu-block] [PATCH v4 09/11] block: drive_backup transaction callback support

2015-05-18 Thread John Snow


On 05/18/2015 11:35 AM, Stefan Hajnoczi wrote:
 On Mon, May 11, 2015 at 07:04:24PM -0400, John Snow wrote:
 +static void drive_backup_cb(BlkActionState *common) +{ +
 BlkActionCallbackData *cb_data = common-cb_data; +
 BlockDriverState *bs = cb_data-opaque; +DriveBackupState
 *state = DO_UPCAST(DriveBackupState, common, common); + +
 assert(state-bs == bs); +if (bs-job) { +
 assert(state-job == bs-job); +}
 
 What is the purpose of the if statement?
 
 Why is it not okay for a new job to have started?
 

Hmm, maybe it's fine -- It was just my thought that it probably
/shouldn't/ occur under normal circumstances.

I think my assumption was that we want to impose an ordering that job
cleanup occurs before another job launches, in general.

I think, though, that you wanted to start allowing non-conflicting
jobs to run concurrently, though, so I'll just eye over this series
again to make sure it's okay for cleanup to happen after another job
starts ...

...Provided the second job does not fiddle with bitmaps, of course. We
should clean those up before another bitmap job starts, definitely.

 + +state-aio_context = bdrv_get_aio_context(bs); +
 aio_context_acquire(state-aio_context);
 
 The bs-job access above should be protected by
 aio_context_acquire().
 

Thanks,
--js



[Qemu-block] [PATCH v4 09/11] block: drive_backup transaction callback support

2015-05-11 Thread John Snow
This patch actually implements the transactional callback system
for the drive_backup action.

(1) We manually pick up a reference to the bitmap if present to allow
its cleanup to be delayed until after all drive_backup jobs launched
by the transaction have fully completed.

(2) We create a functional closure that envelops the original drive_backup
callback, to be able to intercept the completion status and return code
for the job.

(3) We add the drive_backup_cb method for the drive_backup action, which
unpacks the completion information and invokes the final cleanup.

(4) backup_transaction_complete will perform the final cleanup on the
backup job.

(5) In the case of transaction cancellation, drive_backup_cb is still
responsible for cleaning up the mess we may have already made.

Signed-off-by: John Snow js...@redhat.com
Reviewed-by: Max Reitz mre...@redhat.com
---
 block/backup.c|  9 
 blockdev.c| 53 ---
 include/block/block_int.h |  8 +++
 3 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 4ac0be8..1634c88 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -233,6 +233,15 @@ typedef struct {
 int ret;
 } BackupCompleteData;
 
+void backup_transaction_complete(BlockJob *job, int ret)
+{
+BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+
+if (s-sync_bitmap) {
+bdrv_frozen_bitmap_decref(job-bs, s-sync_bitmap, ret);
+}
+}
+
 static void backup_complete(BlockJob *job, void *opaque)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common);
diff --git a/blockdev.c b/blockdev.c
index f391e18..c438949 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1426,7 +1426,6 @@ static void transaction_action_callback(void *opaque, int 
ret)
  *
  * @return The callback to be used instead of @callback.
  */
-__attribute__((__unused__))
 static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
  void *opaque,
  CallbackFn *callback,
@@ -1454,7 +1453,6 @@ static CallbackFn *new_action_cb_wrapper(BlkActionState 
*common,
 /**
  * Undo any actions performed by the above call.
  */
-__attribute__((__unused__))
 static void cancel_action_cb_wrapper(BlkActionState *common)
 {
 /* Stage 0: Wrapper was never created: */
@@ -1806,6 +1804,7 @@ static void do_drive_backup(const char *device, const 
char *target,
 BlockdevOnError on_target_error,
 BlockCompletionFunc *cb, void *opaque,
 Error **errp);
+static void block_job_cb(void *opaque, int ret);
 
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
@@ -1814,6 +1813,9 @@ static void drive_backup_prepare(BlkActionState *common, 
Error **errp)
 BlockBackend *blk;
 DriveBackup *backup;
 Error *local_err = NULL;
+CallbackFn *cb;
+void *opaque;
+BdrvDirtyBitmap *bmap = NULL;
 
 assert(common-action-kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
 backup = common-action-drive_backup;
@@ -1825,6 +1827,19 @@ static void drive_backup_prepare(BlkActionState *common, 
Error **errp)
 }
 bs = blk_bs(blk);
 
+/* BackupBlockJob is opaque to us, so look up the bitmap ourselves */
+if (backup-has_bitmap) {
+bmap = bdrv_find_dirty_bitmap(bs, backup-bitmap);
+if (!bmap) {
+error_setg(errp, Bitmap '%s' could not be found, backup-bitmap);
+return;
+}
+}
+
+/* Create our transactional callback wrapper,
+   and register that we'd like to call .cb() later. */
+cb = new_action_cb_wrapper(common, bs, block_job_cb, opaque);
+
 /* AioContext is released in .clean() */
 state-aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(state-aio_context);
@@ -1837,7 +1852,7 @@ static void drive_backup_prepare(BlkActionState *common, 
Error **errp)
 backup-has_bitmap, backup-bitmap,
 backup-has_on_source_error, backup-on_source_error,
 backup-has_on_target_error, backup-on_target_error,
-NULL, NULL,
+cb, opaque,
 local_err);
 if (local_err) {
 error_propagate(errp, local_err);
@@ -1846,6 +1861,12 @@ static void drive_backup_prepare(BlkActionState *common, 
Error **errp)
 
 state-bs = bs;
 state-job = state-bs-job;
+/* Keep the job alive until .cb(), too:
+ * References are only incremented after the job launches successfully. */
+block_job_incref(state-job);
+if (bmap) {
+bdrv_dirty_bitmap_incref(bmap);
+}
 }
 
 static void drive_backup_abort(BlkActionState *common)
@@ -1857,6 +1878,10 @@ static void drive_backup_abort(BlkActionState *common)
 if (bs  bs-job  bs-job == state-job) {