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

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

[John Snow]

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>
---
 blockjob.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 715c2c2680..43f01ad190 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -483,6 +483,7 @@ static void block_job_completed_txn_success(BlockJob *job)
 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 +496,10 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
     }
 
     job->speed = speed;
+    /* Kick the job to recompute its delay */
+    if ((speed > old_speed) && timer_pending(&job->sleep_timer)) {
+        block_job_enter(job);
+    }
 }
 
 void block_job_complete(BlockJob *job, Error **errp)
-- 
2.14.3

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

[Jeff Cody]

On Mon, Dec 11, 2017 at 06:46:09PM -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>
> ---
>  blockjob.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 715c2c2680..43f01ad190 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -483,6 +483,7 @@ static void block_job_completed_txn_success(BlockJob *job)
>  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 +496,10 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>      }
>  
>      job->speed = speed;
> +    /* Kick the job to recompute its delay */
> +    if ((speed > old_speed) && timer_pending(&job->sleep_timer)) {

job->sleep_timer is protected by block_job_mutex (via
block_job_lock/unlock); is it safe for us to check it here outside the
mutex?

But in any case, I think we could get rid of the timer_pending check, and
just always kick the job if we have a speed increase.  block_job_enter()
should do the right thing (mutex protected check on job->busy and
job->sleep_timer).

> +        block_job_enter(job);
> +    }
>  }
>  
>  void block_job_complete(BlockJob *job, Error **errp)
> -- 
> 2.14.3
> 

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

[John Snow]

On 12/11/2017 07:08 PM, Jeff Cody wrote:
> On Mon, Dec 11, 2017 at 06:46:09PM -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>
>> ---
>>  blockjob.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/blockjob.c b/blockjob.c
>> index 715c2c2680..43f01ad190 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -483,6 +483,7 @@ static void block_job_completed_txn_success(BlockJob *job)
>>  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 +496,10 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>>      }
>>  
>>      job->speed = speed;
>> +    /* Kick the job to recompute its delay */
>> +    if ((speed > old_speed) && timer_pending(&job->sleep_timer)) {
> 
> job->sleep_timer is protected by block_job_mutex (via
> block_job_lock/unlock); is it safe for us to check it here outside the
> mutex?
> 

My hunch is that in this specific case that it is; but only because of
assumptions about holding the aio_context and the QEMU global mutex here.

> But in any case, I think we could get rid of the timer_pending check, and
> just always kick the job if we have a speed increase.  block_job_enter()
> should do the right thing (mutex protected check on job->busy and
> job->sleep_timer).
> 

I could lock it for inarguable correctness; I just didn't want to kick a
job that didn't actually require any kicking to limit any potential
problems from that interaction.

(I'm fond of the extra conditional because I feel like it makes the
intent of the kick explicit.)

I can remove it.

>> +        block_job_enter(job);
>> +    }
>>  }
>>  
>>  void block_job_complete(BlockJob *job, Error **errp)
>> -- 
>> 2.14.3
>>
> 

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

On Tue, Dec 12, 2017 at 01:22:28PM -0500, John Snow wrote:
> 
> 
> On 12/11/2017 07:08 PM, Jeff Cody wrote:
> > On Mon, Dec 11, 2017 at 06:46:09PM -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>
> >> ---
> >>  blockjob.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/blockjob.c b/blockjob.c
> >> index 715c2c2680..43f01ad190 100644
> >> --- a/blockjob.c
> >> +++ b/blockjob.c
> >> @@ -483,6 +483,7 @@ static void block_job_completed_txn_success(BlockJob *job)
> >>  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 +496,10 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> >>      }
> >>  
> >>      job->speed = speed;
> >> +    /* Kick the job to recompute its delay */
> >> +    if ((speed > old_speed) && timer_pending(&job->sleep_timer)) {
> > 
> > job->sleep_timer is protected by block_job_mutex (via
> > block_job_lock/unlock); is it safe for us to check it here outside the
> > mutex?
> > 
> 
> My hunch is that in this specific case that it is; but only because of
> assumptions about holding the aio_context and the QEMU global mutex here.
> 
> > But in any case, I think we could get rid of the timer_pending check, and
> > just always kick the job if we have a speed increase.  block_job_enter()
> > should do the right thing (mutex protected check on job->busy and
> > job->sleep_timer).
> > 
> 
> I could lock it for inarguable correctness; I just didn't want to kick a
> job that didn't actually require any kicking to limit any potential
> problems from that interaction.
> 
> (I'm fond of the extra conditional because I feel like it makes the
> intent of the kick explicit.)
> 
> I can remove it.

Removing the conditional would introduce a bug.  block_job_enter() will
unpause the job.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[John Snow]

On 12/13/2017 05:48 AM, Stefan Hajnoczi wrote:
> On Tue, Dec 12, 2017 at 01:22:28PM -0500, John Snow wrote:
>>
>>
>> On 12/11/2017 07:08 PM, Jeff Cody wrote:
>>> On Mon, Dec 11, 2017 at 06:46:09PM -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>
>>>> ---
>>>>  blockjob.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/blockjob.c b/blockjob.c
>>>> index 715c2c2680..43f01ad190 100644
>>>> --- a/blockjob.c
>>>> +++ b/blockjob.c
>>>> @@ -483,6 +483,7 @@ static void block_job_completed_txn_success(BlockJob *job)
>>>>  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 +496,10 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>>>>      }
>>>>  
>>>>      job->speed = speed;
>>>> +    /* Kick the job to recompute its delay */
>>>> +    if ((speed > old_speed) && timer_pending(&job->sleep_timer)) {
>>>
>>> job->sleep_timer is protected by block_job_mutex (via
>>> block_job_lock/unlock); is it safe for us to check it here outside the
>>> mutex?
>>>
>>
>> My hunch is that in this specific case that it is; but only because of
>> assumptions about holding the aio_context and the QEMU global mutex here.
>>
>>> But in any case, I think we could get rid of the timer_pending check, and
>>> just always kick the job if we have a speed increase.  block_job_enter()
>>> should do the right thing (mutex protected check on job->busy and
>>> job->sleep_timer).
>>>
>>
>> I could lock it for inarguable correctness; I just didn't want to kick a
>> job that didn't actually require any kicking to limit any potential
>> problems from that interaction.
>>
>> (I'm fond of the extra conditional because I feel like it makes the
>> intent of the kick explicit.)
>>
>> I can remove it.
> 
> Removing the conditional would introduce a bug.  block_job_enter() will
> unpause the job.
> 

It will almost certainly pause again immediately, but maybe not before
work is performed.