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 <js...@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. > 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