On Thu, 2017-12-07 at 14:21 +0100, Ilya Dryomov wrote:
> On Mon, Dec 4, 2017 at 4:25 PM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
