On Fri, 2020-02-28 at 15:27 +0100, Ilya Dryomov wrote:
> On Fri, Feb 28, 2020 at 3:15 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > The last thing that this function does is release i_ceph_lock, so
> > have the caller do that instead. Add a lockdep assertion to
> > ensure that the function is always called with i_ceph_lock held.
> > Change the prototype to take a ceph_inode_info pointer and drop
> > the separate mdsc argument as we can get that from the session.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/ceph/caps.c | 23 +++++++++--------------
> > 1 file changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 9b3d5816c109..c02b63070e0a 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -2424,16 +2424,15 @@ void ceph_kick_flushing_caps(struct ceph_mds_client *mdsc,
> > }
> > }
> >
> > -static void kick_flushing_inode_caps(struct ceph_mds_client *mdsc,
> > - struct ceph_mds_session *session,
> > - struct inode *inode)
> > - __releases(ci->i_ceph_lock)
> > +static void kick_flushing_inode_caps(struct ceph_mds_session *session,
> > + struct ceph_inode_info *ci)
> > {
> > - struct ceph_inode_info *ci = ceph_inode(inode);
> > - struct ceph_cap *cap;
> > + struct ceph_mds_client *mdsc = session->s_mdsc;
> > + struct ceph_cap *cap = ci->i_auth_cap;
> > +
> > + lockdep_assert_held(&ci->i_ceph_lock);
> >
> > - cap = ci->i_auth_cap;
> > - dout("kick_flushing_inode_caps %p flushing %s\n", inode,
> > + dout("%s %p flushing %s\n", __func__, &ci->vfs_inode,
> > ceph_cap_string(ci->i_flushing_caps));
> >
> > if (!list_empty(&ci->i_cap_flush_list)) {
> > @@ -2445,9 +2444,6 @@ static void kick_flushing_inode_caps(struct ceph_mds_client *mdsc,
> > spin_unlock(&mdsc->cap_dirty_lock);
> >
> > __kick_flushing_caps(mdsc, session, ci, oldest_flush_tid);
> > - spin_unlock(&ci->i_ceph_lock);
> > - } else {
> > - spin_unlock(&ci->i_ceph_lock);
> > }
> > }
> >
> > @@ -3326,11 +3322,10 @@ static void handle_cap_grant(struct inode *inode,
> > if (le32_to_cpu(grant->op) == CEPH_CAP_OP_IMPORT) {
> > if (newcaps & ~extra_info->issued)
> > wake = true;
> > - kick_flushing_inode_caps(session->s_mdsc, session, inode);
> > + kick_flushing_inode_caps(session, ci);
> > up_read(&session->s_mdsc->snap_rwsem);
> > - } else {
> > - spin_unlock(&ci->i_ceph_lock);
> > }
> > + spin_unlock(&ci->i_ceph_lock);
>
> Hi Jeff,
>
> I would keep the else clause here and release i_ceph_lock before
> snap_rwsem for proper nesting. Otherwise kudos on getting rid of
> another locking quirk!
>
> Thanks,
>
> Ilya
Meh, ok. I don't think the unlock order really matters much here, but
I'll make that change and push to testing.
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
