Ceph » CephFS

Correct. I'll see if I can roll up a testcase for this when I get a few mins.

I found a workaround. We can create single CephContext for multiple ceph_mount.

#define _FILE_OFFSET_BITS 64
#include <pthread.h>
#include <features.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <time.h>
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <assert.h>
#include <random>
#include <cephfs/libcephfs.h>

static struct ceph_mount_info *init_cmount = NULL;

void *foo_func(void *x_void_ptr)
{
        CephContext *cct = ceph_get_mount_context(init_cmount);

        unsigned n = 0;
        while (true) {
                struct ceph_mount_info *cmount1 = NULL;

                ceph_create_with_context(&cmount1, cct);
                ceph_conf_read_file(cmount1, "/etc/ceph/ceph.conf");
                ceph_conf_parse_env(cmount1, NULL);
                assert(ceph_mount(cmount1, NULL) == 0);

                ceph_shutdown(cmount1);
                if (++n % 100 == 0)
                        printf("%ld: %d\n", (long)x_void_ptr, n);
        }
        return NULL;
}

define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))

int main()
{
        ceph_create(&init_cmount, "admin");

        pthread_t threads[128];
        for (int i = 0; i < ARRAY_SIZE(threads); i++) {
                if(pthread_create(&threads[i], NULL, foo_func, (void*)(long)i)) {
                        fprintf(stderr, "Error creating thread\n");
                        return 1;
                }
        }
        for (int i = 0; i < ARRAY_SIZE(threads); i++) {
                pthread_join(threads[i], NULL);
        }
        return 0;
}

Hmm. I'm not sure that really helps. Here's the doc comment over ceph_create_with_context:

/**                                                                             
 * Create a mount handle from a CephContext, which holds the configuration      
 * for the ceph cluster.  A CephContext can be acquired from an existing ceph_mount_info
 * handle, using the @ref ceph_get_mount_context call.  Note that using the same CephContext
 * for two different mount handles results in the same client entity id being used.
 *                                                                              
 * @param cmount the mount info handle to initialize                            
 * @param conf reuse this pre-existing CephContext config                       
 * @returns 0 on success, negative error code on failure                        
 */                                                                             
int ceph_create_with_context(struct ceph_mount_info **cmount, struct CephContext *conf);

In the situation where I noticed this, I really did not want the same client entity ID being used, so this workaround would not have been helpful.

This might however be sufficient for ganesha. We could create a context early on and use the same context for all the mounts.

cc'ing Matt on this bug, as it may have implications for the new code that can fetch config info out of RADOS:

Basically, we need to ensure that when we're dipping into ceph libraries that we only ever create a single CephContext, as object is tied to lifetime of some info that is global -- lockdep, in particular, but also potentially other things.

I think we may need come up with a way to keep around a refcounted CephContext pointer that is populated when we first need it and torn down when the last user goes away.

We could also fix this less invasively too -- try to make g_lockdep_ceph_ctx a refcounted object pointer, and then fix up whatever other similar bugs we fine.

But...the more I look at this code, the more I think that we ought to just make the CephContext a singleton. Is there ever any reason why a single process image would need more than one CephContext?

We can also opt for something less invasive. We could just fix up common_preinit to keep a refcounted pointer, and then fix ceph_mount_info destructor and the rados equivalent to just put the reference to it.

I started a discussion on ceph-devel and I think the consensus is that we can't make CephContext a singleton.

I went in and disabled lockdep and was able to run the reproducer in a loop for several minutes. I did hit one problem of hitting the open file limit, but once that was resolved it worked. So, that seems to indicate that lockdep is the main problem here.

Looking at that code though, it's heavily reliant on global variables. I'm not sure how to move that to per-ctx structures. More importantly, would doing that be sufficient anyway? It's may be possible to concoct a scenario that involves mutexes under two different CephContexts.

What we may have to settle for is making lockdep's structures a singleton somehow.

Ok, I finally settled on just keeping things more or less limping along like they are now with lockdep, and just ensuring that we can handle it sanely when lockdep's cct is unregistered while other threads are flogging locks. So far, this passes my simple testcase for this:

https://github.com/ceph/ceph/pull/17738

Note that I think we do need to convert over programs like ganesha and samba to only keep a single CephContext and share it amongst all mounts/clients/etc. Anything else seems pretty wasteful, as you need extra threads, sockets, etc for each CephContext.

What resources are actually duplicated across a CephContext, which we don't want duplicated? When I think of duplicated resources the top of my list includes the messenger and stuff like that, but right now we don't want to share them if they're using different cephx entities...

Greg Farnum wrote:

What resources are actually duplicated across a CephContext, which we don't want duplicated? When I think of duplicated resources the top of my list includes the messenger and stuff like that, but right now we don't want to share them if they're using different cephx entities...

CephContext encompasses a lot of stuff, and I'm not very familiar with all of it. Seems like maybe some of the observers would be shareable? IDK...

The big issue with this turns out to be with the lockdep code, which was just never designed for a world where you have multiple CephContexts. For now, the patches I have should at least keep the existing code limping along, but we may need to re-think how that should work with multiple cct's eventually if that's the sort of design we're aiming for.

the PR has been merged.

but i am not sure if we should backport it. i am pinging Jeff for his insights on it at https://github.com/ceph/ceph/pull/17738. we should mark this ticket "resolved" or "pending backport" once it's confirmed.

To be clear, I think we may want to leave off the patch that adds the new testcase from this series as it's uncovering some other races (see tracker #21512). Until we shake those out, let's not add the new testcase to the stable releases just yet (not sure if that's routinely done or not).

Jeff Layton wrote:

To be clear, I think we may want to leave off the patch that adds the new testcase from this series as it's uncovering some other races (see tracker #21512). Until we shake those out, let's not add the new testcase to the stable releases just yet (not sure if that's routinely done or not).

I just noticed #20998-24 (quoted above) where Jeff warns about the possible problem encountered with the test case.

Unfortunately, the test case has already been merged to luminous, so I'll open a PR to revert.

Luminous test revert PR: https://github.com/ceph/ceph/pull/21251