Re: [PATCH v6 06/18] jobs: protect jobs with job_lock/unlock

[Emanuele Giuseppe Esposito]

Am 03/06/2022 um 18:40 schrieb Kevin Wolf:
> Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
>> Introduce the job locking mechanism through the whole job API,
>> following the comments  in job.h and requirements of job-monitor
>> (like the functions in job-qmp.c, assume lock is held) and
>> job-driver (like in mirror.c and all other JobDriver, lock is not held).
>>
>> Use the _locked helpers introduced before to differentiate
>> between functions called with and without job_mutex.
>> This only applies to function that are called under both
>> cases, all the others will be renamed later.
>>
>> job_{lock/unlock} is independent from real_job_{lock/unlock}.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>  block.c             |  18 ++++---
>>  block/replication.c |   8 ++-
>>  blockdev.c          |  17 ++++--
>>  blockjob.c          |  56 +++++++++++++-------
>>  job-qmp.c           |   2 +
>>  job.c               | 125 +++++++++++++++++++++++++++++++-------------
>>  monitor/qmp-cmds.c  |   6 ++-
>>  qemu-img.c          |  41 +++++++++------
>>  8 files changed, 187 insertions(+), 86 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 718e4cae8b..5dc46fde11 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4978,7 +4978,9 @@ static void bdrv_close(BlockDriverState *bs)
>>  
>>  void bdrv_close_all(void)
>>  {
>> -    assert(job_next(NULL) == NULL);
>> +    WITH_JOB_LOCK_GUARD() {
>> +        assert(job_next(NULL) == NULL);
>> +    }
>>      GLOBAL_STATE_CODE();
> 
> This series seems really hard to review patch by patch, in this case
> because I would have to know whether you intended job_next() to be
> called with the lock held or not. Nothing in job.h indicates either way
> at this point in the series.

Well if it's under lock it means all its calls will be under lock. If
some cases will be under lock and some other not, I use the _locked
version, as described in the commit description.

> 
> Patch 11 answers this by actually renaming it job_next_locked(), but
> always having to refer to the final state after the whole series is
> applied is really not how things should work. We're splitting the work
> into individual patches so that the state after each single patch makes
> sense on its own. Otherwise the whole series could as well be a single
> patch. :-(

The various function and ordering has changed pretty much in each of the
6 version I sent, because it is very difficult to understand what comes
first and what can go afterwards.

Anyways, I see what you mean but I would not move patch 11 before this
one, because otherwise we would have _locked functions used without
having even a fake lock around, and the next reviewer would complain. In
fact, I think I put it afterwards because someone initially suggested so.

Ideally we want both patches together, but then it will be a total mess
to read, so I would leave it as it is.

In addition, I don't think it would hurt to have "normal" (ie without
_locked) functions wrapped by a nop macro.

Emanuele

> 
> So I'd argue that patch 11 should probably come before this one.
> 
> Anyway, I guess I'll try to make my way to the end of the series quickly
> and then somehow try to verify whatever the state is then.
> 
> Kevin
>