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

[Kevin Wolf <kwolf@redhat.com>]

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?

> +        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?

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

Kevin