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

[John Snow <jsnow@redhat.com>]

On 7/16/19 12:11 PM, Max Reitz wrote:
> On 16.07.19 18:02, John Snow wrote:
>>
>>
>> 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 <jsnow@redhat.com>
>>>> ---
>>>>  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".)
> 
> Hm, OK.  I think it should be an own patch, though.
> 

Path of least edits.

I'm removing this one change (since it doesn't change anything anyway),
making the error message capitalization fix in the reference output, and
staging the series.

--js