should CephContext be a singleton?

should CephContext be a singleton?

[Jeff Layton]

I recently hit the problem described in this bug when rolling some new
tests for cephfs:

    http://tracker.ceph.com/issues/20988

While I hit it in a testcase, I suspect something like ganesha or samba
could also hit this, as they can create several CephContexts in the
course of their duties and their teardown is not necessarily coordinated
in any way.

The problem is basically that the CephContext is a random pile of stuff,
some of which has an effect on global objects (the lockdep stuff, in
particular, but there may be more).

There are a few ways to fix it...we could just ensure that the lockdep
stuff is handled cleanly, but I wonder...is there any real-world
use-case for having multiple CephContexts in a single process image?

It contains stuff like admin sockets, and certain threads are tied to
it, etc. This seems like the sort of thing that should just have a
single instance per process. Should we start moving things in that
direction, or should I look at fixing this another way?

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: should CephContext be a singleton?

[Adam C. Emerson]

On 13/09/2017, Jeff Layton wrote:
> It contains stuff like admin sockets, and certain threads are tied to
> it, etc. This seems like the sort of thing that should just have a
> single instance per process. Should we start moving things in that
> direction, or should I look at fixing this another way?

There was a general consensus of removing use of the g_ceph_context
variable. A PR of mine removed it from the object store, OSD and
messenger, though other things needed doing so I haven't got around to
removing the rest.

If you have the cycles for it, feel free to start removing it from
elsewhere and threading CephContext* in.

There isn't really a consensus on splitting out CephContext so it's
less of a God Object but it might be worth reconsidering that at some
point.

Also the lockdep stuff may be less of an issue as we move away from
Mutex to std::mutex. (Though I do have a debugging mutex class that
lets people into our lockdep code if we find that is an issue. I
/suspect/ something like helgrind might be better for finding lock
ordering errors.)

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: should CephContext be a singleton?

[John Spray]

On Wed, Sep 13, 2017 at 4:09 PM, Adam C. Emerson <aemerson@redhat.com> wrote:
> On 13/09/2017, Jeff Layton wrote:
>> It contains stuff like admin sockets, and certain threads are tied to
>> it, etc. This seems like the sort of thing that should just have a
>> single instance per process. Should we start moving things in that
>> direction, or should I look at fixing this another way?
>
> There was a general consensus of removing use of the g_ceph_context
> variable. A PR of mine removed it from the object store, OSD and
> messenger, though other things needed doing so I haven't got around to
> removing the rest.

From going back and reading the "A Transformation of our Global
Context", it seems like the idea was more around separating the truly
global stuff out, rather than getting rid of the global variable as
such?  In that thread we retained a global thing called
ProcessContext.

The separation between process global stuff and per-cluster stuff
still seems entirely sensible, I don't know how much progress was made
with that for the RGW/RBD bits that have cluster-spanning daemons?

> If you have the cycles for it, feel free to start removing it from
> elsewhere and threading CephContext* in.
>
> There isn't really a consensus on splitting out CephContext so it's
> less of a God Object but it might be worth reconsidering that at some
> point.
>
> Also the lockdep stuff may be less of an issue as we move away from
> Mutex to std::mutex. (Though I do have a debugging mutex class that
> lets people into our lockdep code if we find that is an issue. I
> /suspect/ something like helgrind might be better for finding lock
> ordering errors.)

I was wondering about this the other day, does std::mutex give us the
cycle detection some other way?  lockdep is a seriously useful thing.

John

>
> --
> Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
> IRC: Aemerson@{RedHat, OFTC}
> 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: should CephContext be a singleton?

[Adam C. Emerson]

On 13/09/2017, John Spray wrote:
[snip]
> The separation between process global stuff and per-cluster stuff
> still seems entirely sensible, I don't know how much progress was made
> with that for the RGW/RBD bits that have cluster-spanning daemons?

As I understand it, people agreed on getting rid of g_ceph_context but
not on separation, but if people like separation....

But! Either way, the other advantage of getting rid of g_ceph_context
is that it makes embedding potentially multiple daemons in processes
and clears up other librarification issues. (For example, if we ever
wanted to try client-side erasure coding, the g_ceph_context in the
ErasureCode library might be a problem.)

Sage also worried about logging and the problem of marking messages
from multiple OSDs in a single process and making sure they went to
the appropriate files and so on. I had an idea for keeping the
per-process context and logging thread-local and simulating dynamic
scoping on top of it to address the logging problem, but that was
thought to be too complicated and people liked just passing it around
instead.


> I was wondering about this the other day, does std::mutex give us the
> cycle detection some other way?  lockdep is a seriously useful thing.

Well! If you want lockdep, there's common/mutex_debug.h which plugs
into lockdep. You could insert it anywhere you wanted to use lockdep
or have more run-time checking.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: should CephContext be a singleton?

[Jeff Layton]

On Wed, 2017-09-13 at 11:30 -0400, Adam C. Emerson wrote:
> On 13/09/2017, John Spray wrote:
> [snip]
> > The separation between process global stuff and per-cluster stuff
> > still seems entirely sensible, I don't know how much progress was made
> > with that for the RGW/RBD bits that have cluster-spanning daemons?
> 
> As I understand it, people agreed on getting rid of g_ceph_context but
> not on separation, but if people like separation....
> 
> But! Either way, the other advantage of getting rid of g_ceph_context
> is that it makes embedding potentially multiple daemons in processes
> and clears up other librarification issues. (For example, if we ever
> wanted to try client-side erasure coding, the g_ceph_context in the
> ErasureCode library might be a problem.)
> 
> Sage also worried about logging and the problem of marking messages
> from multiple OSDs in a single process and making sure they went to
> the appropriate files and so on. I had an idea for keeping the
> per-process context and logging thread-local and simulating dynamic
> scoping on top of it to address the logging problem, but that was
> thought to be too complicated and people liked just passing it around
> instead.
> 
> 
> > I was wondering about this the other day, does std::mutex give us the
> > cycle detection some other way?  lockdep is a seriously useful thing.
> 
> Well! If you want lockdep, there's common/mutex_debug.h which plugs
> into lockdep. You could insert it anywhere you wanted to use lockdep
> or have more run-time checking.
> 

Fair enough, and thanks for all the input. At this point, I've pretty
much determined that the main problem in that tracker bug is with
lockdep.

I'm going to opt for smaller, more targeted surgery that should keep
things working more or less as they are, but close the races that can
occur when the cct is unregistered from lockdep while other threads are
concurrently manipulating mutexes. That has the advantage of being
relatively easy to backport too, I think.

I should have a PR up in a day or two, once I do some more testing with
the series.

Thanks!
-- 
Jeff Layton <jlayton@redhat.com>

Re: should CephContext be a singleton?

[Jeff Layton]

On Wed, 2017-09-13 at 11:09 -0400, Adam C. Emerson wrote:
> On 13/09/2017, Jeff Layton wrote:
> > It contains stuff like admin sockets, and certain threads are tied to
> > it, etc. This seems like the sort of thing that should just have a
> > single instance per process. Should we start moving things in that
> > direction, or should I look at fixing this another way?
> 
> There was a general consensus of removing use of the g_ceph_context
> variable. A PR of mine removed it from the object store, OSD and
> messenger, though other things needed doing so I haven't got around to
> removing the rest.
> 
> If you have the cycles for it, feel free to start removing it from
> elsewhere and threading CephContext* in.
> 
> There isn't really a consensus on splitting out CephContext so it's
> less of a God Object but it might be worth reconsidering that at some
> point.
> 
> Also the lockdep stuff may be less of an issue as we move away from
> Mutex to std::mutex. (Though I do have a debugging mutex class that
> lets people into our lockdep code if we find that is an issue. I
> /suspect/ something like helgrind might be better for finding lock
> ordering errors.)

That may be, but this is a problem now. I fear something like manila
that is going to be creating/removing ganesha exports, possibly rapidly
and on demand may trip over this.

It's not really g_ceph_context that's the problem. The main problem I
have hit is with g_lockdep_ceph_ctx, which is a global pointer to the
cct that was passed to the initial call to
lockdep_register_ceph_context.

That pointer is manipulated under a mutex, but lockdep_dout doesn't
necessarily hold that mutex when it does to dereference the pointer.

Note too that the reproducer in that bug exhibits other crashes as well,
so even if we get the lockdep problem cleaned up, there may be other
lurking problems.

I'm still wondering though -- what is the use-case for multiple
CephContexts in a single process image? Why would you ever want that? Is
there some need to allow programs to multiplex different clients that
have different config options? What's the argument for not making this a
singleton?
-- 
Jeff Layton <jlayton@redhat.com>

Re: should CephContext be a singleton?

[Sage Weil]

On Wed, 13 Sep 2017, Jeff Layton wrote:
> I recently hit the problem described in this bug when rolling some new
> tests for cephfs:
> 
>     http://tracker.ceph.com/issues/20988
> 
> While I hit it in a testcase, I suspect something like ganesha or samba
> could also hit this, as they can create several CephContexts in the
> course of their duties and their teardown is not necessarily coordinated
> in any way.
> 
> The problem is basically that the CephContext is a random pile of stuff,
> some of which has an effect on global objects (the lockdep stuff, in
> particular, but there may be more).
> 
> There are a few ways to fix it...we could just ensure that the lockdep
> stuff is handled cleanly, but I wonder...is there any real-world
> use-case for having multiple CephContexts in a single process image?
> 
> It contains stuff like admin sockets, and certain threads are tied to
> it, etc. This seems like the sort of thing that should just have a
> single instance per process. Should we start moving things in that
> direction, or should I look at fixing this another way?

There are a couple of reasons why it's not a singleton:

 - It makes library usage awkward.  Having "global" state in a shared lib 
is an antipattern, so librados was set up with create/destroy functions, 
which are built around the cct.  In practice it means you can instantiate 
multiple independent clients in the same process, which is IMO useful.
 - We would eventually like to host multiple OSD daemons within the same 
process in order to multiplex the network IO across the same connections 
(reducing e.g. RDMA connection count) and allowing more efficient use of 
DPDK/SPDK.  Moving from g_ceph_context -> explicit ccts is a step in that 
direction, and further away from any singletons.

sage