Re: [PATCH for-7.2 1/5] block/mirror: Do not wait for active writes

[Kevin Wolf <kwolf@redhat.com>]

Am 09.11.2022 um 17:54 hat Hanna Reitz geschrieben:
> Waiting for all active writes to settle before daring to create a
> background copying operation means that we will never do background
> operations while the guest does anything (in write-blocking mode), and
> therefore cannot converge.  Yes, we also will not diverge, but actually
> converging would be even nicer.
> 
> It is unclear why we did decide to wait for all active writes to settle
> before creating a background operation, but it just does not seem
> necessary.  Active writes will put themselves into the in_flight bitmap
> and thus properly block actually conflicting background requests.
> 
> It is important for active requests to wait on overlapping background
> requests, which we do in active_write_prepare().  However, so far it was
> not documented why it is important.  Add such documentation now, and
> also to the other call of mirror_wait_on_conflicts(), so that it becomes
> more clear why and when requests need to actively wait for other
> requests to settle.
> 
> Another thing to note is that of course we need to ensure that there are
> no active requests when the job completes, but that is done by virtue of
> the BDS being drained anyway, so there cannot be any active requests at
> that point.
> 
> With this change, we will need to explicitly keep track of how many
> bytes are in flight in active requests so that
> job_progress_set_remaining() in mirror_run() can set the correct number
> of remaining bytes.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2123297
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

> @@ -1327,6 +1336,7 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod method,
>          abort();
>      }
>  
> +    job->active_write_bytes_in_flight -= bytes;
>      if (ret >= 0) {
>          job_progress_update(&job->common.job, bytes);
>      } else {

Preexisting, but it happens to be in the context: Don't we need to call
job_progress_update() unconditionally? Otherwise, the total length will
go backwards in the case of an error.

Kevin