Re: [Qemu-devel] [PATCH v2 09/11] block/backup: teach TOP to never copy unallocated regions

[John Snow]

On 7/16/19 7:43 AM, Max Reitz wrote:
> On 16.07.19 02:01, John Snow wrote:
>> Presently, If sync=TOP is selected, we mark the entire bitmap as dirty.
>> In the write notifier handler, we dutifully copy out such regions.
>>
>> Fix this in three parts:
>>
>> 1. Mark the bitmap as being initialized before the first yield.
>> 2. After the first yield but before the backup loop, interrogate the
>> allocation status asynchronously and initialize the bitmap.
>> 3. Teach the write notifier to interrogate allocation status if it is
>> invoked during bitmap initialization.
>>
>> As an effect of this patch, the job progress for TOP backups
>> now behaves like this:
>>
>> - total progress starts at bdrv_length.
>> - As allocation status is interrogated, total progress decreases.
>> - As blocks are copied, current progress increases.
>>
>> Taken together, the floor and ceiling move to meet each other.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  block/backup.c     | 78 ++++++++++++++++++++++++++++++++++++++++------
>>  block/trace-events |  1 +
>>  2 files changed, 70 insertions(+), 9 deletions(-)
> 
> Looks good to me but for a seemingly unrelated change:
> 
>> diff --git a/block/backup.c b/block/backup.c
>> index b407d57954..e28fd23f6a 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
> 
> [...]
> 
>> @@ -507,10 +565,12 @@ static int coroutine_fn backup_run(Job *job, Error 
>> **errp)
>>               * notify callback service CoW requests. */
>>              job_yield(job);
>>          }
>> +        ret = -ECANCELED;
> 
> This one.  This doesn’t look like it belongs in this patch, and I’m not
> even sure it’s correct.  Being cancelled is the normal state for
> sync=none, so I suppose it is correct to just return 0 then.
> 
> Max
> 
Yeah, this is wiggly, so... yes, we can return 0 here. The job
infrastructure machinery is going to change it to an ECANCELED for us
anyway:

job_completed
  job_update_rc
    if (!job->ret && job_is_cancelled(job)) {
        job->ret = -ECANCELED;
    }

So in this case I just figured that I might as well make it explicit;
this is an error exit.

(I guess just leaving it at 0 means "whatever the job machinery thinks"
too, which is probably also fine. The job machinery does not distinguish
between "canceled and 0" or "canceled and < 0".)


Since we're here, though... I was wondering if it shouldn't be the case
that "canceling" a sync=none job should actually result in success,
unless you force-cancel. OR, allow sync=none jobs to receive "COMPLETE"
verbs to finish successfully, or "CANCEL" verbs to terminate with error.

(I don't like what mirror does and don't wish to mimic it. I continue to
dislike the idea that canceling a ready mirror job allows it to complete
with a successful error code.)

>>      } else {
>>          ret = backup_loop(s);
>>      }
>>  
>> + out:
>>      notifier_with_return_remove(&s->before_write);
>>  
>>      /* wait until pending backup_do_cow() calls have completed */
>