ceph: drop negtive child dentries before try pruning inode's alias
PATCH 1/1 ceph: drop negtive child dentries before try pruning inode's alias
negtive child dentry holds reference on inode's alias, it makes d_prune_aliases() do nothing.
--- fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)
fs/ceph/mds_client.c +36 -7
@@ -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 ; }
Signature · 1 line
2.13.6
Quoted 61 lines · Zheng
negtive child dentry holds reference on inode's alias, it makes
d_prune_aliases()do nothing.Signed-off-by "Yan, Zheng" <[email protected]>--- fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)fs/ceph/mds_client.c +29
-7@@ -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;
fs/ceph/mds_client.c static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg) · line 1538
Quoted 10 lines
fs/ceph/mds_client.c +7
-0+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.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 68 lines · Jeff Layton
Quoted 67 lines · Zheng
negtive child dentry holds reference on inode's alias, it makes
d_prune_aliases()do nothing.Signed-off-by "Yan, Zheng" <[email protected]>--- fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)fs/ceph/mds_client.c +35
-7@@ -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);
Quoted 9 lines
fs/ceph/mds_client.c static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg) · line 1538
Quoted 4 lines
fs/ceph/mds_client.c +1
-0+ }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
Quoted 6 lines
--
Jeff Layton <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 27 lines · Zheng
negtive child dentry holds reference on inode's alias, it makes
d_prune_aliases()do nothing.Signed-off-by "Yan, Zheng" <[email protected]>--- fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)fs/ceph/mds_client.c +11
-0@@ -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)) {
fs/ceph/mds_client.c static int __close_session(struct ceph_mds_client *mdsc, · line 1463
Quoted 10 lines
fs/ceph/mds_client.c +10
-0+ 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.
Quoted 24 lines
fs/ceph/mds_client.c +8
-7+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;
fs/ceph/mds_client.c static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg) · line 1538
Quoted 10 lines
fs/ceph/mds_client.c +7
-0+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.
Signature · 1 line
Jeff Layton <[email protected]>
Ilya Dryomov
maintainerQuoted 74 lines · Jeff Layton
Quoted 27 lines · Zheng
negtive child dentry holds reference on inode's alias, it makes
d_prune_aliases()do nothing.Signed-off-by "Yan, Zheng" <[email protected]>--- fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)fs/ceph/mds_client.c +11
-0@@ -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)) {fs/ceph/mds_client.c static int __close_session(struct ceph_mds_client *mdsc, · line 1463
Quoted 10 lines
fs/ceph/mds_client.c +10
-0+ 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.
Quoted 28 lines
fs/ceph/mds_client.c +12
-7+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--;
Quoted 8 lines
fs/ceph/mds_client.c static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg) · line 1538
Quoted 6 lines
fs/ceph/mds_client.c +3
-0+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
Quoted 90 lines · Ilya Dryomov
Quoted 74 lines · Jeff Layton
Quoted 27 lines · Zheng
negtive child dentry holds reference on inode's alias, it makes
d_prune_aliases()do nothing.Signed-off-by "Yan, Zheng" <[email protected]>--- fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)fs/ceph/mds_client.c +11
-0@@ -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)) {fs/ceph/mds_client.c static int __close_session(struct ceph_mds_client *mdsc, · line 1463
Quoted 10 lines
fs/ceph/mds_client.c +10
-0+ 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.
Quoted 28 lines
fs/ceph/mds_client.c +12
-7+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--;Quoted 8 lines
fs/ceph/mds_client.c static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg) · line 1538
Quoted 6 lines
fs/ceph/mds_client.c +3
-0+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?
Quoted 8 lines
Zheng, negtive and
all_negtivetypos need fixing.Also, would it make sense to merge these two patches ("ceph: decease
session->s_trim_capsonly 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.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 90 lines · Ilya Dryomov
Quoted 74 lines · Jeff Layton
Quoted 27 lines · Zheng
negtive child dentry holds reference on inode's alias, it makes
d_prune_aliases()do nothing.Signed-off-by "Yan, Zheng" <[email protected]>--- fs/ceph/mds_client.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)fs/ceph/mds_client.c +11
-0@@ -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)) {fs/ceph/mds_client.c static int __close_session(struct ceph_mds_client *mdsc, · line 1463
Quoted 10 lines
fs/ceph/mds_client.c +10
-0+ 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.
Quoted 28 lines
fs/ceph/mds_client.c +12
-7+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--;Quoted 8 lines
fs/ceph/mds_client.c static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg) · line 1538
Quoted 6 lines
fs/ceph/mds_client.c +3
-0+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?
Quoted 8 lines
Zheng, negtive and
all_negtivetypos need fixing.Also, would it make sense to merge these two patches ("ceph: decease
session->s_trim_capsonly 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.
Quoted 7 lines
Thanks,
Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html