On Wed, 2021-06-16 at 16:25 +0100, Luis Henriques wrote:
> On Tue, Jun 15, 2021 at 10:57:27AM -0400, Jeff Layton wrote:
> > These locks appear to be completely unnecessary. Almost all of this
> > function is done under the inode->i_ceph_lock, aside from the actual
> > sending of the message. Don't take either lock in this function.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/ceph/caps.c | 61 ++++++--------------------------------------------
> > 1 file changed, 7 insertions(+), 54 deletions(-)
> >
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 919eada97a1f..825b1e463ad3 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -1912,7 +1912,6 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags,
> > struct ceph_cap *cap;
> > u64 flush_tid, oldest_flush_tid;
> > int file_wanted, used, cap_used;
> > - int took_snap_rwsem = 0; /* true if mdsc->snap_rwsem held */
> > int issued, implemented, want, retain, revoking, flushing = 0;
> > int mds = -1; /* keep track of how far we've gone through i_caps list
> > to avoid an infinite loop on retry */
> > @@ -1920,6 +1919,9 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags,
> > bool queue_invalidate = false;
> > bool tried_invalidate = false;
> >
> > + if (session)
> > + ceph_get_mds_session(session);
> > +
> > spin_lock(&ci->i_ceph_lock);
> > if (ci->i_ceph_flags & CEPH_I_FLUSH)
> > flags |= CHECK_CAPS_FLUSH;
> > @@ -2021,8 +2023,6 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags,
> > ((flags & CHECK_CAPS_AUTHONLY) && cap != ci->i_auth_cap))
> > continue;
> >
> > - /* NOTE: no side-effects allowed, until we take s_mutex */
> > -
> > /*
> > * If we have an auth cap, we don't need to consider any
> > * overlapping caps as used.
> > @@ -2085,37 +2085,8 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags,
> > continue; /* nope, all good */
> >
> > ack:
> > - if (session && session != cap->session) {
> > - dout("oops, wrong session %p mutex\n", session);
> > - mutex_unlock(&session->s_mutex);
> > - session = NULL;
> > - }
> > - if (!session) {
> > - session = cap->session;
> > - if (mutex_trylock(&session->s_mutex) == 0) {
> > - dout("inverting session/ino locks on %p\n",
> > - session);
> > - session = ceph_get_mds_session(session);
> > - spin_unlock(&ci->i_ceph_lock);
> > - if (took_snap_rwsem) {
> > - up_read(&mdsc->snap_rwsem);
> > - took_snap_rwsem = 0;
> > - }
> > - if (session) {
> > - mutex_lock(&session->s_mutex);
> > - ceph_put_mds_session(session);
> > - } else {
> > - /*
> > - * Because we take the reference while
> > - * holding the i_ceph_lock, it should
> > - * never be NULL. Throw a warning if it
> > - * ever is.
> > - */
> > - WARN_ON_ONCE(true);
> > - }
> > - goto retry;
> > - }
> > - }
> > + ceph_put_mds_session(session);
> > + session = ceph_get_mds_session(cap->session);
> >
> > /* kick flushing and flush snaps before sending normal
> > * cap message */
> > @@ -2130,19 +2101,6 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags,
> > goto retry_locked;
> > }
> >
> > - /* take snap_rwsem after session mutex */
> > - if (!took_snap_rwsem) {
> > - if (down_read_trylock(&mdsc->snap_rwsem) == 0) {
> > - dout("inverting snap/in locks on %p\n",
> > - inode);
> > - spin_unlock(&ci->i_ceph_lock);
> > - down_read(&mdsc->snap_rwsem);
> > - took_snap_rwsem = 1;
> > - goto retry;
> > - }
> > - took_snap_rwsem = 1;
> > - }
> > -
> > if (cap == ci->i_auth_cap && ci->i_dirty_caps) {
> > flushing = ci->i_dirty_caps;
> > flush_tid = __mark_caps_flushing(inode, session, false,
> > @@ -2179,13 +2137,9 @@ void ceph_check_caps(struct ceph_inode_info *ci, int flags,
> >
> > spin_unlock(&ci->i_ceph_lock);
> >
> > + ceph_put_mds_session(session);
> > if (queue_invalidate)
> > ceph_queue_invalidate(inode);
> > -
> > - if (session)
> > - mutex_unlock(&session->s_mutex);
> > - if (took_snap_rwsem)
> > - up_read(&mdsc->snap_rwsem);
> > }
> >
> > /*
> > @@ -3550,13 +3504,12 @@ static void handle_cap_grant(struct inode *inode,
> > if (wake)
> > wake_up_all(&ci->i_cap_wq);
> >
> > + mutex_unlock(&session->s_mutex);
> > if (check_caps == 1)
> > ceph_check_caps(ci, CHECK_CAPS_AUTHONLY | CHECK_CAPS_NOINVAL,
> > session);
> > else if (check_caps == 2)
> > ceph_check_caps(ci, CHECK_CAPS_NOINVAL, session);
> > - else
> > - mutex_unlock(&session->s_mutex);
> > }
> >
> > /*
> > --
> > 2.31.1
> >
>
> Ugh, this is a tricky one. I couldn't find anything wrong but... yeah,
> here it is:
>
> Reviewed-by: Luis Henriques <lhenriques@xxxxxxx>
>
> (Suggestion: remove the 'retry/retry_locked' goto dance and simply lock
> i_ceph_lock in the only 'goto retry' call.)
>
Thanks. Good suggestion. I'll incorporate it before I merge, but I won't
plan to resend unless there are bigger changes needed.
--
Jeff Layton <jlayton@xxxxxxxxxx>
