blockjob: kick jobs on set-speed
PATCH 1/1 blockjob: kick jobs on set-speed
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.
--- blockjob.c | 5 +++++ 1 file changed, 5 insertions(+)
blockjob.c +5 -0
@@ -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)
Signature · 1 line
2.14.3
Quoted 26 lines · 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 <[email protected]>--- blockjob.c | 5 +++++ 1 file changed, 5 insertions(+)blockjob.c +1
-0@@ -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 );
blockjob.c void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) · line 500
Quoted 5 lines
blockjob.c +2
-0@@ -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).
Quoted 8 lines
blockjob.c +2
-0+block_job_enter (job); + } }void block_job_complete (BlockJob *job,Error **errp)Signature · 1 line
2.14.3
Quoted 29 lines · Jeff Cody
Quoted 28 lines · 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 <[email protected]>--- blockjob.c | 5 +++++ 1 file changed, 5 insertions(+)blockjob.c +1
-0@@ -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) }
Quoted 8 lines
blockjob.c void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) · line 500
Quoted 3 lines
blockjob.c +2
-0job->speed = speed; +/* Kick the job to recompute its delay */ +if ((speed > old_speed) &&timer_pending (&job->sleep_timer)) {
job->sleep_timeris protected byblock_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.
Quoted 5 lines
But in any case, I think we could get rid of the
timer_pendingcheck, and just always kick the job if we have a speed increase.block_job_enter()should do the right thing (mutex protected check onjob->busyandjob->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.
Quoted 9 lines
Quoted 8 lines
blockjob.c +2
-0+block_job_enter (job); + } }void block_job_complete (BlockJob *job,Error **errp)Signature · 1 line
2.14.3
Quoted 50 lines · John Snow
Quoted 29 lines · Jeff Cody
Quoted 28 lines · 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 <[email protected]>--- blockjob.c | 5 +++++ 1 file changed, 5 insertions(+)blockjob.c +1
-0@@ -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) }Quoted 8 lines
blockjob.c void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) · line 500
Quoted 3 lines
blockjob.c +2
-0job->speed = speed; +/* Kick the job to recompute its delay */ +if ((speed > old_speed) &&timer_pending (&job->sleep_timer)) {
job->sleep_timeris protected byblock_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_contextand the QEMU global mutex here.Quoted 5 lines
But in any case, I think we could get rid of the
timer_pendingcheck, and just always kick the job if we have a speed increase.block_job_enter()should do the right thing (mutex protected check onjob->busyandjob->sleep_timer).
Quoted 8 lines
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.
Quoted 55 lines · Stefan Hajnoczi
Quoted 54 lines · John Snow
Quoted 29 lines · Jeff Cody
Quoted 28 lines · 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 <[email protected]>--- blockjob.c | 5 +++++ 1 file changed, 5 insertions(+)blockjob.c +1
-0@@ -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) }Quoted 8 lines
blockjob.c void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) · line 500
Quoted 3 lines
blockjob.c +2
-0job->speed = speed; +/* Kick the job to recompute its delay */ +if ((speed > old_speed) &&timer_pending (&job->sleep_timer)) {
job->sleep_timeris protected byblock_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_contextand the QEMU global mutex here.Quoted 5 lines
But in any case, I think we could get rid of the
timer_pendingcheck, and just always kick the job if we have a speed increase.block_job_enter()should do the right thing (mutex protected check onjob->busyandjob->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.
Quoted 8 lines
Quoted 4 lines
(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.