Re: [Qemu-block] [Qemu-devel] [PATCH v3] blockjob: drain all job nodes in block_job_drain

[Kevin Wolf]

Am 01.08.2019 um 21:44 hat Max Reitz geschrieben:
> On 30.07.19 21:11, John Snow wrote:
> > 
> > 
> > On 7/24/19 5:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> >> Instead of draining additional nodes in each job code, let's do it in
> >> common block_job_drain, draining just all job's children.
> >> BlockJobDriver.drain becomes unused, so, drop it at all.
> >>
> >> It's also a first step to finally get rid of blockjob->blk.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> >> ---
> >>
> >> v3: just resend, as I've some auto returned mails and not sure that
> >>     v2 reached recipients.
> >>
> >> v2: apply Max's suggestions:
> >>  - drop BlockJobDriver.drain
> >>  - do firtly loop of bdrv_drained_begin and then separate loop
> >>    of bdrv_drained_end.
> >>
> >>    Hmm, a question here: should I call bdrv_drained_end in reverse
> >>    order? Or it's OK as is?
> >>
> > 
> > I think it should be OK. These nodes don't necessarily have a well
> > defined relationship between each other, do they?
> 
> It’s OK.  If they do have a relationship, the drain code sorts it out by
> itself anyway.
> 
> [...]
> 
> > Seems much nicer to me. What becomes of the ref/unref pairs?
> > 
> > I guess not needed anymore?, since job cleanup necessarily happens in
> > the main loop context now and we don't have a backup_complete function
> > anymore ...?
> > 
> > In the cases where auto_finalize=true, do we have any guarantee that the
> > completion callbacks cannot be scheduled while we are here?
> 
> Let me try to figure this out...
> 
> The only caller of block_job_drain() is job_drain(), whose only caller
> is job_finish_sync() in an AIO_WAIT_WHILE() loop.  That loop can only
> work in the main loop, so I suppose it does run in the main loop (and
> consequentially, block_job_drain() runs in the main loop, too).
> 
> That AIO_WAIT_WHILE() loop drains the job (so all nodes) and waits until
> the job has completed.  To me that looks like it is designed to have the
> completion callback run at some point...?

Yes, definitely.

However, I wonder why we do this blk_drain(). We are not really
interested in stopping all requests to the nodes involves in the job, we
just want to get the job to make progress so that it will eventually
complete. Draining looks like the entirely wrong tool to me.

This was introduced in commit bae8196d9f9, and it looks to me as if it
was a hack because drain could work cross-AioContext and everything else
couldn't.

Today we use AIO_WAIT_WHILE() in job_finish_sync(), so I wonder if maybe
the whole drain part is unnecessary now and just doing the job_enter()
part would be enough.

> I suppose anything like that is scheduled by job_enter() in job_drain(),
> namely the aio_co_enter() in there.
> 
> (A) If the job runs in the main AioContext, aio_co_enter() will enter
> the coroutine if we do not run in a coroutine right now (safe), or it
> will schedule it for a later point if we do run in a coroutine.  That
> latter part may be unsafe, because I guess some coroutine work in
> bdrv_drained_begin() or bdrv_drained_end() may wake it up, which can
> then mess everything up.

It should be fine, actually. The drain functions drop out of coroutine
context to avoid such problems. So if it gets woken up, it's before the
actual drain has started.

> (B) If the job runs in another context, aio_co_enter() will schedule the
> job immediately, which I guess means that it can run at any point?  So
> it could complete at any point, including block_job_drain().  Ah, no,
> actually.  AIO_WAIT_WHILE() will have the job’s context acquired while
> evaluating the condition (that is, when calling block_job_drain()).  So
> this is safe.
> 
> 
> So, well.  Unless Vladimir can give you a guarantee why e.g.
> block_job_remove_all_bdrv() won’t run during e.g. bdrv_drained_begin(),
> I suppose you’re right and the node list needs to be copied at the
> beginning of this function and all nodes should be ref’d.

At some point, it will probably run, in the polling phase of
bdrv_drained_begin().

Kevin

signature.asc
Description: PGP signature