[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 09/11] block/backup: teach TOP to never copy
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 09/11] block/backup: teach TOP to never copy unallocated regions |
Date: |
Tue, 16 Jul 2019 18:11:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
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 <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".)
Hm, OK. I think it should be an own patch, though.
> 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.
That’s what I had thought. (That canceling would be a success.)
As for COMPLETE, you want it to emit a READY event right when it’s
started? :-)
> (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.)
Hm. Actually, I don’t even care that much. If the user canceled it,
they probably won’t really look at the return code anyway...
Max
>>> } else {
>>> ret = backup_loop(s);
>>> }
>>>
>>> + out:
>>> notifier_with_return_remove(&s->before_write);
>>>
>>> /* wait until pending backup_do_cow() calls have completed */
>>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v2 05/11] iotests/257: test API failures, (continued)
[Qemu-devel] [PATCH v2 03/11] iotests/257: Refactor backup helpers, John Snow, 2019/07/15
[Qemu-devel] [PATCH v2 11/11] iotests/257: test traditional sync modes, John Snow, 2019/07/15
Re: [Qemu-devel] [PATCH v2 00/11] bitmaps: allow bitmaps to be used with full and top, John Snow, 2019/07/17