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.
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
