Re: [PATCH v8 05/20] job.c: add job_lock/unlock while keeping job.h intact - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
To: Emanuele Giuseppe Esposito <eesposit@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>, John Snow <jsnow@redhat.com>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	Wen Congyang <wencongyang2@huawei.com>,
	Xie Changlong <xiechanglong.d@gmail.com>,
	Markus Armbruster <armbru@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v8 05/20] job.c: add job_lock/unlock while keeping job.h intact
Date: Tue, 5 Jul 2022 13:23:07 +0300	[thread overview]
Message-ID: <df24032a-bbf6-72d3-a307-5a4771be843d@yandex-team.ru> (raw)
In-Reply-To: <20220629141538.3400679-6-eesposit@redhat.com>

In general looks good to me.

On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote:
> With "intact" we mean that all job.h functions implicitly
> take the lock. Therefore API callers are unmodified.
> 
> This means that:
> - all static functions become _locked, and call _locked functions

Some static functions don't have _locked prefix.. That's, maybe, not wrong. But it contradicts with commit message and looks inconsistent.

For example job_started and job_should_pause are similar simple getters, job_shoud_pause is updated to be _locked, but job_started is not updated..

job_exit, job_co_entry are correct exclusions

> - all public functions take the lock internally, and call _locked
>    functions

may be just, "all public function take the lock internally if needed", as some public funcitons don't need the lock, like job_txn_new or job_progress_* functions

> - all public functions called internally by other functions in job.c will have a
>    _locked counterpart, to avoid deadlocks (job lock already taken)

counterparts sometimes made public and are unused for now. That's OK, just mention.

> - public functions called only from exernal files (not job.c) do not
>    have _locked() counterpart and take the lock inside

Some small public functions still don't take the lock inside and don't have _locked() prefix.

job_is_internal
job_type
job_type_str

job_sleep_timer_cb is static, but it's called only by external source, so it should take lock inside, like public functions. (and like job_exit)

job_pause_point_locked, job_user_resume_locked, job_complete_locked: need to mention in a comment, that function may temporarily unlock the mutex

Also, the commennt "realeases it temporarily" should be propagated to all _locked callers of the function

> 
> 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>
> ---
>   include/qemu/job.h |  73 +++++-
>   job.c              | 607 +++++++++++++++++++++++++++++++--------------

[..]

>   
> -    AIO_WAIT_WHILE(job->aio_context,
> -                   (job_enter(job), !job_is_completed(job)));
> +    job_unlock();
> +    AIO_WAIT_WHILE_UNLOCKED(job->aio_context,
> +                            (job_enter(job), !job_is_completed(job)));

Is it correct here to move to UNLOCKED wait? We don't touch any other aio-context related things in this patch..

Even if it's OK, it should be in another commit I think.

> +    job_lock();
>   
> -    ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
> -    job_unref(job);
> +    ret = (job_is_cancelled_locked(job) && job->ret == 0)
> +          ? -ECANCELED : job->ret;
> +    job_unref_locked(job);
>       return ret;
>   }
> +
> +int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)
> +{
> +    JOB_LOCK_GUARD();
> +    return job_finish_sync_locked(job, finish, errp);
> +}
> \ No newline at end of file

No newline at end of file... I think, good to add one)

-- 
Best regards,
Vladimir