Re: [PATCH for-6.2 v3 05/12] job: @force parameter for job_cancel_sync{,_all}()

[Hanna Reitz <hreitz@redhat.com>]

On 01.09.21 12:20, Vladimir Sementsov-Ogievskiy wrote:
> 06.08.2021 12:38, Max Reitz wrote:
>> Callers should be able to specify whether they want job_cancel_sync() to
>> force-cancel the job or not.
>>
>> In fact, almost all invocations do not care about consistency of the
>> result and just want the job to terminate as soon as possible, so they
>> should pass force=true.  The replication block driver is the exception.
>>
>> This changes some iotest outputs, because quitting qemu while a mirror
>> job is active will now lead to it being cancelled instead of completed,
>> which is what we want.  (Cancelling a READY mirror job with force=false
>> may take an indefinite amount of time, which we do not want when
>> quitting.  If users want consistent results, they must have all jobs be
>> done before they quit qemu.)
>>
>> Buglink:https://gitlab.com/qemu-project/qemu/-/issues/462
>> Signed-off-by: Max Reitz<mreitz@redhat.com>
>> ---
>>   include/qemu/job.h                    | 10 ++---
>>   block/replication.c                   |  4 +-
>>   blockdev.c                            |  4 +-
>>   job.c                                 | 20 +++++++--
>>   qemu-nbd.c                            |  2 +-
>>   softmmu/runstate.c                    |  2 +-
>>   storage-daemon/qemu-storage-daemon.c  |  2 +-
>>   tests/unit/test-block-iothread.c      |  2 +-
>>   tests/unit/test-blockjob.c            |  2 +-
>>   tests/qemu-iotests/109.out            | 60 +++++++++++----------------
>>   tests/qemu-iotests/tests/qsd-jobs.out |  2 +-
>>   11 files changed, 55 insertions(+), 55 deletions(-)
>>
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 41162ed494..5e8edbc2c8 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -506,19 +506,19 @@ void job_user_cancel(Job *job, bool force, 
>> Error **errp);
>>     /**
>>    * Synchronously cancel the @job.  The completion callback is called
>> - * before the function returns.  The job may actually complete
>> - * instead of canceling itself; the circumstances under which this
>> - * happens depend on the kind of job that is active.
>> + * before the function returns.  If @force is false, the job may
>> + * actually complete instead of canceling itself; the circumstances
>> + * under which this happens depend on the kind of job that is active.
>>    *
>>    * Returns the return value from the job if the job actually completed
>>    * during the call, or -ECANCELED if it was canceled.
>>    *
>>    * Callers must hold the AioContext lock of job->aio_context.
>>    */
>> -int job_cancel_sync(Job *job);
>> +int job_cancel_sync(Job *job, bool force);
>>     /** Synchronously cancels all jobs using job_cancel_sync(). */
>> -void job_cancel_sync_all(void);
>> +void job_cancel_sync_all(bool force);
>
> I think it would be better to keep job_cancel_sync_all(void) prototype 
> and just change its behavior to do force-cancel. Anyway, this patch 
> always pass true to it. And it would be strange to do soft-cancel-all, 
> keeping in mind that soft cancelling only make sense for mirror in 
> ready state.

Actually, yes, that’s true.  I’ll drop the parameter.

Hanna