* [PATCH 2/2] fs: fix dentry_lru_prune() @ 2013-03-07 11:37 Yan, Zheng 2013-03-08 2:04 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Yan, Zheng @ 2013-03-07 11:37 UTC (permalink / raw) To: linux-fsdevel, ceph-devel; +Cc: sage, viro, Yan, Zheng From: "Yan, Zheng" <zheng.z.yan@intel.com> dentry_lru_prune() should always call file system's d_prune callback. Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com> --- fs/dcache.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index 19153a0..f0060aa 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -344,14 +344,9 @@ static void dentry_lru_del(struct dentry *dentry) */ static void dentry_lru_prune(struct dentry *dentry) { - if (!list_empty(&dentry->d_lru)) { - if (dentry->d_flags & DCACHE_OP_PRUNE) - dentry->d_op->d_prune(dentry); - - spin_lock(&dcache_lru_lock); - __dentry_lru_del(dentry); - spin_unlock(&dcache_lru_lock); - } + if (dentry->d_flags & DCACHE_OP_PRUNE) + dentry->d_op->d_prune(dentry); + dentry_lru_del(dentry); } static void dentry_lru_move_list(struct dentry *dentry, struct list_head *list) -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs: fix dentry_lru_prune() 2013-03-07 11:37 [PATCH 2/2] fs: fix dentry_lru_prune() Yan, Zheng @ 2013-03-08 2:04 ` Dave Chinner 2013-03-08 2:43 ` Yan, Zheng 0 siblings, 1 reply; 6+ messages in thread From: Dave Chinner @ 2013-03-08 2:04 UTC (permalink / raw) To: Yan, Zheng; +Cc: linux-fsdevel, ceph-devel, sage, viro On Thu, Mar 07, 2013 at 07:37:36PM +0800, Yan, Zheng wrote: > From: "Yan, Zheng" <zheng.z.yan@intel.com> > > dentry_lru_prune() should always call file system's d_prune callback. Why? What bug does this fix? Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs: fix dentry_lru_prune() 2013-03-08 2:04 ` Dave Chinner @ 2013-03-08 2:43 ` Yan, Zheng 2013-03-08 6:27 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Yan, Zheng @ 2013-03-08 2:43 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-fsdevel, ceph-devel, sage, viro On 03/08/2013 10:04 AM, Dave Chinner wrote: > On Thu, Mar 07, 2013 at 07:37:36PM +0800, Yan, Zheng wrote: >> From: "Yan, Zheng" <zheng.z.yan@intel.com> >> >> dentry_lru_prune() should always call file system's d_prune callback. > > Why? What bug does this fix? > Ceph uses a flag to track if the dcache contents for a directory are complete, and it relies on d_prune() to clear the flag when some dentries are trimmed. We noticed that dentry_lru_prune() sometimes does not call ceph_d_prune(). It seems the dentry in question is ancestor trimmed by try_prune_one_dentry(). Regards Yan, Zheng ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs: fix dentry_lru_prune() 2013-03-08 2:43 ` Yan, Zheng @ 2013-03-08 6:27 ` Dave Chinner 2013-03-08 6:40 ` Yan, Zheng 0 siblings, 1 reply; 6+ messages in thread From: Dave Chinner @ 2013-03-08 6:27 UTC (permalink / raw) To: Yan, Zheng; +Cc: linux-fsdevel, ceph-devel, sage, viro On Fri, Mar 08, 2013 at 10:43:00AM +0800, Yan, Zheng wrote: > On 03/08/2013 10:04 AM, Dave Chinner wrote: > > On Thu, Mar 07, 2013 at 07:37:36PM +0800, Yan, Zheng wrote: > >> From: "Yan, Zheng" <zheng.z.yan@intel.com> > >> > >> dentry_lru_prune() should always call file system's d_prune callback. > > > > Why? What bug does this fix? > > > > Ceph uses a flag to track if the dcache contents for a directory are complete, > and it relies on d_prune() to clear the flag when some dentries are trimmed. > We noticed that dentry_lru_prune() sometimes does not call ceph_d_prune(). > It seems the dentry in question is ancestor trimmed by try_prune_one_dentry(). That doesn't sound right to me. Any dentry that goes through try_prune_one_dentry() is on a LRU list, and will end up in dentry_kill() if the reference count drops to zero and hence calls dentry_lru_prune() with a non-emtpy LRU pointer. If it has a non-zero reference count, it gets removed from the LRU, and the next call to dput() that drops the reference count to zero will add it back to the LRU and it will go around again. So it sounds to me like there is something else going on here. FWIW, if the dentry is not on the LRU, why would it need pruning? If it needs pruning regardless of it's status on the LRU, then dentry_lru_prune() should go away entirely and pruning be done explicity where it is needed rather than wrapped up in an unrelated LRU operation.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs: fix dentry_lru_prune() 2013-03-08 6:27 ` Dave Chinner @ 2013-03-08 6:40 ` Yan, Zheng 2013-03-09 9:35 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Yan, Zheng @ 2013-03-08 6:40 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-fsdevel, ceph-devel, sage, viro On 03/08/2013 02:27 PM, Dave Chinner wrote: > On Fri, Mar 08, 2013 at 10:43:00AM +0800, Yan, Zheng wrote: >> On 03/08/2013 10:04 AM, Dave Chinner wrote: >>> On Thu, Mar 07, 2013 at 07:37:36PM +0800, Yan, Zheng wrote: >>>> From: "Yan, Zheng" <zheng.z.yan@intel.com> >>>> >>>> dentry_lru_prune() should always call file system's d_prune callback. >>> >>> Why? What bug does this fix? >>> >> >> Ceph uses a flag to track if the dcache contents for a directory are complete, >> and it relies on d_prune() to clear the flag when some dentries are trimmed. >> We noticed that dentry_lru_prune() sometimes does not call ceph_d_prune(). >> It seems the dentry in question is ancestor trimmed by try_prune_one_dentry(). > > That doesn't sound right to me. Any dentry that goes through > try_prune_one_dentry() is on a LRU list, and will end up in > dentry_kill() if the reference count drops to zero and hence calls > dentry_lru_prune() with a non-emtpy LRU pointer. > > If it has a non-zero reference count, it gets removed from the LRU, > and the next call to dput() that drops the reference count to zero > will add it back to the LRU and it will go around again. So it > sounds to me like there is something else going on here. > > FWIW, if the dentry is not on the LRU, why would it need pruning? > If it needs pruning regardless of it's status on the LRU, then > dentry_lru_prune() should go away entirely and pruning be done > explicity where it is needed rather than wrapped up in an unrelated > LRU operation.... > I didn't described it clearly static void try_prune_one_dentry(struct dentry *dentry) __releases(dentry->d_lock) { ..... /* Prune ancestors. */ dentry = parent; while (dentry) { spin_lock(&dentry->d_lock); if (dentry->d_count > 1) { dentry->d_count--; spin_unlock(&dentry->d_lock); return; } dentry = dentry_kill(dentry, 1); ~~~~I mean dentries that are pruned here~~~~ } } Regards Yan, Zheng ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fs: fix dentry_lru_prune() 2013-03-08 6:40 ` Yan, Zheng @ 2013-03-09 9:35 ` Dave Chinner 0 siblings, 0 replies; 6+ messages in thread From: Dave Chinner @ 2013-03-09 9:35 UTC (permalink / raw) To: Yan, Zheng; +Cc: linux-fsdevel, ceph-devel, sage, viro On Fri, Mar 08, 2013 at 02:40:46PM +0800, Yan, Zheng wrote: > On 03/08/2013 02:27 PM, Dave Chinner wrote: > > On Fri, Mar 08, 2013 at 10:43:00AM +0800, Yan, Zheng wrote: > >> On 03/08/2013 10:04 AM, Dave Chinner wrote: > >>> On Thu, Mar 07, 2013 at 07:37:36PM +0800, Yan, Zheng wrote: > >>>> From: "Yan, Zheng" <zheng.z.yan@intel.com> > >>>> > >>>> dentry_lru_prune() should always call file system's d_prune callback. > >>> > >>> Why? What bug does this fix? > >>> > >> > >> Ceph uses a flag to track if the dcache contents for a directory are complete, > >> and it relies on d_prune() to clear the flag when some dentries are trimmed. > >> We noticed that dentry_lru_prune() sometimes does not call ceph_d_prune(). > >> It seems the dentry in question is ancestor trimmed by try_prune_one_dentry(). > > > > That doesn't sound right to me. Any dentry that goes through > > try_prune_one_dentry() is on a LRU list, and will end up in > > dentry_kill() if the reference count drops to zero and hence calls > > dentry_lru_prune() with a non-emtpy LRU pointer. > > > > If it has a non-zero reference count, it gets removed from the LRU, > > and the next call to dput() that drops the reference count to zero > > will add it back to the LRU and it will go around again. So it > > sounds to me like there is something else going on here. > > > > FWIW, if the dentry is not on the LRU, why would it need pruning? > > If it needs pruning regardless of it's status on the LRU, then > > dentry_lru_prune() should go away entirely and pruning be done > > explicity where it is needed rather than wrapped up in an unrelated > > LRU operation.... > > > > I didn't described it clearly > > static void try_prune_one_dentry(struct dentry *dentry) > __releases(dentry->d_lock) > { > ..... > /* Prune ancestors. */ > dentry = parent; > while (dentry) { > spin_lock(&dentry->d_lock); > if (dentry->d_count > 1) { > dentry->d_count--; > spin_unlock(&dentry->d_lock); > return; > } > dentry = dentry_kill(dentry, 1); > ~~~~I mean dentries that are pruned here~~~~ IOWs, what we have is a situation where the ancestor dentry never goes through dput(), and so never gets put on the LRU when it's reference count goes to zero. Hence associating d_prune with removing the dentry from the LRU is the wrong thing to be doing here. What about the other places that use dentry_lru_prune() - do they have the same problem? Oh, there's only one - shrink_dcache_for_umount_subtree() - and it looks to have the same problem as ancestors have their d_count decremented to zero in the loop rather than by dput() and so won't be on the LRU. IOWs, as I mentioned above, dentry_lru_prune should die as .d_prune is not related to the dentry LRU state at all and needs to be done unconditionally... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-09 9:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-07 11:37 [PATCH 2/2] fs: fix dentry_lru_prune() Yan, Zheng 2013-03-08 2:04 ` Dave Chinner 2013-03-08 2:43 ` Yan, Zheng 2013-03-08 6:27 ` Dave Chinner 2013-03-08 6:40 ` Yan, Zheng 2013-03-09 9:35 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).