qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/5] blockjob: do not allow corouti


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/5] blockjob: do not allow coroutine double entry or entry-after-completion
Date: Tue, 21 Nov 2017 10:47:11 +0000
User-agent: Mutt/1.9.1 (2017-09-22)

On Mon, Nov 20, 2017 at 08:36:19AM -0500, Jeff Cody wrote:
> On Mon, Nov 20, 2017 at 11:16:53AM +0000, Stefan Hajnoczi wrote:
> > On Sun, Nov 19, 2017 at 09:46:42PM -0500, Jeff Cody wrote:
> > > --- a/blockjob.c
> > > +++ b/blockjob.c
> > > @@ -291,10 +291,10 @@ void block_job_start(BlockJob *job)
> > >  {
> > >      assert(job && !block_job_started(job) && job->paused &&
> > >             job->driver && job->driver->start);
> > > -    job->co = qemu_coroutine_create(block_job_co_entry, job);
> > >      job->pause_count--;
> > >      job->busy = true;
> > >      job->paused = false;
> > > +    job->co = qemu_coroutine_create(block_job_co_entry, job);
> > >      bdrv_coroutine_enter(blk_bs(job->blk), job->co);
> > >  }
> > >  
> > 
> > This hunk makes no difference.  The coroutine is only entered by
> > bdrv_coroutine_enter() so the order of job field initialization doesn't
> > matter.
> > 
> 
> It likely makes no difference with the current code (unless there is a
> latent bug). However I made the change to protect against the following
> scenario - which, perhaps to your point, would be a bug in any case:
> 
> 1. job->co = qemu_coroutine_create()
> 
>     * Now block_job_started() returns true, as it just checks for job->co
> 
> 2. Another thread calls block_job_enter(), before we call
>    bdrv_coroutine_enter().

The job is protected by AioContext acquire/release.  Other threads
cannot touch it because the block_job_start() caller has already
acquired the AioContext.

> 
>     * block_job_enter() checks job->busy and block_job_started() to
>       determine if coroutine entry is allowed.  Without this change, these
>       checks could pass and coroutine entry could occur.
> 
>     * I don't think this can happen in the current code, but the above hunk
>       change is still correct, and would protect against such an
>       occurrence.
>
> I guess the question is, "is it worth doing?", to try and prevent that sort
> of buggy behavior. My thought was "yes" because:
> 
>     A) there is no penalty in doing it this way
> 
>     B) while a bug, double entry like this can lead to memory and/or
>     data corruption, and the checks for co->caller et al. might not
>     catch it.  This is particularly true if the coroutine exits
>     (COROUTINE_TERMINATE) before the re-entry.
> 
> But maybe if we are concerned about that we should figure out a way to
> abort() instead.  Of course, that makes allowing recursive coroutines more
> difficult in the future.

The compiler and CPU can reorder memory accesses so simply reordering
assignment statements is ineffective against threads.

I'm against merging this hunk because:

1. There is a proper thread-safety mechanism in place that callers are
   already using, so this is the wrong way to attempt to provide
   thread-safety.

2. This change doesn't protect against the multi-threaded scenario you
   described because the memory order isn't being controlled.

> 
> 
> > > @@ -797,11 +797,14 @@ void block_job_sleep_ns(BlockJob *job, 
> > > QEMUClockType type, int64_t ns)
> > >          return;
> > >      }
> > >  
> > > -    job->busy = false;
> > > +    /* We need to leave job->busy set here, because when we have
> > > +     * put a coroutine to 'sleep', we have scheduled it to run in
> > > +     * the future.  We cannot enter that same coroutine again before
> > > +     * it wakes and runs, otherwise we risk double-entry or entry after
> > > +     * completion. */
> > >      if (!block_job_should_pause(job)) {
> > >          co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns);
> > >      }
> > > -    job->busy = true;
> > >  
> > >      block_job_pause_point(job);
> > 
> > This leaves a stale doc comment in include/block/blockjob_int.h:
> > 
> >   /**
> >    * block_job_sleep_ns:
> >    * @job: The job that calls the function.
> >    * @clock: The clock to sleep on.
> >    * @ns: How many nanoseconds to stop for.
> >    *
> >    * Put the job to sleep (assuming that it wasn't canceled) for @ns
> >    * nanoseconds.  Canceling the job will interrupt the wait immediately.
> >                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    */
> 
> I didn't catch the doc, that should be changed as well.
> 
> >   void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
> > 
> > This raises questions about the ability to cancel sleep:
> > 
> > 1. Does something depend on cancelling sleep?
> > 
> 
> Not that I can tell.  The advantage is that you don't have to wait for the
> timer, so something like qmp_block_job_cancel() will cancel sooner.
> 
> But it is obviously broken with the current coroutine implementation to try
> to do that.
> 
> > 2. Did cancellation work properly in commit
> >    4513eafe928ff47486f4167c28d364c72b5ff7e3 ("block: add
> >    block_job_sleep_ns") and was it broken afterwards?
> > 
> 
> With iothreads, the answer is complicated.  It was broken for a while for
> other reasons.
> 
> It broke after using aio_co_wake() in the sleep timer cb (commmit
> 2f47da5f7f), which added the ability to schedule a coroutine if the timer
> callback was called from the wrong AioContext.
> 
> Prior to that it "worked" in that the segfault was not present.
> 
> But even to bisect back to 2f47da5f7f was not straightforward, because
> attempting them stream/cancel with iothreads would not even work until
> c324fd0 (so I only bisected back as far as c324fd0 would cleanly apply).
> 
> And it is tricky to say if it "works" or not, because it is racy.  What may
> have appeared to work may be more attributed to luck and timing.
> 
> If the coroutine is going to run at a future time, we cannot enter it
> beforehand.  We risk the coroutine not even existing when the timer does run
> the sleeping coroutine.  At the very least, early entry with the current
> code would require a way to delete the associated timer.
> 
> > It is possible to fix the recursive coroutine entry without losing sleep
> > cancellation.  Whether it's worth the trouble depends on the answers to
> > the above questions.
> > 
> 
> I contemplated the same thing.
> 
> At least for 2.11, fixing recursive coroutine entry is probably more than we
> want to do.
> 
> Long term, my opinion is that we should fix it, because preventing it
> becomes more difficult. It is easy to miss something that might cause a
> recursive entry in code reviews, and since it can be racy, casual testing
> may often miss it as well.

I think both your and Paolos answers show that we don't need to cancel
the timer.  It's okay if the coroutine sleeps for the full duration.
I'm happy with your approach.

Stefan

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]