[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 08/20] blockjob.h: introduce block_job _locked() APIs
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v8 08/20] blockjob.h: introduce block_job _locked() APIs |
Date: |
Wed, 6 Jul 2022 14:05:06 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
Am 05/07/2022 um 17:01 schrieb Vladimir Sementsov-Ogievskiy:
> On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote:
>> Just as done with job.h, create _locked() functions in blockjob.h
>
> We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject.
>
> Also, we start to introduce _locked block_job_* APIs.
>
> Does it mean that BlockJob and Job share the global mutex to protect
> themselves? Than I think we should document in BlockJob struct what is
> protected by job_mutex.
There is nothing in the struct (apart from Job) that is protected by the
job lock. I can add a comment "Protected by job mutex" on top of Job job
field?
>
> And please, let's be consistent on whether we add or not add "with mutex
> held" / "with mutex not held" comments. For job API you mostly add it
> for each function.. Let's do same here? Same for "temporary unlock"
> comments.
Where did I miss the mutex lock/unlock comments? Yes I forgot the
"temporary unlock" thing but apart from that all functions have a
comment saying if they take the lock or not.
>
>>
>> These functions will be later useful when caller has already taken
>> the lock. All blockjob _locked functions call job _locked functions.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>> blockjob.c | 52 ++++++++++++++++++++++++++++++++--------
>> include/block/blockjob.h | 15 ++++++++++++
>> 2 files changed, 57 insertions(+), 10 deletions(-)
>>
>> diff --git a/blockjob.c b/blockjob.c
>> index 7da59a1f1c..0d59aba439 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -44,21 +44,27 @@ static bool is_block_job(Job *job)
>> job_type(job) == JOB_TYPE_STREAM;
>> }
>> -BlockJob *block_job_next(BlockJob *bjob)
>> +BlockJob *block_job_next_locked(BlockJob *bjob)
>> {
>> Job *job = bjob ? &bjob->job : NULL;
>> GLOBAL_STATE_CODE();
>> do {
>> - job = job_next(job);
>> + job = job_next_locked(job);
>> } while (job && !is_block_job(job));
>> return job ? container_of(job, BlockJob, job) : NULL;
>> }
>> -BlockJob *block_job_get(const char *id)
>> +BlockJob *block_job_next(BlockJob *bjob)
>> {
>> - Job *job = job_get(id);
>> + JOB_LOCK_GUARD();
>> + return block_job_next_locked(bjob);
>> +}
>> +
>> +BlockJob *block_job_get_locked(const char *id)
>> +{
>> + Job *job = job_get_locked(id);
>> GLOBAL_STATE_CODE();
>> if (job && is_block_job(job)) {
>> @@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id)
>> }
>> }
>> +BlockJob *block_job_get(const char *id)
>> +{
>> + JOB_LOCK_GUARD();
>> + return block_job_get_locked(id);
>> +}
>> +
>> void block_job_free(Job *job)
>> {
>> BlockJob *bjob = container_of(job, BlockJob, job);
>> @@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job)
>> return timer_pending(&job->sleep_timer);
>> }
>> -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error
>> **errp)
>> {
>> const BlockJobDriver *drv = block_job_driver(job);
>> int64_t old_speed = job->speed;
>> GLOBAL_STATE_CODE();
>> - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
>> + if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) <
>> 0) {
>> return false;
>> }
>> if (speed < 0) {
>> @@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t
>> speed, Error **errp)
>> job->speed = speed;
>> if (drv->set_speed) {
>> + job_unlock();
>> drv->set_speed(job, speed);
>> + job_lock();
>> }
>> if (speed && speed <= old_speed) {
>> @@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t
>> speed, Error **errp)
>> }
>> /* kick only if a timer is pending */
>> - job_enter_cond(&job->job, job_timer_pending);
>> + job_enter_cond_locked(&job->job, job_timer_pending);
>> return true;
>> }
>> +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>> +{
>> + JOB_LOCK_GUARD();
>> + return block_job_set_speed_locked(job, speed, errp);
>> +}
>> +
>> int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
>> {
>> IO_CODE();
>> return ratelimit_calculate_delay(&job->limit, n);
>> }
>> -BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
>> {
>> BlockJobInfo *info;
>> uint64_t progress_current, progress_total;
>> @@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error
>> **errp)
>> info->len = progress_total;
>> info->speed = job->speed;
>> info->io_status = job->iostatus;
>> - info->ready = job_is_ready(&job->job),
>> + info->ready = job_is_ready_locked(&job->job),
>> info->status = job->job.status;
>> info->auto_finalize = job->job.auto_finalize;
>> info->auto_dismiss = job->job.auto_dismiss;
>> @@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job,
>> Error **errp)
>> return info;
>> }
>> +BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>> +{
>> + JOB_LOCK_GUARD();
>> + return block_job_query_locked(job, errp);
>> +}
>> +
>> static void block_job_iostatus_set_err(BlockJob *job, int error)
>> {
>> if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>> @@ -478,7 +504,7 @@ fail:
>> return NULL;
>> }
>> -void block_job_iostatus_reset(BlockJob *job)
>> +void block_job_iostatus_reset_locked(BlockJob *job)
>> {
>> GLOBAL_STATE_CODE();
>> if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>> @@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job)
>> job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
>> }
>> +void block_job_iostatus_reset(BlockJob *job)
>> +{
>> + JOB_LOCK_GUARD();
>> + block_job_iostatus_reset_locked(job);
>> +}
>> +
>> void block_job_user_resume(Job *job)
>> {
>> BlockJob *bjob = container_of(job, BlockJob, job);
>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>> index 6525e16fd5..3959a98612 100644
>> --- a/include/block/blockjob.h
>> +++ b/include/block/blockjob.h
>> @@ -92,6 +92,9 @@ typedef struct BlockJob {
>> */
>> BlockJob *block_job_next(BlockJob *job);
>> +/* Same as block_job_next(), but called with job lock held. */
>> +BlockJob *block_job_next_locked(BlockJob *job);
>> +
>> /**
>> * block_job_get:
>> * @id: The id of the block job.
>> @@ -102,6 +105,9 @@ BlockJob *block_job_next(BlockJob *job);
>> */
>> BlockJob *block_job_get(const char *id);
>> +/* Same as block_job_get(), but called with job lock held. */
>> +BlockJob *block_job_get_locked(const char *id);
>> +
>> /**
>> * block_job_add_bdrv:
>> * @job: A block job
>> @@ -145,6 +151,9 @@ bool block_job_has_bdrv(BlockJob *job,
>> BlockDriverState *bs);
>> */
>> bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
>> +/* Same as block_job_set_speed(), but called with job lock held. */
>> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error
>> **errp);
>> +
>> /**
>> * block_job_query:
>> * @job: The job to get information about.
>> @@ -153,6 +162,9 @@ bool block_job_set_speed(BlockJob *job, int64_t
>> speed, Error **errp);
>> */
>> BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
>> +/* Same as block_job_query(), but called with job lock held. */
>> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp);
>> +
>> /**
>> * block_job_iostatus_reset:
>> * @job: The job whose I/O status should be reset.
>> @@ -162,6 +174,9 @@ BlockJobInfo *block_job_query(BlockJob *job, Error
>> **errp);
>> */
>> void block_job_iostatus_reset(BlockJob *job);
>> +/* Same as block_job_iostatus_reset(), but called with job lock
>> held. */
>> +void block_job_iostatus_reset_locked(BlockJob *job);
>> +
>> /*
>> * block_job_get_aio_context:
>> *
>
>