Ceph » Linux kernel client

The ceph_osd_client in question is osdc @ 0xffff8802244a4950

The ceph_osd is osd @ 0xffff88020a285000
(struct rb_root *) &osdc->osds.rb_node is ffff8802218a1018 -> {
long unsigned int __rb_parent_color = 0000000000000001
struct rb_node * rb_right = ffff880063513018 ->
struct rb_node * rb_left = ffff880209db4818
}

(struct rb_node *) &osd->node is 0xffff88020a285018 = {
long unsigned int __rb_parent_color = ffff880209db2819
struct rb_node * rb_right = 0000000000000000
struct rb_node * rb_left = 0000000000000000
}

The osd getting removed has empty requests and linger requests
lists, and otherwise looks completely fine (though it does have
two references).

struct ceph_osd @ 0xffff88020a285000 -> {
atomic_t o_ref = { 00000002 }
(pad) = 00000000
struct ceph_osd_client * o_osdc = ffff8802244a4950

int                        o_osd = 00000005
    int                        o_incarnation = 00000001
    struct rb_node             o_node = {
            parent_color = ffff880209db2819
        right = 0000000000000000
        left = 0000000000000000
    }
    struct ceph_connection     o_con = {
        ffff88020a285000 ffffffffa00cf1c0
            ...
        0000000000000000
        0000000000000032
    }
    struct list_head           o_requests =  ffff88020a285538 = {
        next = ffff88020a285538 
        prev = ffff88020a285538 
    }
    struct list_head           o_linger_requests  ffff88020a285548 = {
        next = ffff88020a285548 
        prev = ffff88020a285548 
    }
    struct list_head           o_osd_lru  ffff88020a285558 = {
        next = ffff88020a285558 
        prev = ffff88020a285558 
    }
    struct ceph_auth_handshake o_auth =  ffff88020a285568 = {
        struct ceph_authorizer *   authorizer = 0
        void *                     authorizer_buf = 0
        size_t                     authorizer_buf_len = 0
        void *                     authorizer_reply_buf = 0
        size_t                     authorizer_reply_buf_len = 0
    }
    long unsigned int          lru_ttl = 00000000
    int                        o_marked_for_keepalive = 00000000
    struct list_head           o_keepalive_item @ ffff88020a285590 = {
        next = ffff88020a285590 
        prev = ffff88020a285590 
    }
};

I think I may have just found something.

It appears as though access to the osd client's osd red-black
tree is supposed to be protected using osdc->request_mutex.

An osd is added to that tree only in __insert_osd(). That
function is only called by __map_request(), which does no
additional locking before calling that function. There
are three places __map_request() is called: kick_requests()
(twice); and ceph_osdc_start_request().

In ceph_osdc_start_request(), osdc->request_mutex is held during
the __map_request() call. In kick_requests(), osdc->request_mutex
is taken within that function before calling __map_request()

So all insertions are protected by the request mutex.

An osd is removed from an osd client's tree only in __remove_osd().
There are three callers of __remove_osd(): remove_all_osds();
remove_old_osds(); and __reset_osd(). The first two of these
take and hold osdc->request_mutex while __remove_osd() is called.

But _reset_osd() does not. That function is called in two places,
_kick_osd_requests() and reset_changed_osds(). The first one
takes osdc->request_mutex before calling __remove_osd().
But reset_changed_osds() does not take the mutex. And the
one place reset_changed_osds() is called drops the mutex before
it makes the call (in kick_requests()).

So...

I think what happened is that kick_requests() got called, ultimately
changing the osd client's red-black tree, concurrent with another
call (in this case a call to __remove_osd() but there are other
possible failure modes), and that ultimately led to the corruption
in the osd client's osd red-black tree.

The problem can be seen in this commit:
e6d50f67 libceph: always reset osds when kicking
In that, the call to reset_changed_osds() was moved inside
kick_requests() rather than being called after it.

However even there, the kick_requests call was only protected
by osdc->map_sem (for writing), and that situation seems to
go back to antiquity (to when net/ceph/osd_client.c first got
added to the tree).

One last point. It's possible that osdc->map_sem was intended
to protect updates to the red-black tree. However that's wrong
too, because there appear to be cases where the semaphore is
taken for reading when in fact the tree could get updated.

I think the simplest fix is to move the call in kick_requests()
to reset_changed_osds() up a few lines, before the request
mutex is dropped. This would follow a call to
ceph_monc_request_next_osdmap(), but I think that's fine because
that appears to be an asynchronous request to the monitor and
shouldn't impact the osd client.