[PATCH 0/3] Add support for sequential backups

[PATCH 0/3] Add support for sequential backups

[Stefan Reiter]

Backups can already be done for multiple drives in a transaction. However, these
jobs will start all at once, potentially hogging a lot of disk IO all at once.
This problem is made worse, since IO throttling is only available on a per-job
basis.

Add a flag to QMP to support sequential transactions for backups. This way,
every job will be executed one after the other, while still providing the
benefit of transactions (i.e. once one fails, all remaining ones will be
cancelled).

We've internally (in Proxmox VE) been doing sequential backups for a long time
with great success, albeit in a different fashion. This series is the result of
aligning our internal changes closer to upstream, and might be useful for other
people as well.


Stefan Reiter (3):
  job: add sequential transaction support
  blockdev: add sequential mode to *-backup transactions
  backup: initialize bcs bitmap on job create, not start

 block/backup.c        |  4 ++--
 blockdev.c            | 25 ++++++++++++++++++++++---
 include/qemu/job.h    | 12 ++++++++++++
 job.c                 | 24 ++++++++++++++++++++++++
 qapi/transaction.json |  6 +++++-
 5 files changed, 65 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/3] job: add sequential transaction support

[Stefan Reiter]

Jobs in a sequential transaction should never be started with job_start
manually. job_txn_start_seq and the sequentially called job_start will
take care of it, 'assert'ing in case a job is already running or has
finished.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 include/qemu/job.h | 12 ++++++++++++
 job.c              | 24 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 32aabb1c60..f7a6a0926a 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -280,6 +280,18 @@ typedef enum JobCreateFlags {
  */
 JobTxn *job_txn_new(void);
 
+/**
+ * Create a new transaction and set it to sequential mode, i.e. run all jobs
+ * one after the other instead of at the same time.
+ */
+JobTxn *job_txn_new_seq(void);
+
+/**
+ * Helper method to start the first job in a sequential transaction to kick it
+ * off. Other jobs will be run after this one completes.
+ */
+void job_txn_start_seq(JobTxn *txn);
+
 /**
  * Release a reference that was previously acquired with job_txn_add_job or
  * job_txn_new. If it's the last reference to the object, it will be freed.
diff --git a/job.c b/job.c
index 8fecf38960..4df7c1d2ca 100644
--- a/job.c
+++ b/job.c
@@ -72,6 +72,8 @@ struct JobTxn {
 
     /* Reference count */
     int refcnt;
+
+    bool sequential;
 };
 
 /* Right now, this mutex is only needed to synchronize accesses to job->busy
@@ -102,6 +104,25 @@ JobTxn *job_txn_new(void)
     return txn;
 }
 
+JobTxn *job_txn_new_seq(void)
+{
+    JobTxn *txn = job_txn_new();
+    txn->sequential = true;
+    return txn;
+}
+
+void job_txn_start_seq(JobTxn *txn)
+{
+    assert(txn->sequential);
+    assert(!txn->aborting);
+
+    Job *first = QLIST_FIRST(&txn->jobs);
+    assert(first);
+    assert(first->status == JOB_STATUS_CREATED);
+
+    job_start(first);
+}
+
 static void job_txn_ref(JobTxn *txn)
 {
     txn->refcnt++;
@@ -840,6 +861,9 @@ static void job_completed_txn_success(Job *job)
      */
     QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
         if (!job_is_completed(other_job)) {
+            if (txn->sequential) {
+                job_start(other_job);
+            }
             return;
         }
         assert(other_job->ret == 0);
-- 
2.20.1

[PATCH 2/3] blockdev: add sequential mode to *-backup transactions

[Stefan Reiter]

Only supported with completion-mode 'grouped', since it relies on a
JobTxn to exist. This means that for now it is only available for
{drive,blockdev}-backup transactions.

Since only one job will be running at a time, bandwidth-limits can be
applied effectively. It can also prevent overloading a host's IO
capabilities in general.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 blockdev.c            | 25 ++++++++++++++++++++++---
 qapi/transaction.json |  6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab..3691e5e791 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1826,7 +1826,10 @@ static void drive_backup_commit(BlkActionState *common)
     aio_context_acquire(aio_context);
 
     assert(state->job);
-    job_start(&state->job->job);
+
+    if (!common->txn_props->sequential) {
+        job_start(&state->job->job);
+    }
 
     aio_context_release(aio_context);
 }
@@ -1927,7 +1930,9 @@ static void blockdev_backup_commit(BlkActionState *common)
     aio_context_acquire(aio_context);
 
     assert(state->job);
-    job_start(&state->job->job);
+    if (!common->txn_props->sequential) {
+        job_start(&state->job->job);
+    }
 
     aio_context_release(aio_context);
 }
@@ -2303,6 +2308,11 @@ static TransactionProperties *get_transaction_properties(
         props->completion_mode = ACTION_COMPLETION_MODE_INDIVIDUAL;
     }
 
+    if (!props->has_sequential) {
+        props->has_sequential = true;
+        props->sequential = false;
+    }
+
     return props;
 }
 
@@ -2328,7 +2338,11 @@ void qmp_transaction(TransactionActionList *dev_list,
      */
     props = get_transaction_properties(props);
     if (props->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
-        block_job_txn = job_txn_new();
+        block_job_txn = props->sequential ? job_txn_new_seq() : job_txn_new();
+    } else if (props->sequential) {
+        error_setg(errp, "Sequential transaction mode is not supported with "
+                         "completion-mode = individual");
+        return;
     }
 
     /* drain all i/o before any operations */
@@ -2367,6 +2381,11 @@ void qmp_transaction(TransactionActionList *dev_list,
         }
     }
 
+    /* jobs in sequential txns don't start themselves on commit */
+    if (block_job_txn && props->sequential) {
+        job_txn_start_seq(block_job_txn);
+    }
+
     /* success */
     goto exit;
 
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 15ddebdbc3..4808383088 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -84,11 +84,15 @@
 #                   Actions will complete or fail as a group.
 #                   See @ActionCompletionMode for details.
 #
+# @sequential: Run the jobs in the transaction one after the other, instead
+#              of all at once. Not supported for completion-mode 'individual'.
+#
 # Since: 2.5
 ##
 { 'struct': 'TransactionProperties',
   'data': {
-       '*completion-mode': 'ActionCompletionMode'
+       '*completion-mode': 'ActionCompletionMode',
+       '*sequential': 'bool'
   }
 }
 
-- 
2.20.1

[PATCH 3/3] backup: initialize bcs bitmap on job create, not start

[Stefan Reiter]

After backup_init_bcs_bitmap the copy-before-write behaviour is active.
This way, multiple backup jobs created at once but running in a
sequential transaction will still represent the same point in time.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

I'd imagine this was done on job start for a purpose, so this is potentially
wrong. In testing it works fine.

Sent along for feedback, since it would be necessary to really make use of the
sequential backup feature (without it, the individual backup jobs would not have
a consistent view of the guest).


 block/backup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 4f13bb20a5..14660eef45 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -237,8 +237,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
     int ret = 0;
 
-    backup_init_bcs_bitmap(s);
-
     if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
         int64_t offset = 0;
         int64_t count;
@@ -471,6 +469,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
                        &error_abort);
 
+    backup_init_bcs_bitmap(job);
+
     return &job->common;
 
  error:
-- 
2.20.1

Re: [PATCH 0/3] Add support for sequential backups

[Stefan Reiter]

Friendly ping :)

On 8/26/20 2:13 PM, Stefan Reiter wrote:
> Backups can already be done for multiple drives in a transaction. However, these
> jobs will start all at once, potentially hogging a lot of disk IO all at once.
> This problem is made worse, since IO throttling is only available on a per-job
> basis.
> 
> Add a flag to QMP to support sequential transactions for backups. This way,
> every job will be executed one after the other, while still providing the
> benefit of transactions (i.e. once one fails, all remaining ones will be
> cancelled).
> 
> We've internally (in Proxmox VE) been doing sequential backups for a long time
> with great success, albeit in a different fashion. This series is the result of
> aligning our internal changes closer to upstream, and might be useful for other
> people as well.
> 
> 
> Stefan Reiter (3):
>    job: add sequential transaction support
>    blockdev: add sequential mode to *-backup transactions
>    backup: initialize bcs bitmap on job create, not start
> 
>   block/backup.c        |  4 ++--
>   blockdev.c            | 25 ++++++++++++++++++++++---
>   include/qemu/job.h    | 12 ++++++++++++
>   job.c                 | 24 ++++++++++++++++++++++++
>   qapi/transaction.json |  6 +++++-
>   5 files changed, 65 insertions(+), 6 deletions(-)
>