Re: [PATCH] block-jobs: add final flush

[Hanna Czenczek <hreitz@redhat.com>]

On 01.11.23 20:53, Vladimir Sementsov-Ogievskiy wrote:
> On 31.10.23 17:05, Hanna Czenczek wrote:
>> On 04.10.23 15:56, Vladimir Sementsov-Ogievskiy wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> Actually block job is not completed without the final flush. It's
>>> rather unexpected to have broken target when job was successfully
>>> completed long ago and now we fail to flush or process just
>>> crashed/killed.
>>>
>>> Mirror job already has mirror_flush() for this. So, it's OK.
>>>
>>> Add similar things for other jobs: backup, stream, commit.
>>>
>>> Note, that stream has (documented) different treatment of IGNORE
>>> action: it don't retry the operation, continue execution and report
>>> error at last. We keep it for final flush too.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>
>>> Was: [PATCH v4] block-jobs: flush target at the end of .run()
>>>    But now rewritten.
>>> Supersedes: <20230725174008.1147467-1-vsementsov@yandex-team.ru>
>>>
>>>   block/backup.c             |  2 +-
>>>   block/block-copy.c         |  7 +++++++
>>>   block/commit.c             | 16 ++++++++++++----
>>>   block/stream.c             | 21 +++++++++++++++++----
>>>   include/block/block-copy.h |  1 +
>>>   5 files changed, 38 insertions(+), 9 deletions(-)
>>
>> [...]
>>
>>> diff --git a/block/commit.c b/block/commit.c
>>> index aa45beb0f0..5205c77ec9 100644
>>> --- a/block/commit.c
>>> +++ b/block/commit.c
>>
>> [...]
>>
>>> @@ -187,7 +187,15 @@ static int coroutine_fn commit_run(Job *job, 
>>> Error **errp)
>>>           }
>>>       }
>>> -    return 0;
>>> +    do {
>>> +        ret = blk_co_flush(s->base);
>>> +        if (ret < 0) {
>>> +            action = block_job_error_action(&s->common, s->on_error,
>>> +                                            false, -ret);
>>> +        }
>>> +    } while (ret < 0 && action != BLOCK_ERROR_ACTION_REPORT);
>>
>> Do we need to yield in this loop somewhere so that 
>> BLOCK_ERROR_ACTION_STOP can pause the job?
>>
>
> block_job_error_action calls job_pause_locked() itself in this case

But that doesn’t really pause the job, does it?  As far as I understand, 
it increases job->pause_count, then enters the job, and the job is then 
supposed to yield at some point so job_pause_point_locked() is called, 
which sees the increased job->pause_count and will actually pause the job.

Hanna