Re: [PATCH for-6.1? 1/6] mirror: Keep s->synced on error

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

22.07.2021 15:26, Max Reitz wrote:
> An error does not take us out of the READY phase, which is what
> s->synced signifies.  It does of course mean that source and target are
> no longer in sync, but that is what s->actively_sync is for -- s->synced
> never meant that source and target are in sync, only that they were at
> some point (and at that point we transitioned into the READY phase).
> 
> The tangible problem is that we transition to READY once we are in sync
> and s->synced is false.  By resetting s->synced here, we will transition
> from READY to READY once the error is resolved (if the job keeps
> running), and that transition is not allowed.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/mirror.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 98fc66eabf..d73b704473 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -121,7 +121,6 @@ typedef enum MirrorMethod {
>   static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
>                                               int error)
>   {
> -    s->synced = false;
>       s->actively_synced = false;
>       if (read) {
>           return block_job_error_action(&s->common, s->on_source_error,
> 

Looked through.. Yes, seems s->synced used as "is ready". Isn't it better to drop s->synced at all and use job_is_read() instead?

Hmm, s->actively_synced used only for assertion in active_write_settle().. That's not wrong, just interesting.

-- 
Best regards,
Vladimir