Re: [PATCH v2 1/3] block/commit: implement final flush

[Vladimir Sementsov-Ogievskiy]

On 29.07.24 15:25, Kevin Wolf wrote:
> Am 19.07.2024 um 12:35 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > On 18.07.24 22:22, Kevin Wolf wrote:
> > > Am 26.06.2024 um 16:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > 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.
> > > >
> > > > Do this for commit job too.
> > > >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > ---
> > > >    block/commit.c | 41 +++++++++++++++++++++++++++--------------
> > > >    1 file changed, 27 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/block/commit.c b/block/commit.c
> > > > index 7c3fdcb0ca..81971692a2 100644
> > > > --- a/block/commit.c
> > > > +++ b/block/commit.c
> > > > @@ -134,6 +134,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
> > > >        int64_t n = 0; /* bytes */
> > > >        QEMU_AUTO_VFREE void *buf = NULL;
> > > >        int64_t len, base_len;
> > > > +    bool need_final_flush = true;
> > > >        len = blk_co_getlength(s->top);
> > > >        if (len < 0) {
> > > > @@ -155,8 +156,8 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
> > > >        buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
> > > > -    for (offset = 0; offset < len; offset += n) {
> > > > -        bool copy;
> > > > +    for (offset = 0; offset < len || need_final_flush; offset += n) {
> > >
> > > In general, the control flow would be nicer to read if the final flush
> > > weren't integrated into the loop, but just added after it.
> > >
> > > But I assume this is pretty much required for pausing the job during
> > > error handling in the final flush if you don't want to duplicate a lot
> > > of the logic into a second loop?
> >
> > Right, that's the reason.
>
> This would probably be the right solution if it affected only commit.
> But I've thought a bit more about this and given that the same thing
> happens in all of the block jobs, I'm really wondering if introducing a
> block job helper function wouldn't be better, so that each block job
> could just add something like this after its main loop:
>
>      do {
>          ret = blk_co_flush();
>      } while (block_job_handle_error(job, ret));
>
> And the helper would call block_job_error_action(), stop the job if
> necessary, check if it's cancelled, include a pause point etc.

Agree. Thanks, I'll try.

--
Best regards,
Vladimir