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

[PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct

Emanuele Giuseppe Esposito posted 21 patches 3 years, 7 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, John Snow <jsnow@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, 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>
There is a newer version of this series
[PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct
Posted by Emanuele Giuseppe Esposito 3 years, 7 months ago 
iostatus is the only field (together with .job) that needs
protection using the job mutex.

It is set in the main loop (GLOBAL_STATE functions) but read
in I/O code (block_job_error_action).

In order to protect it, change block_job_iostatus_set_err
to block_job_iostatus_set_err_locked(), always called under
job lock.

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

diff --git a/blockjob.c b/blockjob.c
index a2559b97a7..893c8ff08e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -367,7 +367,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
     return block_job_query_locked(job, errp);
 }
 
-static void block_job_iostatus_set_err(BlockJob *job, int error)
+/* Called with job lock held */
+static void block_job_iostatus_set_err_locked(BlockJob *job, int error)
 {
     if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
         job->iostatus = error == ENOSPC ? BLOCK_DEVICE_IO_STATUS_NOSPACE :
@@ -586,8 +587,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
                  */
                 job->job.user_paused = true;
             }
+            block_job_iostatus_set_err_locked(job, error);
         }
-        block_job_iostatus_set_err(job, error);
     }
     return action;
 }
-- 
2.31.1
Re: [PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct
Posted by Vladimir Sementsov-Ogievskiy 3 years, 7 months ago 
On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
> iostatus is the only field (together with .job) that needs
> protection using the job mutex.
> 
> It is set in the main loop (GLOBAL_STATE functions) but read
> in I/O code (block_job_error_action).
> 
> In order to protect it, change block_job_iostatus_set_err
> to block_job_iostatus_set_err_locked(), always called under
> job lock.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Seems, that (and previous) patch should go before 15.

> ---
>   blockjob.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index a2559b97a7..893c8ff08e 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -367,7 +367,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>       return block_job_query_locked(job, errp);
>   }
>   
> -static void block_job_iostatus_set_err(BlockJob *job, int error)
> +/* Called with job lock held */
> +static void block_job_iostatus_set_err_locked(BlockJob *job, int error)
>   {
>       if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>           job->iostatus = error == ENOSPC ? BLOCK_DEVICE_IO_STATUS_NOSPACE :
> @@ -586,8 +587,8 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
>                    */
>                   job->job.user_paused = true;
>               }
> +            block_job_iostatus_set_err_locked(job, error);
>           }
> -        block_job_iostatus_set_err(job, error);
>       }
>       return action;
>   }


-- 
Best regards,
Vladimir
Re: [PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct
Posted by Emanuele Giuseppe Esposito 3 years, 6 months ago 

Am 11/07/2022 um 16:51 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
>> iostatus is the only field (together with .job) that needs
>> protection using the job mutex.
>>
>> It is set in the main loop (GLOBAL_STATE functions) but read
>> in I/O code (block_job_error_action).
>>
>> In order to protect it, change block_job_iostatus_set_err
>> to block_job_iostatus_set_err_locked(), always called under
>> job lock.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Seems, that (and previous) patch should go before 15.

Why?

> 
>> ---
>>   blockjob.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/blockjob.c b/blockjob.c
>> index a2559b97a7..893c8ff08e 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -367,7 +367,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error
>> **errp)
>>       return block_job_query_locked(job, errp);
>>   }
>>   -static void block_job_iostatus_set_err(BlockJob *job, int error)
>> +/* Called with job lock held */
>> +static void block_job_iostatus_set_err_locked(BlockJob *job, int error)
>>   {
>>       if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>>           job->iostatus = error == ENOSPC ?
>> BLOCK_DEVICE_IO_STATUS_NOSPACE :
>> @@ -586,8 +587,8 @@ BlockErrorAction block_job_error_action(BlockJob
>> *job, BlockdevOnError on_err,
>>                    */
>>                   job->job.user_paused = true;
>>               }
>> +            block_job_iostatus_set_err_locked(job, error);
>>           }
>> -        block_job_iostatus_set_err(job, error);
>>       }
>>       return action;
>>   }
> 
>
Re: [PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct
Posted by Vladimir Sementsov-Ogievskiy 3 years, 6 months ago 
On 7/19/22 16:07, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 11/07/2022 um 16:51 schrieb Vladimir Sementsov-Ogievskiy:
>> On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
>>> iostatus is the only field (together with .job) that needs
>>> protection using the job mutex.
>>>
>>> It is set in the main loop (GLOBAL_STATE functions) but read
>>> in I/O code (block_job_error_action).
>>>
>>> In order to protect it, change block_job_iostatus_set_err
>>> to block_job_iostatus_set_err_locked(), always called under
>>> job lock.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>
>> Seems, that (and previous) patch should go before 15.
> 
> Why?

As I understand, before patch 15 everything was protected by aio-context lock. After patch 15, this becomes a bug, which we fix now. If we do it before patch 15, we never introduce the bug.. Or what I miss?

> 
>>
>>> ---
>>>    blockjob.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/blockjob.c b/blockjob.c
>>> index a2559b97a7..893c8ff08e 100644
>>> --- a/blockjob.c
>>> +++ b/blockjob.c
>>> @@ -367,7 +367,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error
>>> **errp)
>>>        return block_job_query_locked(job, errp);
>>>    }
>>>    -static void block_job_iostatus_set_err(BlockJob *job, int error)
>>> +/* Called with job lock held */
>>> +static void block_job_iostatus_set_err_locked(BlockJob *job, int error)
>>>    {
>>>        if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>>>            job->iostatus = error == ENOSPC ?
>>> BLOCK_DEVICE_IO_STATUS_NOSPACE :
>>> @@ -586,8 +587,8 @@ BlockErrorAction block_job_error_action(BlockJob
>>> *job, BlockdevOnError on_err,
>>>                     */
>>>                    job->job.user_paused = true;
>>>                }
>>> +            block_job_iostatus_set_err_locked(job, error);
>>>            }
>>> -        block_job_iostatus_set_err(job, error);
>>>        }
>>>        return action;
>>>    }
>>
>>
> 


-- 
Best regards,
Vladimir
Re: [PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct
Posted by Emanuele Giuseppe Esposito 3 years, 6 months ago 

Am 20/07/2022 um 15:15 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/19/22 16:07, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 11/07/2022 um 16:51 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
>>>> iostatus is the only field (together with .job) that needs
>>>> protection using the job mutex.
>>>>
>>>> It is set in the main loop (GLOBAL_STATE functions) but read
>>>> in I/O code (block_job_error_action).
>>>>
>>>> In order to protect it, change block_job_iostatus_set_err
>>>> to block_job_iostatus_set_err_locked(), always called under
>>>> job lock.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>
>>> Seems, that (and previous) patch should go before 15.
>>
>> Why?
> 
> As I understand, before patch 15 everything was protected by aio-context
> lock. After patch 15, this becomes a bug, which we fix now. If we do it
> before patch 15, we never introduce the bug.. Or what I miss?

Makes sense.

Emanuele

> 
>>
>>>
>>>> ---
>>>>    blockjob.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/blockjob.c b/blockjob.c
>>>> index a2559b97a7..893c8ff08e 100644
>>>> --- a/blockjob.c
>>>> +++ b/blockjob.c
>>>> @@ -367,7 +367,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error
>>>> **errp)
>>>>        return block_job_query_locked(job, errp);
>>>>    }
>>>>    -static void block_job_iostatus_set_err(BlockJob *job, int error)
>>>> +/* Called with job lock held */
>>>> +static void block_job_iostatus_set_err_locked(BlockJob *job, int
>>>> error)
>>>>    {
>>>>        if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>>>>            job->iostatus = error == ENOSPC ?
>>>> BLOCK_DEVICE_IO_STATUS_NOSPACE :
>>>> @@ -586,8 +587,8 @@ BlockErrorAction block_job_error_action(BlockJob
>>>> *job, BlockdevOnError on_err,
>>>>                     */
>>>>                    job->job.user_paused = true;
>>>>                }
>>>> +            block_job_iostatus_set_err_locked(job, error);
>>>>            }
>>>> -        block_job_iostatus_set_err(job, error);
>>>>        }
>>>>        return action;
>>>>    }
>>>
>>>
>>
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.