> David Chinner <[email protected]> wrote:
>
> +/*
> + * Shrink the dentry LRU on a given superblock.
> + *
> + * If flags is non-zero, we need to do special processing based on
> + * which flags are set. This means we don't need to maintain multiple
> + * similar copies of this loop.
> + */
> +static void __shrink_dcache_sb(struct super_block *sb, int *count, int
> flags)
> +{
> + struct dentry *dentry;
> + int cnt = *count;
> +
> + spin_lock(&dcache_lock);
> + while (!list_empty(&sb->s_dentry_lru) && cnt--) {
> + dentry = list_entry(sb->s_dentry_lru.prev,
> + struct dentry, d_lru);
> + dentry_lru_del_init(dentry);
> + BUG_ON(dentry->d_sb != sb);
> + prefetch(sb->s_dentry_lru.prev);
> +
> + spin_lock(&dentry->d_lock);
> + /*
> + * We found an inuse dentry which was not removed from
> + * the LRU because of laziness during lookup. Do not free
> + * it - just keep it off the LRU list.
> + */
> + if (atomic_read(&dentry->d_count)) {
> + spin_unlock(&dentry->d_lock);
> + continue;
> + }
> + /*
> + * If we are honouring the DCACHE_REFERENCED flag and the
> + * dentry has this flag set, don't free it. Clear the flag
> + * and put it back on the LRU
> + */
> + if ((flags & DCACHE_REFERENCED) &&
> + (dentry->d_flags & DCACHE_REFERENCED)) {
> + dentry->d_flags &= ~DCACHE_REFERENCED;
> + dentry_lru_add(dentry);
> + spin_unlock(&dentry->d_lock);
> + continue;
> + }
> + prune_one_dentry(dentry);
> + }
> + spin_unlock(&dcache_lock);
> + *count = cnt;
> +}
> +
> /**
> * shrink_dcache_sb - shrink dcache for a superblock
> * @sb: superblock
> @@ -507,44 +529,9 @@ static void prune_dcache(int count, stru
> * is used to free the dcache before unmounting a file
> * system
> */
> -
> void shrink_dcache_sb(struct super_block * sb)
> {
> - struct list_head *tmp, *next;
> - struct dentry *dentry;
> -
> - /*
> - * Pass one ... move the dentries for the specified
> - * superblock to the most recent end of the unused list.
> - */
> - spin_lock(&dcache_lock);
> - list_for_each_safe(tmp, next, &dentry_unused) {
> - dentry = list_entry(tmp, struct dentry, d_lru);
> - if (dentry->d_sb != sb)
> - continue;
> - list_move(tmp, &dentry_unused);
> - }
> -
> - /*
> - * Pass two ... free the dentries for this superblock.
> - */
> -repeat:
> - list_for_each_safe(tmp, next, &dentry_unused) {
> - dentry = list_entry(tmp, struct dentry, d_lru);
> - if (dentry->d_sb != sb)
> - continue;
> - dentry_stat.nr_unused--;
> - list_del_init(tmp);
> - spin_lock(&dentry->d_lock);
> - if (atomic_read(&dentry->d_count)) {
> - spin_unlock(&dentry->d_lock);
> - continue;
> - }
> - prune_one_dentry(dentry);
> - cond_resched_lock(&dcache_lock);
> - goto repeat;
> - }
> - spin_unlock(&dcache_lock);
> + __shrink_dcache_sb(sb, &sb->s_dentry_lru_nr, 0);
> }
This doesn't prune all the dentries on the unused list. The parents of the
pruned dentries are added to the unused list. Therefore just shrinking
sb->s_dentry_lru_nr dentries isn't enough.
Jan
On Tue, May 30, 2006 at 05:04:38PM +0200, Jan Blunck wrote:
> > David Chinner <[email protected]> wrote:
> > -
> > void shrink_dcache_sb(struct super_block * sb)
> > {
....
> > + __shrink_dcache_sb(sb, &sb->s_dentry_lru_nr, 0);
> > }
>
> This doesn't prune all the dentries on the unused list. The parents of the
> pruned dentries are added to the unused list. Therefore just shrinking
> sb->s_dentry_lru_nr dentries isn't enough.
Yes, you are right, Jan. I'm surprised I didn't see problems due to this.
The original patch got this right by shrinking in this case until the list
was empty. I'll wrap this one in a while loop...
Cheers,
Dave.
--
Dave Chinner
R&D Software Enginner
SGI Australian Software Group
David Chinner wrote:
> On Tue, May 30, 2006 at 05:04:38PM +0200, Jan Blunck wrote:
>>> David Chinner <[email protected]> wrote:
>>> -
>>> void shrink_dcache_sb(struct super_block * sb)
>>> {
> ....
>>> + __shrink_dcache_sb(sb, &sb->s_dentry_lru_nr, 0);
>>> }
>> This doesn't prune all the dentries on the unused list. The parents of the
>> pruned dentries are added to the unused list. Therefore just shrinking
>> sb->s_dentry_lru_nr dentries isn't enough.
>
> Yes, you are right, Jan. I'm surprised I didn't see problems due to this.
> The original patch got this right by shrinking in this case until the list
> was empty. I'll wrap this one in a while loop...
>
> Cheers,
>
> Dave.
Good catch,
I suspect the reason why the problem never showed up is because select_parent()
would take care of ensuring that the parent entries move to LRU list (this is
the case of regular umounts, which do not call shrink_dcache_sb() directly).
Maybe even the dentry_unused list should be per-superblock now.
--
Regards,
Balbir Singh,
Linux Technology Center,
IBM Software Labs