Re: [PATCH for-6.0? 1/3] job: Add job_wait_unpaused() for block-job-complete

[Kevin Wolf]

Am 09.04.2021 um 11:31 hat Max Reitz geschrieben:
> On 08.04.21 18:55, John Snow wrote:
> > On 4/8/21 12:20 PM, Max Reitz wrote:
> > > +    /* Similarly, if the job is still drained, waiting will not
> > > help either */
> > > +    if (job->pause_count > 0) {
> > > +        error_setg(errp, "Job '%s' is blocked and cannot be
> > > unpaused", job->id);
> > > +        return -EBUSY;
> > > +    }
> > > +
> > 
> > This leaks an internal state detail out to the caller. In which
> > circumstances does this happen?
> 
> Hm.  Now that you ask it.
> 
> The circumstance would be a concurrent drain in some other IO thread.
> Probably the IO thread the job runs in?  I don’t know any other thread that
> could concurrently drain, because this function runs in the main thread, and
> there shouldn’t be any drain in the background.
> 
> If it is another IO thread, waiting would indeed help, so there would not be
> a need to error out.
> 
> Perhaps it’s possible to have a background drain in the main thread?  I
> don’t think so, though...

Hm... Maybe like this:

1. bdrv_do_drained_begin() in the main thread
2. BDRV_POLL_WHILE() reenters the QMP dispatcher coroutine
3. qmp_job_complete()
4. Deadlock because we poll here until the job is undrained

This is why nested event loops are evil...

I guess a way to fix this would be making qmp_job_complete() a coroutine
handler and yielding instead of actively polling. Then job_pause_point()
would have to wake the waiting coroutine rather than calling
aio_wait_kick().

On the other hand, this fix would be a lot more complex for -rc3 than
what you posted here.

So maybe take this one for -rc3 and do the coroutine thing for 6.1?

> > Looks about right to me, but you'll want Kevin's look-see for the finer
> > details, of course.
> > 
> > My concern is that this adds a wait of an indefinite period to the
> > job_complete command. We mitigate this by checking for some other
> > internal state criteria first, and then by process of elimination deduce
> > that it's safe to wait, as it will (likely) be very quick.
> > 
> > Do we open the door for ourselves to get into trouble here, either by a
> > state we are forgetting to rule out (You'd have added it if you know the
> > answer to this) or a hypothetical future change where we forget to
> > update this function?
> 
> Well.  Who knows.
> 
> The alternatives I see are:
> 
> (A) Let drained_end wait for the block jobs to be resumed.  There are some
> details to consider there, I had some discussion around this with Kevin on
> Tuesday.  For example, should every drained_end wait for all jobs involved
> to be resumed?  (That would mean waiting for concurrent drained_ends, too.)
> Would the drained_end_counter be the right tool for the job?  (None of this
> is unsolvable, I guess, but it would mean having another discussion.)

The advantage there would be that you don't have to deal with new nested
event loops.

I think the problem you brought up was that we can't wait for the job
resuming if it is at the same time paused externally. But we could just
check that and not increase the drained_end_counter in this case.

This would probably result in a fix that is simple enough for -rc3.

> It would also mean that you basically just move the wait to wherever the
> drained_end occurs, for example to qmp_transaction().  Now, every
> drained_end is preceded by a drained_begin that always has to wait, so it
> probably isn’t bad.  OTOH, if qmp_transaction() would be allowed to wait for
> a job to be resumed, I think we can allow the same for
> qmp_block_job_complete().
> (And there’s the fact that most of the time not having the block job running
> after drained_end poses no problem.  This is the first time I’m aware of a
> problem, so I think it would be preferable to wait only on the rare occasion
> where we have to.)

Yeah, I can see your point, but on the other hand, waiting in
drained_end until we can be sure that the effect of drained_begin has
been undone is a single place that covers all occasions.

I'm not overly confident that we would catch all occasions where we have
to if we try attacking them one by one. As you noticed, this is not some
simple case where things just fail all the time when you get it wrong,
but results in race conditions that are hard to reproduce.

> (B) Have block-job-complete be kind of asynchronous.  We talked about that
> on IRC yesterday, and the main problem seems to be that we don’t know what
> we’d do with errors.  We could only emit them via an event, or let the whole
> job fail, both of which seem like bad solutions.

I guess my suggestion to make it a coroutine QMP handlers is similar,
except that it blocks QMP, so you could still deliver an error.

On the other hand, this wouldn't be able to address the concern that we
might be waiting for too long (but I'm not convinced that this is a
problem anyway).

> (C) Perhaps mirror’s completion function works just fine when the job is
> paused (we just would have to skip the job_enter()).  I don’t know. Someone™
> would need to find out.

Who knows. Apart from Someone™, of course.

Kevin