| Message ID | 20171130141259.8577-1-zyan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- 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
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.
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 -- 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
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.
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 -- 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
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; }
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(-)