Re: [PATCH] ceph: drop negtive child dentries before try pruning inode's alias

[Jeff Layton <jlayton@redhat.com>]

On Thu, 2017-12-07 at 14:21 +0100, Ilya Dryomov wrote:
> On Mon, Dec 4, 2017 at 4:25 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Thu, 2017-11-30 at 22:12 +0800, Yan, Zheng wrote:
> > > negtive child dentry holds reference on inode's alias, it makes
> > > d_prune_aliases() do nothing.
> > > 
> > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> > > ---
> > >  fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 36 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > index da181acd4a61..9e7bb5fa9295 100644
> > > --- a/fs/ceph/mds_client.c
> > > +++ b/fs/ceph/mds_client.c
> > > @@ -1440,6 +1440,30 @@ static int __close_session(struct ceph_mds_client *mdsc,
> > >       return request_close_session(mdsc, session);
> > >  }
> > > 
> > > +static bool drop_negative_children(struct dentry *dentry)
> > > +{
> > > +     struct dentry *child;
> > > +     bool all_negtive = true;
> > > +
> > > +     if (!d_is_dir(dentry))
> > > +             goto out;
> > > +
> > > +     spin_lock(&dentry->d_lock);
> > > +     list_for_each_entry(child, &dentry->d_subdirs, d_child) {
> > > +             if (d_really_is_positive(child)) {
> > > +                     all_negtive = false;
> > > +                     break;
> > > +             }
> > > +     }
> > > +     spin_unlock(&dentry->d_lock);
> > > +
> > > +     if (all_negtive)
> > > +             shrink_dcache_parent(dentry);
> > > +out:
> > > +     dput(dentry);
> > 
> > It's sort of nasty to drop the reference here. It makes it harder to
> > track how that gets done when you acquire and drop references across a
> > function boundary like this. There's also no reason for it since you
> > always do it last here anyway.
> > 
> > It'd be nicer to just move that into the caller.
> > 
> > > +     return all_negtive;
> > > +}
> > > +
> > >  /*
> > >   * Trim old(er) caps.
> > >   *
> > > @@ -1495,15 +1519,20 @@ static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg)
> > >               __ceph_remove_cap(cap, true);
> > >               session->s_trim_caps--;
> > >       } else {
> > > -             int refs;
> > > +             struct dentry *dentry;
> > >               /* try dropping referring dentries */
> > >               spin_unlock(&ci->i_ceph_lock);
> > > -             d_prune_aliases(inode);
> > > -             refs = atomic_read(&inode->i_count);
> > > -             if (refs == 1)
> > > -                     session->s_trim_caps--;
> > > -             dout("trim_caps_cb %p cap %p  pruned, count now %d\n",
> > > -                  inode, cap, refs);
> > > +             dentry = d_find_any_alias(inode);
> > > +             /* drop_negative_children() calls dput() */
> > > +             if (dentry && drop_negative_children(dentry)) {
> > > +                     int refs;
> > > +                     d_prune_aliases(inode);
> > > +                     refs = atomic_read(&inode->i_count);
> > > +                     if (refs == 1)
> > > +                             session->s_trim_caps--;
> > > +                     dout("trim_caps_cb %p cap %p pruned, count now %d\n",
> > > +                          inode, cap, refs);
> > > +             }
> > >               return 0;
> > >       }
> > > 
> > 
> > I think the rest looks fine though.
> 
> Hi Jeff, Zheng,
> 
> What is the status of this patch?  It is marked for stable in the
> testing branch and Zheng force-pushed it two days ago, addressing
> comments.  Jeff, can you take a look?
> 
> Zheng, negtive and all_negtive typos need fixing.
> 
> Also, would it make sense to merge these two patches ("ceph: decease
> session->s_trim_caps only after caps get trimmed" and "ceph: drop
> negtive child dentries before try pruning inode's alias") into one?
> They seem to fix the same bug and the second almost entirely redoes the
> first.
> 


Yep, that one (56b5c1eed996) looks good to me.

-- 
Jeff Layton <jlayton@redhat.com>