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

[Jeff Cody <jcody@redhat.com>]

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