New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client/Client.cc: after reset session from MDS - reconnect #13522

Merged
merged 1 commit into from Apr 15, 2017

Conversation

Projects
None yet
5 participants
@singler
Contributor

singler commented Feb 19, 2017

Client.cc marks session as stale instead of reconecting after received
reset from MDS. On MDS side session it is closed so MDS is ignoring cap
renew. This fixes it by doing reconnect instead of just marking session
stale.

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

Signed-off-by: Henrik Korkuc henrik@kirneh.eu

@smithfarm smithfarm added the cephfs label Feb 19, 2017

@jcsp jcsp added the bug fix label Feb 20, 2017

ldout(cct, 1) << "reset from mds we were open; mark session as stale" << dendl;
s->state = MetaSession::STATE_STALE;
ldout(cct, 1) << "reset from mds we were open; reconnect mds session" << dendl;
send_reconnect(s);

This comment has been minimized.

@ukernel

ukernel Feb 24, 2017

Member

we should use _closed_mds_session(s) here

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Mar 7, 2017

@ukernel ping?

@jcsp

This comment has been minimized.

Contributor

jcsp commented Mar 8, 2017

That was the only place that STATE_STALE was set, so the flag is now unused. The kick_stale_sessions command also becomes meaningless once STATE_STALE is no longer used. If we go down this path then the patch needs to remove the now-dead bits too.

The stale handling stuff was added in this commit from @ukernel :

commit 09a1bc5a4601d356b9cc69be8541e6515d763861
Author: Yan, Zheng <zheng.z.yan@intel.com>
Date:   Fri Apr 11 15:03:37 2014 +0800

    client: add asok command to kick sessions that were remote reset
    
    Fixes: #8021
    Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>

I'm guessing the whole idea of that change was to make this an explicit human-driven process, rather than automatically reconnecting, presumably because reconnecting in this situation is not necessarily safe/healthy, because things might have changed while the client was away.

In practice I think most users are not going to understand/use the kick_stale_sessions command though.

More discussion is probably needed.

@singler

This comment has been minimized.

Contributor

singler commented Apr 12, 2017

Not sure if you had a chance to have a discussion about this issue, but I made it a config option to enable automatic reconnects, defaulting to original behaviour. It works for me with test case in tracker issue.

client/Client.cc: add feature to reconnect client after MDS reset
Client.cc marks session as stale instead of reconecting after received
reset from MDS. On MDS side session is closed so MDS is ignoring cap
renew. This adds option to reconnect stale client sessions instead of
just marking sessions stale.

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

Signed-off-by: Henrik Korkuc <henrik@kirneh.eu>

@jcsp jcsp merged commit 1776999 into ceph:master Apr 15, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@singler singler deleted the singler:wip-reconnect-client branch Apr 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment