On Tue, 2019-07-09 at 22:50 +0800, Yan, Zheng wrote:
> On Tue, Jul 9, 2019 at 6:13 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Tue, 2019-07-09 at 07:52 +0800, Yan, Zheng wrote:
> > > On Tue, Jul 9, 2019 at 3:23 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > > I've been working on a patchset to add inline write support to kcephfs,
> > > > and have run across a potential race in fsync. I could use someone to
> > > > sanity check me though since I don't have a great grasp of the MDS
> > > > session handling:
> > > >
> > > > ceph_fsync() calls try_flush_caps() to flush the dirty metadata back to
> > > > the MDS when Fw caps are flushed back. try_flush_caps does this,
> > > > however:
> > > >
> > > > if (cap->session->s_state < CEPH_MDS_SESSION_OPEN) {
> > > > spin_unlock(&ci->i_ceph_lock);
> > > > goto out;
> > > > }
> > > >
> > >
> > > enum {
> > > CEPH_MDS_SESSION_NEW = 1,
> > > CEPH_MDS_SESSION_OPENING = 2,
> > > CEPH_MDS_SESSION_OPEN = 3,
> > > CEPH_MDS_SESSION_HUNG = 4,
> > > CEPH_MDS_SESSION_RESTARTING = 5,
> > > CEPH_MDS_SESSION_RECONNECTING = 6,
> > > CEPH_MDS_SESSION_CLOSING = 7,
> > > CEPH_MDS_SESSION_REJECTED = 8,
> > > };
> > >
> > > the value of reconnect state is larger than 2
> > >
> >
> > Right, I get that. The big question is whether you can ever move from a
> > higher state to something less than CEPH_MDS_SESSION_OPEN.
> >
>
> I guess it does not happen because closing session happens only when
> umounting.
>
Got it, that makes some sense.
> But there is a corner case in handle_cap_export(). It set inode's auth
> cap to a place holder cap. the placeholder cap's session can be in
> opening state.
>
> > __do_request can do this:
> >
> > if (session->s_state == CEPH_MDS_SESSION_NEW ||
> > session->s_state == CEPH_MDS_SESSION_CLOSING)
> > __open_session(mdsc, session);
> >
Given that though, does it make sense to call __open_session when the
state is CLOSING? Should we not just set req->r_err and return at that
point?
> > ...and __open_session does this:
> >
> > session->s_state = CEPH_MDS_SESSION_OPENING;
> >
> > ...so it sort of looks like you can go from CLOSING(7) to OPENING(2).
> > That said, I don't have a great feel for the session state transitions,
> > and don't know whether this is a real possibility.
> >
> > > > ...at that point, try_flush_caps will return 0, and set *ptid to 0 on
> > > > the way out. ceph_fsync won't see that Fw is still dirty at that point
> > > > and won't wait, returning without flushing metadata.
> > > >
> > > > Am I missing something that prevents this? I can open a tracker bug for
> > > > this if it is a problem, but I wanted to be sure it was a bug before I
> > > > did so.
> > > >
> > > > Thanks,
> > > > --
> > > > Jeff Layton <jlayton@xxxxxxxxxx>
> > > >
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>
> >
--
Jeff Layton <jlayton@xxxxxxxxxx>
