[sheepdog] [PATCH v2 1/3] sheep: make {register, unregister}_event thread safe

Liu Yuan namei.unix at gmail.com
Tue Dec 17 08:11:30 CET 2013


On Tue, Dec 17, 2013 at 03:58:07PM +0900, Hitoshi Mitake wrote:
> At Tue, 17 Dec 2013 14:50:19 +0800,
> Liu Yuan wrote:
> > 
> > On Tue, Dec 17, 2013 at 03:42:29PM +0900, Hitoshi Mitake wrote:
> > > At Tue, 17 Dec 2013 14:31:56 +0800,
> > > Liu Yuan wrote:
> > > > 
> > > > This allow us to call even handling functions in worker thread
> > > > 
> > > > Signed-off-by: Liu Yuan <namei.unix at gmail.com>
> > > > ---
> > > 
> > > I think this change is dangerous. This permits worker threads to
> > > unregister events even if these events are processed in the main
> > > thread. Making a new work queue and delegate it to register/unregister
> > > events would be safer.
> > 
> > This scheme is not pratical for async request.
> > 
> > This is just internal API, which are supported to be called by programmers
> > and check it is correct.
> 
> The checking will cost us lots of time. The problem is a race
> condition caused by multiple threads.

Why we would face this kind of problem? we can make sure at any time, there will
be a single thread manipulate it, be it worker or main thread.

I think we are talking about different issues, you are supposed that events
handling will be generically thread-safe for multiple threads. But this wouldn't
happen I think. Instead, assumption 'one event will be only be manipulated by
single entity (thus no multiple threads case)' will hold true in the long run.

>
> From the perspective of
> maintenance and debugging, I cannot agree with this patch. Adding a
> new workqueue for register/unregister in the main loop will save lots
> of our time in the future.

I think this approach will not work for async request handling, e.g,

+       uatomic_inc(&iocb->count);
+       req->iocb = iocb;
+       register_event(req->local_req_efd, local_req_async_handler, req);
+       submit_local_request(req);

I have to register the event before sumit_local_request().

If I don't and put registeration in a delegated thread, suppose that is called
after request is already handled. So I'll miss the notification that the request
is done and local_req_async_handler will never be called.

How do we handle above problem if we use your approach?

If we take performance into account, we can't do it in your approch either.
Suppose we will issue 1000 requests in a async mode, we can't affort to be trapped
to main thread to register event for every request.

Thanks
Yuan



More information about the sheepdog mailing list