Re: [Qemu-block] [Qemu-devel] [RFC v3 05/14] blockjobs: add block_job_dismiss

[John Snow]

On 01/29/2018 12:38 PM, Kevin Wolf wrote:
> Am 27.01.2018 um 03:05 hat John Snow geschrieben:
>> For jobs that have reached their terminal state, prior to having their
>> last reference put down (meaning jobs that have completed successfully,
>> unsuccessfully, or have been canceled), allow the user to dismiss the
>> job's lingering status report via block-job-dismiss.
>>
>> This gives management APIs the chance to conclusively determine if a job
>> failed or succeeded, even if the event broadcast was missed.
>>
>> Note that jobs do not yet linger in any such state, they are freed
>> immediately upon reaching this previously-unnamed state. such a state is
>> added immediately in the next commit.
>>
>> Valid objects:
>> Concluded: (added in a future commit); dismisses the concluded job.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/trace-events       |  1 +
>>  blockdev.c               | 14 ++++++++++++++
>>  blockjob.c               | 30 ++++++++++++++++++++++++++++++
>>  include/block/blockjob.h |  9 +++++++++
>>  qapi/block-core.json     | 19 +++++++++++++++++++
>>  5 files changed, 73 insertions(+)
>>
>> diff --git a/block/trace-events b/block/trace-events
>> index 11c8d5f590..8f61566770 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -46,6 +46,7 @@ qmp_block_job_cancel(void *job) "job %p"
>>  qmp_block_job_pause(void *job) "job %p"
>>  qmp_block_job_resume(void *job) "job %p"
>>  qmp_block_job_complete(void *job) "job %p"
>> +qmp_block_job_dismiss(void *job) "job %p"
>>  qmp_block_stream(void *bs, void *job) "bs %p job %p"
>>  
>>  # block/file-win32.c
>> diff --git a/blockdev.c b/blockdev.c
>> index 2c0773bba7..5e8edff322 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3849,6 +3849,20 @@ void qmp_block_job_complete(const char *device, Error 
>> **errp)
>>      aio_context_release(aio_context);
>>  }
>>  
>> +void qmp_block_job_dismiss(const char *id, Error **errp)
>> +{
>> +    AioContext *aio_context;
>> +    BlockJob *job = find_block_job(id, &aio_context, errp);
>> +
>> +    if (!job) {
>> +        return;
>> +    }
>> +
>> +    trace_qmp_block_job_dismiss(job);
>> +    block_job_dismiss(&job, errp);
>> +    aio_context_release(aio_context);
>> +}
>> +
>>  void qmp_change_backing_file(const char *device,
>>                               const char *image_node_name,
>>                               const char *backing_file,
>> diff --git a/blockjob.c b/blockjob.c
>> index ea216aca5e..5531f5c2ab 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -58,6 +58,7 @@ enum BlockJobVerb {
>>      BLOCK_JOB_VERB_RESUME,
>>      BLOCK_JOB_VERB_SET_SPEED,
>>      BLOCK_JOB_VERB_COMPLETE,
>> +    BLOCK_JOB_VERB_DISMISS,
>>      BLOCK_JOB_VERB__MAX
>>  };
>>  
>> @@ -68,6 +69,7 @@ bool 
>> BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
>>      [BLOCK_JOB_VERB_RESUME]               = {0, 0, 0, 1, 0},
>>      [BLOCK_JOB_VERB_SET_SPEED]            = {0, 1, 1, 1, 1},
>>      [BLOCK_JOB_VERB_COMPLETE]             = {0, 0, 0, 0, 1},
>> +    [BLOCK_JOB_VERB_DISMISS]              = {0, 0, 0, 0, 0},
> 
> This makes it very obvious that the patches should probably be
> reordered.
> 

I think this bit is fine, but the half-baked dismiss error checking and
the broken transition to UNDEFINED before deference makes a better case.
I'll play with it.

>>  };
>>  
>>  static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
>> @@ -426,6 +428,13 @@ static void block_job_cancel_async(BlockJob *job)
>>      job->cancelled = true;
>>  }
>>  
>> +static void block_job_do_dismiss(BlockJob *job)
>> +{
>> +    assert(job && job->manual == true);
>> +    block_job_state_transition(job, BLOCK_JOB_STATUS_UNDEFINED);
> 
> I don't think you made that an allowed transition from anywhere?
> 

Ah, you know, this is true... until the next commit.

>> +    block_job_unref(job);
>> +}
>> +
>>  static int block_job_finish_sync(BlockJob *job,
>>                                   void (*finish)(BlockJob *, Error **errp),
>>                                   Error **errp)
>> @@ -455,6 +464,9 @@ static int block_job_finish_sync(BlockJob *job,
>>          aio_poll(qemu_get_aio_context(), true);
>>      }
>>      ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
>> +    if (job->manual) {
>> +        block_job_do_dismiss(job);
>> +    }
>>      block_job_unref(job);
>>      return ret;
>>  }
>> @@ -570,6 +582,24 @@ void block_job_complete(BlockJob *job, Error **errp)
>>      job->driver->complete(job, errp);
>>  }
>>  
>> +void block_job_dismiss(BlockJob **jobptr, Error **errp)
>> +{
>> +    BlockJob *job = *jobptr;
>> +    /* similarly to _complete, this is QMP-interface only. */
>> +    assert(job->id);
>> +    if (!job->manual) {
>> +        error_setg(errp, "The active block job '%s' was not started with "
>> +                   "\'manual\': true, and so cannot be dismissed as it will 
>> "
>> +                   "clean up after itself automatically", job->id);
>> +        return;
>> +    }
> 
> If you check instead that the job is in the right state to be dismissed
> (CONCLUDED), the job->manual check wouldn't be needed any more because
> the user can never catch an automatically completed job in CONCLUDED.
> 
>> +    error_setg(errp, "unimplemented");
> 
> This should hopefully go away when you reorder the patches.
> 
>> +    block_job_do_dismiss(job);
>> +    *jobptr = NULL;
>> +}
> 
> Kevin
> 

Thanks,

--js