1. Patchew
  2. QEMU
  3. [RFC PATCH v2 00/14] job: replace AioContext lock with job_mutex
  4. View patch

[RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU

Emanuele Giuseppe Esposito posted 14 patches 4 years, 3 months ago
Maintainers: Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>, John Snow <jsnow@redhat.com>, Wen Congyang <wencongyang2@huawei.com>, Hanna Reitz <hreitz@redhat.com>, Markus Armbruster <armbru@redhat.com>, Xie Changlong <xiechanglong.d@gmail.com>, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>, Stefan Hajnoczi <stefanha@redhat.com>
There is a newer version of this series
[RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU
Posted by Emanuele Giuseppe Esposito 4 years, 3 months ago 
Once job lock is used and aiocontext is removed, mirror has
to perform job operations under the same critical section,
using the helpers prepared in previous commit.

Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/mirror.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 00089e519b..f22fa7da6e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job)
     BlockDriverState *target_bs;
     BlockDriverState *mirror_top_bs;
     Error *local_err = NULL;
-    bool abort = job->ret < 0;
+    bool abort = job_has_failed(job);
     int ret = 0;
 
     if (s->prepared) {
@@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp)
     s->should_complete = true;
 
     /* If the job is paused, it will be re-entered when it is resumed */
-    if (!job->paused) {
-        job_enter(job);
-    }
+    job_enter_not_paused(job);
 }
 
 static void coroutine_fn mirror_pause(Job *job)
@@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job)
      * from one of our own drain sections, to avoid a deadlock waiting for
      * ourselves.
      */
-    if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) {
+    if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) {
         return true;
     }
 
-- 
2.27.0
Re: [RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU
Posted by Vladimir Sementsov-Ogievskiy 4 years, 1 month ago 
04.11.2021 17:53, Emanuele Giuseppe Esposito wrote:
> Once job lock is used and aiocontext is removed, mirror has
> to perform job operations under the same critical section,
> using the helpers prepared in previous commit.
> 
> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are *nop*.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/mirror.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 00089e519b..f22fa7da6e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job)
>       BlockDriverState *target_bs;
>       BlockDriverState *mirror_top_bs;
>       Error *local_err = NULL;
> -    bool abort = job->ret < 0;
> +    bool abort = job_has_failed(job);
>       int ret = 0;
>   
>       if (s->prepared) {
> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp)
>       s->should_complete = true;
>   
>       /* If the job is paused, it will be re-entered when it is resumed */
> -    if (!job->paused) {
> -        job_enter(job);
> -    }
> +    job_enter_not_paused(job);
>   }
>   
>   static void coroutine_fn mirror_pause(Job *job)
> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job)
>        * from one of our own drain sections, to avoid a deadlock waiting for
>        * ourselves.
>        */
> -    if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) {
> +    if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) {
>           return true;
>       }
>   
> 

Why to introduce a separate API function for every use case?

Could we instead just use WITH_JOB_LOCK_GUARD() ?


-- 
Best regards,
Vladimir
Re: [RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU
Posted by Emanuele Giuseppe Esposito 4 years, 1 month ago 

On 18/12/2021 12:53, Vladimir Sementsov-Ogievskiy wrote:
> 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote:
>> Once job lock is used and aiocontext is removed, mirror has
>> to perform job operations under the same critical section,
>> using the helpers prepared in previous commit.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block/mirror.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 00089e519b..f22fa7da6e 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job)
>>       BlockDriverState *target_bs;
>>       BlockDriverState *mirror_top_bs;
>>       Error *local_err = NULL;
>> -    bool abort = job->ret < 0;
>> +    bool abort = job_has_failed(job);
>>       int ret = 0;
>>       if (s->prepared) {
>> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp)
>>       s->should_complete = true;
>>       /* If the job is paused, it will be re-entered when it is 
>> resumed */
>> -    if (!job->paused) {
>> -        job_enter(job);
>> -    }
>> +    job_enter_not_paused(job);
>>   }
>>   static void coroutine_fn mirror_pause(Job *job)
>> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job)
>>        * from one of our own drain sections, to avoid a deadlock 
>> waiting for
>>        * ourselves.
>>        */
>> -    if (!s->common.job.paused && !job_is_cancelled(&job->job) && 
>> !s->in_drain) {
>> +    if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) {
>>           return true;
>>       }
>>
> 
> Why to introduce a separate API function for every use case?
> 
> Could we instead just use WITH_JOB_LOCK_GUARD() ?
> 

This implies making the struct job_mutex public. Is that ok for you?

Thank you,
Emanuele
Re: [RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU
Posted by Vladimir Sementsov-Ogievskiy 4 years, 1 month ago 
20.12.2021 13:34, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 18/12/2021 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote:
>>> Once job lock is used and aiocontext is removed, mirror has
>>> to perform job operations under the same critical section,
>>> using the helpers prepared in previous commit.
>>>
>>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>>> are *nop*.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   block/mirror.c | 8 +++-----
>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 00089e519b..f22fa7da6e 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job)
>>>       BlockDriverState *target_bs;
>>>       BlockDriverState *mirror_top_bs;
>>>       Error *local_err = NULL;
>>> -    bool abort = job->ret < 0;
>>> +    bool abort = job_has_failed(job);
>>>       int ret = 0;
>>>       if (s->prepared) {
>>> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp)
>>>       s->should_complete = true;
>>>       /* If the job is paused, it will be re-entered when it is resumed */
>>> -    if (!job->paused) {
>>> -        job_enter(job);
>>> -    }
>>> +    job_enter_not_paused(job);
>>>   }
>>>   static void coroutine_fn mirror_pause(Job *job)
>>> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job)
>>>        * from one of our own drain sections, to avoid a deadlock waiting for
>>>        * ourselves.
>>>        */
>>> -    if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) {
>>> +    if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) {
>>>           return true;
>>>       }
>>>
>>
>> Why to introduce a separate API function for every use case?
>>
>> Could we instead just use WITH_JOB_LOCK_GUARD() ?
>>
> 
> This implies making the struct job_mutex public. Is that ok for you?
> 

Yes, I think it's OK.

Alternatively, you can use job_lock() / job_unlock(), or even rewrite WITH_JOB_LOCK_GUARD() macro using job_lock/job_unlock, to keep mutex private.. But I don't think it really worth it now.

Note that struct Job is already public, so if we'll use per-job mutex in future it still is not a problem. Only when we decide to make struct Job private, we'll have to decide something about JOB_LOCK_GUARD(), and at this point we'll just rewrite it to work through some helper function instead of directly touching the mutex.


-- 
Best regards,
Vladimir
Re: [RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU
Posted by Emanuele Giuseppe Esposito 4 years, 1 month ago 

On 20/12/2021 11:47, Vladimir Sementsov-Ogievskiy wrote:
> 20.12.2021 13:34, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 18/12/2021 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote:
>>>> Once job lock is used and aiocontext is removed, mirror has
>>>> to perform job operations under the same critical section,
>>>> using the helpers prepared in previous commit.
>>>>
>>>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>>>> are *nop*.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>>   block/mirror.c | 8 +++-----
>>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>> index 00089e519b..f22fa7da6e 100644
>>>> --- a/block/mirror.c
>>>> +++ b/block/mirror.c
>>>> @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job)
>>>>       BlockDriverState *target_bs;
>>>>       BlockDriverState *mirror_top_bs;
>>>>       Error *local_err = NULL;
>>>> -    bool abort = job->ret < 0;
>>>> +    bool abort = job_has_failed(job);
>>>>       int ret = 0;
>>>>       if (s->prepared) {
>>>> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error 
>>>> **errp)
>>>>       s->should_complete = true;
>>>>       /* If the job is paused, it will be re-entered when it is 
>>>> resumed */
>>>> -    if (!job->paused) {
>>>> -        job_enter(job);
>>>> -    }
>>>> +    job_enter_not_paused(job);
>>>>   }
>>>>   static void coroutine_fn mirror_pause(Job *job)
>>>> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job)
>>>>        * from one of our own drain sections, to avoid a deadlock 
>>>> waiting for
>>>>        * ourselves.
>>>>        */
>>>> -    if (!s->common.job.paused && !job_is_cancelled(&job->job) && 
>>>> !s->in_drain) {
>>>> +    if (job_not_paused_nor_cancelled(&s->common.job) && 
>>>> !s->in_drain) {
>>>>           return true;
>>>>       }
>>>>
>>>
>>> Why to introduce a separate API function for every use case?
>>>
>>> Could we instead just use WITH_JOB_LOCK_GUARD() ?
>>>
>>
>> This implies making the struct job_mutex public. Is that ok for you?
>>
> 
> Yes, I think it's OK.
> 
> Alternatively, you can use job_lock() / job_unlock(), or even rewrite 
> WITH_JOB_LOCK_GUARD() macro using job_lock/job_unlock, to keep mutex 
> private.. But I don't think it really worth it now.
> 
> Note that struct Job is already public, so if we'll use per-job mutex in 
> future it still is not a problem. Only when we decide to make struct Job 
> private, we'll have to decide something about JOB_LOCK_GUARD(), and at 
> this point we'll just rewrite it to work through some helper function 
> instead of directly touching the mutex.
> 
> 

Ok I will do that. Just FYI the initial idea was that drivers like 
monitor would not need to know about job_mutex lock, that is why I made 
the helpers in mirror.c.

Thank you,
Emanuele

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.