client: fix fuse client hang because its pipe to mds is not ok #24172

yunfeiguan · 2018-09-19T07:17:49Z

There is a risk client will hang if fuse client session had been killed by mds and
the mds daemon restart or hot-standby switch happens right away but the client
did not receive any message from monitor due to network or other whatever reason
untill the mds become active again.Thus cause client didn't do closed_mds_session
lead the seession still is STATE_OPEN but client can't send any message to
mds because its pipe is not ok.

So we should create pipe to mds guarantee any meta request can be sent to
server.When mds recevie the message will send a CLOSE_SESSION to client
becasue its session for this client is STATE_CLOSED.After the previous
steps the old session of client can be closed and new session and pipe
can be established and the mountpoint will be ok.

Fixes:http://tracker.ceph.com/issues/36079
Signed-off-by:Guan yunfei yunfei.guan@xtaotech.com

tchaikov · 2018-09-19T08:06:44Z

src/mds/Server.cc

@@ -1777,6 +1777,11 @@ void Server::handle_client_request(const MClientRequest::const_ref &req)
 	       session->is_closing() ||
 	       session->is_killing()) {
      dout(5) << "session closed|closing|killing, dropping" << dendl;
+      if (session->is_closed()) {
+	dout(1) << "send SESSION_CLOSE to client,so that it can reopen session" << dendl;
+	mds->send_message_client(new MClientSession(CEPH_SESSION_CLOSE), session);


should use MClientSession::create() instead.

tchaikov · 2018-09-19T10:12:10Z

159/159 Test   #3: test_objectstore_memstore.sh ............***Timeout 3600.05 sec
...
[ RUN      ] ObjectStore/StoreTest.SimpleListTest/0
2018-09-19 08:41:39.814 7f92ca812a00  1 memstore(memstore.test_temp_dir) mkfs new fsid 87a21399-ec81-40Errors while running CTest
Build step 'Execute shell' marked build as failure

jenkins, retest this please

batrick · 2018-09-24T22:03:49Z

src/msg/simple/SimpleMessenger.cc

@@ -708,7 +708,7 @@ void SimpleMessenger::mark_down(Connection *con)
  lock.Lock();
  Pipe *p = static_cast<Pipe *>(static_cast<PipeConnection*>(con)->get_pipe());
  if (p) {
-    ldout(cct,1) << "mark_down " << con << " -- " << p << dendl;
+    ldout(cct,0) << "mark_down " << con << " -- " << p << dendl;


Please undo this change

batrick · 2018-09-24T22:04:00Z

src/mds/Server.cc

@@ -1777,6 +1777,11 @@ void Server::handle_client_request(const MClientRequest::const_ref &req)
 	       session->is_closing() ||
 	       session->is_killing()) {
      dout(5) << "session closed|closing|killing, dropping" << dendl;
+      if (session->is_closed()) {
+	dout(1) << "send SESSION_CLOSE to client,so that it can reopen session" << dendl;


indentation is wrong here

batrick · 2018-09-24T22:05:13Z

src/client/Client.cc

@@ -2668,6 +2668,10 @@ void Client::handle_mds_map(MMDSMap* m)
    } else if (mdsmap->get_addrs(mds) != session->addrs) {
      session->con->mark_down();
      session->addrs = mdsmap->get_addrs(mds);
+      if (mds_sessions.count(p->first)) {


Please use the mds variable instead of p->first.

batrick · 2018-09-24T22:24:35Z

src/client/Client.cc

@@ -2668,6 +2668,10 @@ void Client::handle_mds_map(MMDSMap* m)
    } else if (mdsmap->get_addrs(mds) != session->addrs) {
      session->con->mark_down();
      session->addrs = mdsmap->get_addrs(mds);
+      if (mds_sessions.count(p->first)) {
+	   ldout(cct, 1) << "handle_mds_map update connection " << dendl;
+	   session->con = messenger->connect_to_mds(session->addrs);


I'm not sure this is the right approach. We don't necessarily want clients reconnecting to the MDS before it reaches STATE_RECONNECT.

I think a better option would be for the client to set the session to stale in the else if (newstate >= MDSMap::STATE_ACTIVE) block below IF the session is down (because we marked it down when the addresses changed) AND conf->client_reconnect_stale. Same logic as in Client::ms_handle_remote_reset.

IOW, if the client knows that its session is stale, it shouldn't try to blindly reconnect.

The client really don't need reconnect to MDS before it reaches STATE_RECONNECT. But we may miss this state because network is not ok. The root cause is we only marked down Pipe when the addresses changed,the Session is still STATE_OPEN. So we need update connection so that the session can be closed in later logic.

The client really don't need reconnect to MDS before it reaches STATE_RECONNECT. But we may miss this state because network is not ok.

We can detect this situation though. Try this:

diff --git a/src/client/Client.cc b/src/client/Client.cc index 82577eff64f..98608349572 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -2663,9 +2663,7 @@ void Client::handle_mds_map(MMDSMap* m) int oldstate = oldmap->get_state(mds); int newstate = mdsmap->get_state(mds); - if (!mdsmap->is_up(mds)) { - session->con->mark_down(); - } else if (mdsmap->get_addrs(mds) != session->addrs) { + if (!mdsmap->is_up(mds) || mdsmap->get_addrs(mds) != session->addrs) { session->con->mark_down(); session->addrs = mdsmap->get_addrs(mds); // When new MDS starts to take over, notify kernel to trim unused entries @@ -2674,10 +2672,21 @@ void Client::handle_mds_map(MMDSMap* m) trim_cache_for_reconnect(session); } else if (oldstate == newstate) continue; // no change - + + /* If we get here, the MDS has changed state or (!) another MDS has taken + * over. (Keep in mind that it's possible (oldstate == newstate == + * STATE_ACTIVE) but the addr has changed! This happens when the client + * misses epochs. + */ + session->mds_state = newstate; - if (newstate == MDSMap::STATE_RECONNECT) { + if (newstate >= MDSMap::STATE_RECONNECT) { session->con = messenger->connect_to_mds(session->addrs); + /* If we missed reconnect, the MDS will reset the session and we'll only + * reconnect if client_reconnect_stale==true. + */ + } + if (newstate == MDSMap::STATE_RECONNECT) { send_reconnect(session); } else if (newstate >= MDSMap::STATE_ACTIVE) { if (oldstate < MDSMap::STATE_ACTIVE) {

batrick · 2018-10-04T19:39:51Z

src/client/Client.cc

@@ -2668,6 +2668,10 @@ void Client::handle_mds_map(MMDSMap* m)
    } else if (mdsmap->get_addrs(mds) != session->addrs) {
      session->con->mark_down();
      session->addrs = mdsmap->get_addrs(mds);
+      if (mds_sessions.count(p->first)) {
+	   ldout(cct, 1) << "handle_mds_map update connection " << dendl;
+	   session->con = messenger->connect_to_mds(session->addrs);


The client really don't need reconnect to MDS before it reaches STATE_RECONNECT. But we may miss this state because network is not ok.

We can detect this situation though. Try this:

diff --git a/src/client/Client.cc b/src/client/Client.cc index 82577eff64f..98608349572 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -2663,9 +2663,7 @@ void Client::handle_mds_map(MMDSMap* m) int oldstate = oldmap->get_state(mds); int newstate = mdsmap->get_state(mds); - if (!mdsmap->is_up(mds)) { - session->con->mark_down(); - } else if (mdsmap->get_addrs(mds) != session->addrs) { + if (!mdsmap->is_up(mds) || mdsmap->get_addrs(mds) != session->addrs) { session->con->mark_down(); session->addrs = mdsmap->get_addrs(mds); // When new MDS starts to take over, notify kernel to trim unused entries @@ -2674,10 +2672,21 @@ void Client::handle_mds_map(MMDSMap* m) trim_cache_for_reconnect(session); } else if (oldstate == newstate) continue; // no change - + + /* If we get here, the MDS has changed state or (!) another MDS has taken + * over. (Keep in mind that it's possible (oldstate == newstate == + * STATE_ACTIVE) but the addr has changed! This happens when the client + * misses epochs. + */ + session->mds_state = newstate; - if (newstate == MDSMap::STATE_RECONNECT) { + if (newstate >= MDSMap::STATE_RECONNECT) { session->con = messenger->connect_to_mds(session->addrs); + /* If we missed reconnect, the MDS will reset the session and we'll only + * reconnect if client_reconnect_stale==true. + */ + } + if (newstate == MDSMap::STATE_RECONNECT) { send_reconnect(session); } else if (newstate >= MDSMap::STATE_ACTIVE) { if (oldstate < MDSMap::STATE_ACTIVE) {

yunfeiguan · 2018-10-08T04:09:25Z

okay

batrick · 2018-10-17T17:47:10Z

src/mds/Server.cc

+        dout(1) << "send SESSION_CLOSE to client,so that it can reopen session" << dendl;
+        mds->send_message_client(MClientSession::create(CEPH_SESSION_CLOSE), session);
+        return;
+      }


@ukernel I think we tried this before but you said this should not be done. Comments?

ukernel · 2018-10-18T02:38:32Z

If client missed some mdsmap, I think it's better to use mds incarnation to detect if there were mds failover. if there were failover, set oldstate to STATE_NULL

ukernel · 2018-10-18T02:51:41Z

src/client/Client.cc

-      // in its dcache/icache. Hopefully, the kernel will release some unused
-      // inodes before the new MDS enters reconnect state.
-      trim_cache_for_reconnect(session);
+      if (mdsmap->get_addrs(mds) != session->addrs) {


if mds it not up, mdsmap->get_addrs(mds) will cause crash

i will change it later.

yunfeiguan · 2018-11-07T08:50:25Z

@ukernel @batrick help to review, thx

ukernel · 2018-11-29T07:23:38Z

how about following change. so that we avoid changing Server.cc

diff --git a/src/client/Client.cc b/src/client/Client.cc
index ae65738975..91c33020fc 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -2726,7 +2726,10 @@ void Client::handle_mds_map(MMDSMap* m)
       session->con = messenger->connect_to_mds(session->addrs);
       send_reconnect(session);
     } else if (newstate >= MDSMap::STATE_ACTIVE) {
-      if (oldstate < MDSMap::STATE_ACTIVE) {
+      if (oldstate < MDSMap::STATE_RECONNECT) {
+       // missed reconnect
+       _closed_mds_session(session);
+      } else if (oldstate < MDSMap::STATE_ACTIVE) {
        // kick new requests
        kick_requests(session);
        kick_flushing_caps(session);

yunfeiguan · 2018-11-29T08:32:25Z

The oldstate and newstate both are not sure because client may lose any piece of Message. So if the newstate is in the middle of STATE_RECONNECT and STATE_ACTIVE the changes above wont't make any sense. What do you think of following modification?

@@ -2688,8 +2688,9 @@ void Client::handle_mds_map(MMDSMap* m)
       continue;  // no change
     
     session->mds_state = newstate;
-    if (old_inc != new_inc && newstate > MDSMap::STATE_RECONNECT){
-      session->con = messenger->connect_to_mds(session->addrs);
+    if (old_inc != new_inc && newstate > MDSMap::STATE_RECONNECT) {
+      // missed reconnect
+      _closed_mds_session(session);
     }

ukernel · 2018-11-30T08:40:14Z

It works. If the test is true. you need to avoid executing code blocks of "else if (newstate >= MDSMap::STATE_ACTIVE)" and "else if (newstate == MDSMap::STATE_NULL && mds >= mdsmap->get_max_mds())"

ukernel · 2018-11-30T08:49:36Z

Besides, it's better to close old session and let client to re-open session when mds is fully recovered

batrick · 2018-12-10T17:01:12Z

src/mds/MDSMap.h

+  int get_incarnation(mds_rank_t m) const {
+    std::map<mds_rank_t, mds_gid_t>::const_iterator u = up.find(m);
+    if (u == up.end())
+      return -1;


return MDS_GID_NONE

batrick · 2018-12-10T17:01:48Z

src/mds/MDSMap.h

+  /**
+   * Get MDS rank incarnation if the rank is up, else -1
+   */
+  int get_incarnation(mds_rank_t m) const {


mds_gid_t get_incarnation

Please fix the return type of this method.

yunfeiguan · 2019-01-02T07:34:06Z

@ukernel @batrick help to review, thanks!

batrick · 2019-01-03T16:45:23Z

src/mds/MDSMap.h

+  /**
+   * Get MDS rank incarnation if the rank is up, else -1
+   */
+  int get_incarnation(mds_rank_t m) const {


Please fix the return type of this method.

batrick · 2019-01-03T16:45:37Z

src/client/Client.cc

@@ -2614,6 +2614,7 @@ void Client::handle_fs_map_user(MFSMapUser *m)

 void Client::handle_mds_map(MMDSMap* m)
 {
+  int old_inc = 0, new_inc = 0;


mds_gid_t instead of int

If fuse client session had been killed by mds and the mds daemon restart or hot-standby switch happens right away but the client did not receive any message from monitor due to network or other whatever reason untill the mds become active again.Thus cause client didn't do closed_mds_session lead the seession still is STATE_OPEN but client can't send any message to mds because its pipe is not ok.So we should close the stale session so that it can be reopened again. Fixes: http://tracker.ceph.com/issues/36079 Signed-off-by: Guan yunfei <yunfei.guan@xtaotech.com>

* refs/pull/24172/head: client: fix fuse client hang because its pipe to mds is not ok Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

yunfeiguan force-pushed the wip-36079 branch from a99b083 to c913e11 Compare September 19, 2018 07:37

tchaikov reviewed Sep 19, 2018

View reviewed changes

tchaikov added the cephfs Ceph File System label Sep 19, 2018

yunfeiguan force-pushed the wip-36079 branch from c913e11 to 15c0f7c Compare September 19, 2018 08:32

batrick requested changes Sep 24, 2018

View reviewed changes

yunfeiguan force-pushed the wip-36079 branch from 15c0f7c to 34065fa Compare September 29, 2018 02:29

batrick requested changes Oct 4, 2018

View reviewed changes

yunfeiguan force-pushed the wip-36079 branch 4 times, most recently from 670f4e1 to 290504a Compare October 8, 2018 09:06

batrick reviewed Oct 17, 2018

View reviewed changes

ukernel reviewed Oct 18, 2018

View reviewed changes

yunfeiguan force-pushed the wip-36079 branch 3 times, most recently from 43a4953 to 59897c7 Compare November 7, 2018 08:39

yunfeiguan force-pushed the wip-36079 branch from 59897c7 to 624cf8d Compare November 30, 2018 13:05

batrick requested changes Dec 10, 2018

View reviewed changes

yunfeiguan force-pushed the wip-36079 branch from 624cf8d to 8334382 Compare December 20, 2018 02:30

yunfeiguan force-pushed the wip-36079 branch from 8334382 to 7d10350 Compare January 1, 2019 00:42

batrick requested changes Jan 3, 2019

View reviewed changes

yunfeiguan force-pushed the wip-36079 branch 2 times, most recently from b613610 to 44c11b9 Compare January 4, 2019 03:31

yunfeiguan force-pushed the wip-36079 branch from 44c11b9 to 0e137de Compare January 4, 2019 04:22

batrick approved these changes Jan 4, 2019

View reviewed changes

batrick added the wip-pdonnell-testing label Jan 4, 2019

batrick merged commit 0e137de into ceph:master Jan 8, 2019

batrick added a commit that referenced this pull request Jan 8, 2019

Merge PR #24172 into master

6873dfc

* refs/pull/24172/head: client: fix fuse client hang because its pipe to mds is not ok Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

yunfeiguan deleted the wip-36079 branch April 8, 2019 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: fix fuse client hang because its pipe to mds is not ok #24172

client: fix fuse client hang because its pipe to mds is not ok #24172

yunfeiguan commented Sep 19, 2018 •

edited

tchaikov Sep 19, 2018

yunfeiguan Sep 19, 2018

tchaikov commented Sep 19, 2018

batrick Sep 24, 2018

batrick Sep 24, 2018

batrick Sep 24, 2018

batrick Sep 24, 2018

yunfeiguan Sep 25, 2018

batrick Oct 4, 2018

batrick Oct 4, 2018

yunfeiguan commented Oct 8, 2018

batrick Oct 17, 2018

ukernel commented Oct 18, 2018 •

edited

ukernel Oct 18, 2018

yunfeiguan Oct 18, 2018

yunfeiguan commented Nov 7, 2018

ukernel commented Nov 29, 2018 •

edited by tchaikov

yunfeiguan commented Nov 29, 2018 •

edited

ukernel commented Nov 30, 2018

ukernel commented Nov 30, 2018

batrick Dec 10, 2018

batrick Dec 10, 2018

batrick Jan 3, 2019

yunfeiguan commented Jan 2, 2019

batrick Jan 3, 2019

batrick Jan 3, 2019

client: fix fuse client hang because its pipe to mds is not ok #24172

client: fix fuse client hang because its pipe to mds is not ok #24172

Conversation

yunfeiguan commented Sep 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunfeiguan commented Oct 8, 2018

Choose a reason for hiding this comment

ukernel commented Oct 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunfeiguan commented Nov 7, 2018

ukernel commented Nov 29, 2018 • edited by tchaikov

yunfeiguan commented Nov 29, 2018 • edited

ukernel commented Nov 30, 2018

ukernel commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunfeiguan commented Jan 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunfeiguan commented Sep 19, 2018 •

edited

ukernel commented Oct 18, 2018 •

edited

ukernel commented Nov 29, 2018 •

edited by tchaikov

yunfeiguan commented Nov 29, 2018 •

edited