Re: [PATCH v4 3/3] ceph: don't unregister an MDS session before removing its caps

[Xiubo Li]

Hi Max

I think you are right. I just checked the code again, I might have
messed up the code with my local temp patches.

Then this LGTM.

Reviewed-by: Xiubo Li <xiubo.li@xxxxxxxxx>

Thanks

On Mon, 7 Sept 2026 at 12:15, Max Kellermann <max.kellermann@xxxxxxxxx> wrote:
>
> On Sat, Sep 5, 2026 at 3:56 AM Xiubo Li <xiubo.li@xxxxxxxxx> wrote:
> > Just one remaining teardown path still unregisters the session before
> > removing its caps: the fallback loop in ceph_mdsc_close_sessions():
> >
> > mutex_lock(&mdsc->mutex);
> > for (i = 0; i < mdsc->max_sessions; i++) {
> >         if (mdsc->sessions[i]) {
> >                 session = ceph_get_mds_session(mdsc->sessions[i]);
> >                 __unregister_session(mdsc, session);
> >                 mutex_unlock(&mdsc->mutex);
> >                 mutex_lock(&session->s_mutex);
> >                 remove_session_caps(session);
> >                 mutex_unlock(&session->s_mutex);
> >                 ...
> >
> > The rest of this series consistently changes the ordering to keep the
> > rank occupied until remove_session_caps() has completed. Is the
> > ordering in ceph_mdsc_close_sessions() intentionally excluded?
> >
> > This path is not limited to shutdown: it is also reached from the
> > corrupt snap-trace handling in ceph_handle_caps(), ceph_handle_reply()
> > and handle_snap(). In those cases mdsc->stopping may be false and the
> > mount need not be in CEPH_MOUNT_FENCE_IO, so a concurrent
> > __do_request() can register a new session for the rank in the window
> > between __unregister_session() and remove_session_caps().
>
> I don't think that's true for the call paths you mentioned. They call
> ceph_mdsc_close_sessions() only after ceph_update_snap_trace() has
> failed, and on failure, that function always sets CEPH_MOUNT_FENCE_IO,
> which prevents creating a new session.
>
> The only other callers are ceph_put_super() and the mount-failure path
> in ceph_get_tree(), where this is not relevant (no requests exist).
>
> Am I missing something?
>
> --
> Max Kellermann
> Principal Architect
> Hosting Technology
>
> cm4all | Im Mediapark 6a | 50670 Köln | Germany
> General information about the company can be found here:
> https://www.cm4all.com/impressum
> A member of the IONOS Group