Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Stefan Hajnoczi]

On Wed, Jun 15, 2016 at 9:53 AM, Paolo Bonzini <address@hidden> wrote:
> On 14/06/2016 20:17, Stefan Hajnoczi wrote:
>> Block jobs are coroutines that usually perform I/O but sometimes also
>> sleep or yield.  Currently only sleeping or yielded block jobs can be
>> paused.  This means jobs that do not sleep or yield (using
>> block_job_yield()) are unaffected by block_job_pause().
>>
>> Add block_job_pause_point() so that block jobs can mark quiescent points
>> that are suitable for pausing.  This solves the problem that it can take
>> a block job a long time to pause if it is performing a long series of
>> I/O operations.
>>
>> Transitioning to paused state involves a .pause()/.resume() callback.
>> These callbacks are used to ensure that I/O and event loop activity has
>> ceased while the job is at a pause point.
>>
>> Note that this patch introduces a stricter pause state than previously.
>> The job->busy flag was incorrectly documented as a quiescent state
>> without I/O pending.  This is violated by any job that has I/O pending
>> across sleep or block_job_yield(), like the mirror block job.
>
> Right, we should document job->busy as a quiescent state where no one
> will re-enter the coroutine.

That statement doesn't correspond with how it's used:

block_job_sleep_ns() leaves a timer pending and the job will re-enter
when the timer expires.  So "no one will re-enter the coroutine" is
too strict.

The important thing is it's safe to call block_job_enter().  In the
block_job_sleep_ns() case the timer is cancelled to prevent doubly
re-entry.

The doc comment I have in v4 allows the block_job_sleep_ns() case:

  /*
   * Set to false by the job while the coroutine has yielded and may be
   * re-entered by block_job_enter().  There may still be I/O or event loop
   * activity pending.
   */
  bool busy;

Stefan