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.)
Cheers,
--
Luís
