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

[Vladimir Sementsov-Ogievskiy]

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.

> > +        bool copy = false;
> >           bool error_in_source = true;
> >   
> >           /* Note that even when no rate limit is applied we need to yield
> > @@ -166,22 +167,34 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
> >           if (job_is_cancelled(&s->common.job)) {
> >               break;
> >           }
> > -        /* Copy if allocated above the base */
> > -        ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
> > -                                        offset, COMMIT_BUFFER_SIZE, &n);
> > -        copy = (ret > 0);
> > -        trace_commit_one_iteration(s, offset, n, ret);
> > -        if (copy) {
> > -            assert(n < SIZE_MAX);
> >   
> > -            ret = blk_co_pread(s->top, offset, n, buf, 0);
> > -            if (ret >= 0) {
> > -                ret = blk_co_pwrite(s->base, offset, n, buf, 0);
> > -                if (ret < 0) {
> > -                    error_in_source = false;
> > +        if (offset < len) {
> > +            /* Copy if allocated above the base */
> > +            ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
> > +                                            offset, COMMIT_BUFFER_SIZE, &n);
> > +            copy = (ret > 0);
> > +            trace_commit_one_iteration(s, offset, n, ret);
> > +            if (copy) {
> > +                assert(n < SIZE_MAX);
> > +
> > +                ret = blk_co_pread(s->top, offset, n, buf, 0);
> > +                if (ret >= 0) {
> > +                    ret = blk_co_pwrite(s->base, offset, n, buf, 0);
> > +                    if (ret < 0) {
> > +                        error_in_source = false;
> > +                    }
> >                   }
> >               }
> > +        } else {
> > +            assert(need_final_flush);
> > +            ret = blk_co_flush(s->base);
> > +            if (ret < 0) {
> > +                error_in_source = false;
> > +            } else {
> > +                need_final_flush = false;
> > +            }
>
> Should we set n = 0 in this block to avoid counting the last chunk twice
> for the progress?

Oops, right. Will fix, thanks

> >           }
> > +
> >           if (ret < 0) {
> >               BlockErrorAction action =
> >                   block_job_error_action(&s->common, s->on_error,
>
> Kevin

--
Best regards,
Vladimir