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 <eespo...@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
>