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

[Max Reitz]

On 22.07.21 18:25, Vladimir Sementsov-Ogievskiy wrote:
> 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?

Sounds good, though I think for the change to be clear, I’d like to keep 
this patch and then drop s->synced on top.

Max

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