On Thu, Dec 7, 2017 at 9:21 PM, Ilya Dryomov <idryomov@xxxxxxxxx> 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.
>
I merged them and re-push the new one to testing branch.
Thank all of you.
> Thanks,
>
> Ilya
> --
> 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
--
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
