* [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).