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@xxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxxxxxx
--
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
