Re: [Qemu-devel] [RFC PATCH 01/33] blockjob: Wrappers for progress counter access

[Eric Blake]

On 04/24/2018 10:24 AM, Kevin Wolf wrote:
> Block job drivers are not expected to mess with the internal of the

s/internal/internals/

> BlockJob object, so provide wrapper functions for one of the cases where
> they still do it: Updating the progress counter.
> 
> Signed-off-by: Kevin Wolf 
> ---

> +++ b/include/block/blockjob.h
> @@ -278,6 +278,25 @@ void block_job_finalize(BlockJob *job, Error **errp);
>  void block_job_dismiss(BlockJob **job, Error **errp);
>  
>  /**
> + * block_job_progress_update:
> + * @job: The job that has made progress
> + * @done: How much progress the job made
> + *
> + * Updates the progress counter of the job.
> + */
> +void block_job_progress_update(BlockJob *job, uint64_t done);

We already document (and libvirt does as well) that a job progress meter
does not necessarily represent anything obviously correlating to the
real job, other than that the ratio between them serves as an estimate
of completion; and that we are free to move not only the progress
indicator, but also the end goal, over the course of a job, and that a
job can even appear to move backwards when comparing the ratios (for
example, getting the pairs 0/1, 1/1000, 50/1000, 800/1200, 800/1300,
1200/1300, 1/1 over the life of a job is a valid possibility for a
sequence, including the apparent backwards motion at the 800/1300 step).
 But this new API only modifies one of the two (arbitrary) stats - is
that sufficient?

/me reads on...

> +
> +/**
> + * block_job_progress_set_remaining:
> + * @job: The job whose expected progress end value is set
> + * @remaining: Expected end value of the progress counter of the job
> + *
> + * Sets the expected end value of the progress counter of a job so that a
> + * completion percentage can be calculated when the progress is updated.
> + */
> +void block_job_progress_set_remaining(BlockJob *job, uint64_t remaining);

...Oh. You have two separate functions to modify the two arbitrary
values at will.  Why not just a single function?  Maybe seeing how the
callers use it will demonstrate that two separate functions is slightly
easier...

> +
> +/**
>   * block_job_query:
>   * @job: The job to get information about.
>   *
> diff --git a/block/backup.c b/block/backup.c
> index 453cd62c24..5d95805472 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -39,6 +39,7 @@ typedef struct BackupBlockJob {
>  BlockdevOnError on_source_error;
>  BlockdevOnError on_target_error;
>  CoRwlock flush_rwlock;
> +uint64_t len;
>  uint64_t bytes_read;
>  int64_t cluster_size;
>  bool compress;
> @@ -118,7 +119,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>  
>  trace_backup_do_cow_process(job, start);
>  
> -n = MIN(job->cluster_size, job->common.len - start);
> +n = MIN(job->cluster_size, job->len - start);
>  
>  if (!bounce_buffer) {
>  bounce_buffer = blk_blockalign(blk, job->cluster_size);
> @@ -159,7 +160,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>   * offset field is an opaque progress value, it is not a disk offset.
>   */
>  job->bytes_read += n;
> -job->common.offset += n;
> +block_job_progress_update(>common, n);

...Okay, when the end goal isn't changing, updating just the progress is
indeed a bit easier.

>  }
>  
>  out:
> @@ -261,7 +262,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
>  return;
>  }
>  
> -len = DIV_ROUND_UP(backup_job->common.len, backup_job->cluster_size);
> +len = DIV_ROUND_UP(backup_job->len, backup_job->cluster_size);
>  hbitmap_set(backup_job->copy_bitmap, 0, len);
>  }
>  
> @@ -420,8 +421,9 @@ static void 
> backup_incremental_init_copy_bitmap(BackupBlockJob *job)
>  bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
>  }
>  
> -job->common.offset = job->common.len -
> - hbitmap_count(job->copy_bitmap) * job->cluster_size;
> +/* TODO block_job_progress_set_remaining() would make more sense */
> +block_job_progress_update(>common,
> +job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);

Hmm. The old code couldn't touch job->common.len; the new code now has
two separate lengths (job->len that can't be touched, and
job->common.len that is now internal to stat reporting and therefore no
longer something that we can't touch).  The old code makes the job skip
forward according to the size of the bitmap that is not set (since we
don't have to do any work on that portion), which could indeed be a
rapid skip followed by the rest of the job moving at a slower pace;
where the new code could perhaps instead modify the goal to match just
the size that the bitmap occupies, so that the progress is more linearly
related.  So you're right that
block_job_progress_set_remaining(-hbitmap_count(job->copy_bitmap) *
job->cluster_size) (the negative adjustment is intentional) might make

[Qemu-devel] [RFC PATCH 01/33] blockjob: Wrappers for progress counter access

[Kevin Wolf]

Block job drivers are not expected to mess with the internal of the
BlockJob object, so provide wrapper functions for one of the cases where
they still do it: Updating the progress counter.

Signed-off-by: Kevin Wolf 
---
 include/block/blockjob.h | 19 +++
 block/backup.c   | 22 +-
 block/commit.c   | 16 
 block/mirror.c   | 11 +--
 block/stream.c   | 14 --
 blockjob.c   | 10 ++
 6 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index fc645dac68..a2cc52233b 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -278,6 +278,25 @@ void block_job_finalize(BlockJob *job, Error **errp);
 void block_job_dismiss(BlockJob **job, Error **errp);
 
 /**
+ * block_job_progress_update:
+ * @job: The job that has made progress
+ * @done: How much progress the job made
+ *
+ * Updates the progress counter of the job.
+ */
+void block_job_progress_update(BlockJob *job, uint64_t done);
+
+/**
+ * block_job_progress_set_remaining:
+ * @job: The job whose expected progress end value is set
+ * @remaining: Expected end value of the progress counter of the job
+ *
+ * Sets the expected end value of the progress counter of a job so that a
+ * completion percentage can be calculated when the progress is updated.
+ */
+void block_job_progress_set_remaining(BlockJob *job, uint64_t remaining);
+
+/**
  * block_job_query:
  * @job: The job to get information about.
  *
diff --git a/block/backup.c b/block/backup.c
index 453cd62c24..5d95805472 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -39,6 +39,7 @@ typedef struct BackupBlockJob {
 BlockdevOnError on_source_error;
 BlockdevOnError on_target_error;
 CoRwlock flush_rwlock;
+uint64_t len;
 uint64_t bytes_read;
 int64_t cluster_size;
 bool compress;
@@ -118,7 +119,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
 
 trace_backup_do_cow_process(job, start);
 
-n = MIN(job->cluster_size, job->common.len - start);
+n = MIN(job->cluster_size, job->len - start);
 
 if (!bounce_buffer) {
 bounce_buffer = blk_blockalign(blk, job->cluster_size);
@@ -159,7 +160,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
  * offset field is an opaque progress value, it is not a disk offset.
  */
 job->bytes_read += n;
-job->common.offset += n;
+block_job_progress_update(>common, n);
 }
 
 out:
@@ -261,7 +262,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
 return;
 }
 
-len = DIV_ROUND_UP(backup_job->common.len, backup_job->cluster_size);
+len = DIV_ROUND_UP(backup_job->len, backup_job->cluster_size);
 hbitmap_set(backup_job->copy_bitmap, 0, len);
 }
 
@@ -420,8 +421,9 @@ static void 
backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
 }
 
-job->common.offset = job->common.len -
- hbitmap_count(job->copy_bitmap) * job->cluster_size;
+/* TODO block_job_progress_set_remaining() would make more sense */
+block_job_progress_update(>common,
+job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
 
 bdrv_dirty_iter_free(dbi);
 }
@@ -437,7 +439,9 @@ static void coroutine_fn backup_run(void *opaque)
 QLIST_INIT(>inflight_reqs);
 qemu_co_rwlock_init(>flush_rwlock);
 
-nb_clusters = DIV_ROUND_UP(job->common.len, job->cluster_size);
+nb_clusters = DIV_ROUND_UP(job->len, job->cluster_size);
+block_job_progress_set_remaining(>common, job->len);
+
 job->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
 if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 backup_incremental_init_copy_bitmap(job);
@@ -461,7 +465,7 @@ static void coroutine_fn backup_run(void *opaque)
 ret = backup_run_incremental(job);
 } else {
 /* Both FULL and TOP SYNC_MODE's require copying.. */
-for (offset = 0; offset < job->common.len;
+for (offset = 0; offset < job->len;
  offset += job->cluster_size) {
 bool error_is_read;
 int alloced = 0;
@@ -620,7 +624,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 goto error;
 }
 
-/* job->common.len is fixed, so we can't allow resize */
+/* job->len is fixed, so we can't allow resize */
 job = block_job_create(job_id, _job_driver, txn, bs,
BLK_PERM_CONSISTENT_READ,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
@@ -676,7 +680,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 /* Required permissions are already taken with target's blk_new() */
 block_job_add_bdrv(>common, "target", target, 0, BLK_PERM_ALL,