[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-7.2 1/5] block/mirror: Do not wait for active writes
From: |
Kevin Wolf |
Subject: |
Re: [PATCH for-7.2 1/5] block/mirror: Do not wait for active writes |
Date: |
Thu, 10 Nov 2022 13:04:10 +0100 |
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
- [PATCH for-7.2 0/5] block/mirror: Do not wait for active writes, Hanna Reitz, 2022/11/09
- [PATCH for-7.2 2/5] block/mirror: Drop mirror_wait_for_any_operation(), Hanna Reitz, 2022/11/09
- [PATCH for-7.2 4/5] iotests/151: Test that active mirror progresses, Hanna Reitz, 2022/11/09
- [PATCH for-7.2 1/5] block/mirror: Do not wait for active writes, Hanna Reitz, 2022/11/09
- Re: [PATCH for-7.2 1/5] block/mirror: Do not wait for active writes,
Kevin Wolf <=
- [PATCH for-7.2 5/5] iotests/151: Test active requests on mirror start, Hanna Reitz, 2022/11/09
- [PATCH for-7.2 3/5] block/mirror: Fix NULL s->job in active writes, Hanna Reitz, 2022/11/09