| Message ID | 20210722122627.29605-2-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mirror: Handle errors after READY cancel | expand |
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.
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.
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> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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,
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(-)