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

[Hanna Czenczek]

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