* [PATCH] ceph: drop negtive child dentries before try pruning inode's alias
@ 2017-11-30 14:12 Yan, Zheng
2017-11-30 14:37 ` Jeff Layton
2017-12-04 15:25 ` Jeff Layton
0 siblings, 2 replies; 7+ messages in thread
From: Yan, Zheng @ 2017-11-30 14:12 UTC (permalink / raw)
To: ceph-devel; +Cc: jlayton, Yan, Zheng
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);
+ 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;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ceph: drop negtive child dentries before try pruning inode's alias
2017-11-30 14:12 [PATCH] ceph: drop negtive child dentries before try pruning inode's alias Yan, Zheng
@ 2017-11-30 14:37 ` Jeff Layton
2017-12-01 1:07 ` Yan, Zheng
2017-12-04 15:25 ` Jeff Layton
1 sibling, 1 reply; 7+ messages in thread
From: Jeff Layton @ 2017-11-30 14:37 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel
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);
> + 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;
> }
>
Would it be better to do this at the VFS layer? Dropping negative
children when we're going to prune dentries for an inode seems like it
ought to be done as a matter of course in d_prune_aliases.
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ceph: drop negtive child dentries before try pruning inode's alias
2017-11-30 14:37 ` Jeff Layton
@ 2017-12-01 1:07 ` Yan, Zheng
0 siblings, 0 replies; 7+ messages in thread
From: Yan, Zheng @ 2017-12-01 1:07 UTC (permalink / raw)
To: Jeff Layton; +Cc: Yan, Zheng, ceph-devel, Al Viro
On Thu, Nov 30, 2017 at 10:37 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);
>> + 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;
>> }
>>
>
> Would it be better to do this at the VFS layer? Dropping negative
> children when we're going to prune dentries for an inode seems like it
> ought to be done as a matter of course in d_prune_aliases.
>
I checked other users of d_prune_aliases. Some of them call
d_invalidate/shrink_dcache_parent before calling d_prune_aliases, some
of them only call d_prune_aliases for non-directory inode. It seems
only ceph has this requirement (trim unused inodes so that MDS can
trim corresponding objects from its cache)
Regards
Yan, Zheng
> --
> Jeff Layton <jlayton@redhat.com>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ceph: drop negtive child dentries before try pruning inode's alias
2017-11-30 14:12 [PATCH] ceph: drop negtive child dentries before try pruning inode's alias Yan, Zheng
2017-11-30 14:37 ` Jeff Layton
@ 2017-12-04 15:25 ` Jeff Layton
2017-12-07 13:21 ` Ilya Dryomov
1 sibling, 1 reply; 7+ messages in thread
From: Jeff Layton @ 2017-12-04 15:25 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel
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.
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ceph: drop negtive child dentries before try pruning inode's alias
2017-12-04 15:25 ` Jeff Layton
@ 2017-12-07 13:21 ` Ilya Dryomov
2017-12-07 20:42 ` Jeff Layton
2017-12-08 7:32 ` Yan, Zheng
0 siblings, 2 replies; 7+ messages in thread
From: Ilya Dryomov @ 2017-12-07 13:21 UTC (permalink / raw)
To: Jeff Layton; +Cc: Yan, Zheng, Ceph Development
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.
Thanks,
Ilya
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ceph: drop negtive child dentries before try pruning inode's alias
2017-12-07 13:21 ` Ilya Dryomov
@ 2017-12-07 20:42 ` Jeff Layton
2017-12-08 7:32 ` Yan, Zheng
1 sibling, 0 replies; 7+ messages in thread
From: Jeff Layton @ 2017-12-07 20:42 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Yan, Zheng, Ceph Development
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ceph: drop negtive child dentries before try pruning inode's alias
2017-12-07 13:21 ` Ilya Dryomov
2017-12-07 20:42 ` Jeff Layton
@ 2017-12-08 7:32 ` Yan, Zheng
1 sibling, 0 replies; 7+ messages in thread
From: Yan, Zheng @ 2017-12-08 7:32 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Jeff Layton, Yan, Zheng, Ceph Development
On Thu, Dec 7, 2017 at 9:21 PM, Ilya Dryomov <idryomov@gmail.com> 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.
>
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-12-08 7:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-30 14:12 [PATCH] ceph: drop negtive child dentries before try pruning inode's alias Yan, Zheng
2017-11-30 14:37 ` Jeff Layton
2017-12-01 1:07 ` Yan, Zheng
2017-12-04 15:25 ` Jeff Layton
2017-12-07 13:21 ` Ilya Dryomov
2017-12-07 20:42 ` Jeff Layton
2017-12-08 7:32 ` Yan, Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.