mds: manage Message lifetime with intrusive_ptr

[batrick]

This change turned out to be far more extensive than I hoped but the end result
should prevent all Message-related memory leaks. I believe I fixed several
incidentally.

Fixes: http://tracker.ceph.com/issues/24306

This is a WIP.

[batrick]

Note: this doens't compile yet. Most of what's left to fix is to adjust the small amounts of code that tries to modify a dispatched message. e.g.:

/home/pdonnell/ceph/src/mds/Locker.cc: In member function ‘void Locker::handle_client_caps(const const_ref&)’:
/home/pdonnell/ceph/src/mds/Locker.cc:2699:22: error: passing ‘const MClientCaps’ as ‘this’ argument discards qualifiers [-fpermissive]
       m->clear_dirty();
                      ^
In file included from /home/pdonnell/ceph/src/mds/CInode.h:43:0,
                 from /home/pdonnell/ceph/src/mds/SessionMap.h:34,
                 from /home/pdonnell/ceph/src/mds/MDSRank.h:31,
                 from /home/pdonnell/ceph/src/mds/Locker.cc:17:
/home/pdonnell/ceph/src/messages/MClientCaps.h:128:8: note:   in call to ‘void MClientCaps::clear_dirty()’
   void clear_dirty() { head.dirty = 0; }
        ^~~~~~~~~~~

[ukernel]

I haven't read the code carefully. But I think there is a simple way:

Let ms_dispatch() call message->put().
If any function want to delay handling a message, increase the message's reference count and put the message to the wait list.

[batrick]

Let ms_dispatch() call message->put().
If any function want to delay handling a message, increase the message's reference count and put the message to the wait list.

That indeed would be simpler however this patch is trying to eliminate the manual reference management which is so prone to leaks.

[cbodley]

great improvements to the messenger interfaces. would love to see these intrusive_ptr overloads replace the raw pointer ones eventually

[cbodley]

maybe just duplicate the existing MessageRef typedef here, so you can still use the forward declaration of Message? for that reason, i prefer MessageRef to the new nested Message::ref/const_ref

[batrick]

Moved MessageRef to MessageRef.h. What do you think now @cbodley ?

[cbodley]

that works! in the seastar messenger, i added a net/Fwd.h with forward declarations for all of the messenger types. so i could see this living in a more general header, but it's up to you

[batrick]

@cbodley @liewegas @tchaikov @ukernel please take a look. I think this is done.

[ukernel]

Commit "msg: add factory method to correctly construct smart_ptr" add similar codes to all message types. why not make Message's operator new private and define a template factory function. Something like

struct A {
private:
        void * operator new(size_t size);
        template <class M, class... Args>
        friend std::shared_ptr<M> alloc_msg(Args&&... args);
public: 
};

template <class M, class... Args>
std::shared_ptr<M> alloc_msg(Args&&... args) {
        return std::make_shared<M>(std::forward<Args>(args)...);
}

struct B : public A {
};

int main(int argc, char *argv[])
{
        // auto b1 = new B;
        auto b2 = alloc_msg < B > ();
}

[tchaikov]

#include <iostream>
#include <memory>
#include <boost/smart_ptr/intrusive_ref_counter.hpp>
#include <boost/smart_ptr/intrusive_ptr.hpp>

using namespace std;

template <class A>
class Creater {
public:
  using ref = boost::intrusive_ptr<A>;
  template<typename... Args>
  static ref create(Args&&... args) {
    return ref{new A{std::forward<Args>(args)...}};
  }
};

class Message : public boost::intrusive_ref_counter<Message>
{
public:
  virtual ~Message() = default;
};

class MOSDMap : public Message, public Creater<MOSDMap> {
public:
  MOSDMap(int i) {
    cout << "hello world" << endl;
  }
  ~MOSDMap() {
    cout << "goodbye, cruel world" << endl;
  }
  friend class Creater<MOSDMap>;
};

int main(int argc, char *argv[])
{
  auto p = MOSDMap::create(1);
}

i'd suggest use the CRTP pattern here. so we can have the ref alias also.

[batrick]

http://pulpito.ceph.com/?branch=wip-pdonnell-testing-20180815.203711