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

[Max Reitz <mreitz@redhat.com>]

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.