[Qemu-devel] [PATCH v2] blockjob: kick jobs on set-speed

John Snow posted 1 patch 6 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171213204611.26276-1-jsnow@redhat.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
blockjob.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH v2] blockjob: kick jobs on set-speed
Posted by John Snow 6 years, 3 months ago
If users set an unreasonably low speed (like one byte per second), the
calculated delay may exceed many hours. While we like to punish users
for asking for stupid things, we do also like to allow users to correct
their wicked ways.

When a user provides a new speed, kick the job to allow it to recalculate
its delay.

Signed-off-by: John Snow <jsnow@redhat.com>
---

RFC: Why is block_job_mutex shared between all jobs,
     instead of being per-job?

 blockjob.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index 715c2c2680..6173e4728c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -59,6 +59,7 @@ static void __attribute__((__constructor__)) block_job_init(void)
 
 static void block_job_event_cancelled(BlockJob *job);
 static void block_job_event_completed(BlockJob *job, const char *msg);
+static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
 
 /* Transactional group of block jobs */
 struct BlockJobTxn {
@@ -480,9 +481,16 @@ static void block_job_completed_txn_success(BlockJob *job)
     }
 }
 
+/* Assumes the block_job_mutex is held */
+static bool block_job_timer_pending(BlockJob *job)
+{
+    return timer_pending(&job->sleep_timer);
+}
+
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
     Error *local_err = NULL;
+    int64_t old_speed = job->speed;
 
     if (!job->driver->set_speed) {
         error_setg(errp, QERR_UNSUPPORTED);
@@ -495,6 +503,12 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
     }
 
     job->speed = speed;
+    if (speed <= old_speed) {
+        return;
+    }
+
+    /* kick only if a timer is pending */
+    block_job_enter_cond(job, block_job_timer_pending);
 }
 
 void block_job_complete(BlockJob *job, Error **errp)
@@ -821,7 +835,11 @@ void block_job_resume_all(void)
     }
 }
 
-void block_job_enter(BlockJob *job)
+/*
+ * Conditionally enter a block_job pending a call to fn() while
+ * under the block_job_lock critical section.
+ */
+static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job))
 {
     if (!block_job_started(job)) {
         return;
@@ -836,6 +854,11 @@ void block_job_enter(BlockJob *job)
         return;
     }
 
+    if (fn && !fn(job)) {
+        block_job_unlock();
+        return;
+    }
+
     assert(!job->deferred_to_main_loop);
     timer_del(&job->sleep_timer);
     job->busy = true;
@@ -843,6 +866,11 @@ void block_job_enter(BlockJob *job)
     aio_co_wake(job->co);
 }
 
+void block_job_enter(BlockJob *job)
+{
+    block_job_enter_cond(job, NULL);
+}
+
 bool block_job_is_cancelled(BlockJob *job)
 {
     return job->cancelled;
-- 
2.14.3


Re: [Qemu-devel] [PATCH v2] blockjob: kick jobs on set-speed
Posted by Paolo Bonzini 6 years, 3 months ago
On 13/12/2017 21:46, John Snow wrote:
> 
> When a user provides a new speed, kick the job to allow it to recalculate
> its delay.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 
> RFC: Why is block_job_mutex shared between all jobs,
>      instead of being per-job?

Because that patch was partly extracted out of a bigger one, and I was
both lazy and worried about breaking things close to the release.

In other words more uses of the mutex are coming, and it will need to be
shared between jobs to work fine with transactions and monitor commands
(which don't know the job they're working on until they've looked it
up).  Starting a paused block job is not a hot path. :)

Paolo

Re: [Qemu-devel] [PATCH v2] blockjob: kick jobs on set-speed
Posted by Stefan Hajnoczi 6 years, 3 months ago
On Wed, Dec 13, 2017 at 03:46:11PM -0500, John Snow wrote:
> If users set an unreasonably low speed (like one byte per second), the
> calculated delay may exceed many hours. While we like to punish users
> for asking for stupid things, we do also like to allow users to correct
> their wicked ways.
> 
> When a user provides a new speed, kick the job to allow it to recalculate
> its delay.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 
> RFC: Why is block_job_mutex shared between all jobs,
>      instead of being per-job?
> 
>  blockjob.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [Qemu-devel] [PATCH v2] blockjob: kick jobs on set-speed
Posted by Jeff Cody 6 years, 3 months ago
On Wed, Dec 13, 2017 at 03:46:11PM -0500, John Snow wrote:
> If users set an unreasonably low speed (like one byte per second), the
> calculated delay may exceed many hours. While we like to punish users
> for asking for stupid things, we do also like to allow users to correct
> their wicked ways.
> 
> When a user provides a new speed, kick the job to allow it to recalculate
> its delay.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 
> RFC: Why is block_job_mutex shared between all jobs,
>      instead of being per-job?
>

The mutex protects job-specific fields of job->busy and job->sleep_timer, so
I think the mutex should be specific to job, not global across all jobs.


>  blockjob.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 715c2c2680..6173e4728c 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -59,6 +59,7 @@ static void __attribute__((__constructor__)) block_job_init(void)
>  
>  static void block_job_event_cancelled(BlockJob *job);
>  static void block_job_event_completed(BlockJob *job, const char *msg);
> +static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
>  
>  /* Transactional group of block jobs */
>  struct BlockJobTxn {
> @@ -480,9 +481,16 @@ static void block_job_completed_txn_success(BlockJob *job)
>      }
>  }
>  
> +/* Assumes the block_job_mutex is held */
> +static bool block_job_timer_pending(BlockJob *job)
> +{
> +    return timer_pending(&job->sleep_timer);
> +}
> +
>  void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>  {
>      Error *local_err = NULL;
> +    int64_t old_speed = job->speed;
>  
>      if (!job->driver->set_speed) {
>          error_setg(errp, QERR_UNSUPPORTED);
> @@ -495,6 +503,12 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>      }
>  
>      job->speed = speed;
> +    if (speed <= old_speed) {
> +        return;
> +    }
> +
> +    /* kick only if a timer is pending */
> +    block_job_enter_cond(job, block_job_timer_pending);
>  }
>  
>  void block_job_complete(BlockJob *job, Error **errp)
> @@ -821,7 +835,11 @@ void block_job_resume_all(void)
>      }
>  }
>  
> -void block_job_enter(BlockJob *job)
> +/*
> + * Conditionally enter a block_job pending a call to fn() while
> + * under the block_job_lock critical section.
> + */
> +static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job))
>  {
>      if (!block_job_started(job)) {
>          return;
> @@ -836,6 +854,11 @@ void block_job_enter(BlockJob *job)
>          return;
>      }
>  
> +    if (fn && !fn(job)) {
> +        block_job_unlock();
> +        return;
> +    }
> +
>      assert(!job->deferred_to_main_loop);
>      timer_del(&job->sleep_timer);
>      job->busy = true;
> @@ -843,6 +866,11 @@ void block_job_enter(BlockJob *job)
>      aio_co_wake(job->co);
>  }
>  
> +void block_job_enter(BlockJob *job)
> +{
> +    block_job_enter_cond(job, NULL);
> +}
> +
>  bool block_job_is_cancelled(BlockJob *job)
>  {
>      return job->cancelled;
> -- 
> 2.14.3
>



Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc block

-Jeff