> Somewhere in among the comments (thanks), I realised that I was only
> closing half the race. I had tried to make sure there were no stray
> references to any dentries, but there is still the inode which is
> being iput which can cause problem.
>
> The following patch takes a totally different approach, is based on an
> idea from Jan Kara, and is much less intrusive.
>
> We:
> - keep track of "who" is calling prune_dcache, and when a filesystem
> is being unmounted (s_root == NULL) we only allow the unmount thread
> to prune dentries.
> - keep track of how many dentries are in the process of having
> dentry_iput called on them for pruning
> - don't allow umount to proceed until that count hits zero
> - bias the count this way and that to make sure we get a wake_up at
> the right time
> - reuse 's_wait_unfrozen' to wait on the iput to complete.
>
> Again, I'm very keen on feedback. This race is very hard to trigger,
> so code review is the only real way to evaluate that patch.
>
> Thanks,
> NeilBrown
>
The code changes look big, have you looked at
http://marc.theaimsgroup.com/?l=linux-kernel&m=113817279225962&w=2
Some top of the head feedback below. Will try and do a detailed review later.
>
> Signed-off-by: Neil Brown <[email protected]>
>
> ### Diffstat output
> ./fs/dcache.c | 17 +++++++++++++----
> ./fs/super.c | 11 +++++++++++
> ./include/linux/fs.h | 2 ++
> 3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff ./fs/dcache.c~current~ ./fs/dcache.c
> --- ./fs/dcache.c~current~ 2006-03-06 16:54:59.000000000 +1100
> +++ ./fs/dcache.c 2006-03-06 16:55:33.000000000 +1100
> @@ -366,6 +366,7 @@ static inline void prune_one_dentry(stru
> {
> struct dentry * parent;
>
> + dentry->d_sb->s_pending_iputs ++;
> __d_drop(dentry);
> list_del(&dentry->d_u.d_child);
> dentry_stat.nr_dentry--; /* For d_free, below */
> @@ -375,6 +376,9 @@ static inline void prune_one_dentry(stru
> if (parent != dentry)
> dput(parent);
> spin_lock(&dcache_lock);
> + dentry->d_sb->s_pending_iputs --;
> + if (dentry->d_sb->s_pending_iputs < 0)
> + wake_up(&dentry->d_sb->s_wait_unfrozen);
> }
>
> /**
> @@ -390,7 +394,7 @@ static inline void prune_one_dentry(stru
> * all the dentries are in use.
> */
>
> -static void prune_dcache(int count)
> +static void prune_dcache(int count, struct dentry *parent)
> {
> spin_lock(&dcache_lock);
> for (; count ; count--) {
> @@ -407,6 +411,11 @@ static void prune_dcache(int count)
> dentry_stat.nr_unused--;
> dentry = list_entry(tmp, struct dentry, d_lru);
>
> + if (dentry->d_sb->s_root == NULL &&
> + (parent == NULL ||
> + parent->d_sb != dentry->d_sb))
> + continue;
> +
> spin_lock(&dentry->d_lock);
> /*
> * We found an inuse dentry which was not removed from
> @@ -635,7 +644,7 @@ void shrink_dcache_parent(struct dentry
> int found;
>
> while ((found = select_parent(parent)) != 0)
> - prune_dcache(found);
> + prune_dcache(found, parent);
> }
>
> /**
> @@ -673,7 +682,7 @@ void shrink_dcache_anon(struct hlist_hea
> }
> }
> spin_unlock(&dcache_lock);
> - prune_dcache(found);
> + prune_dcache(found, NULL);
> } while(found);
> }
>
> @@ -694,7 +703,7 @@ static int shrink_dcache_memory(int nr,
> if (nr) {
> if (!(gfp_mask & __GFP_FS))
> return -1;
> - prune_dcache(nr);
> + prune_dcache(nr, NULL);
> }
> return (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
> }
>
> diff ./fs/super.c~current~ ./fs/super.c
> --- ./fs/super.c~current~ 2006-03-06 16:54:59.000000000 +1100
> +++ ./fs/super.c 2006-03-06 16:57:19.000000000 +1100
> @@ -230,7 +230,18 @@ void generic_shutdown_super(struct super
> struct super_operations *sop = sb->s_op;
>
> if (root) {
> + spin_lock(&dcache_lock);
> + /* disable stray dputs */
> sb->s_root = NULL;
> +
> + /* trigger a wake_up */
> + sb->s_pending_iputs --;
> + spin_unlock(&dcache_lock);
> + wait_event(sb->s_wait_unfrozen,
> + sb->s_pending_iputs < 0);
> + /* avoid further wakeups */
> + sb->s_pending_iputs = 65000;
This looks a bit ugly, what is 65000?
> +
> shrink_dcache_parent(root);
> shrink_dcache_anon(&sb->s_anon);
> dput(root);
>
> diff ./include/linux/fs.h~current~ ./include/linux/fs.h
> --- ./include/linux/fs.h~current~ 2006-03-06 16:54:59.000000000 +1100
> +++ ./include/linux/fs.h 2006-03-06 12:49:55.000000000 +1100
> @@ -833,6 +833,8 @@ struct super_block {
> struct hlist_head s_anon; /* anonymous dentries for (nfs) exporting */
> struct list_head s_files;
>
> + int s_pending_iputs;
> +
> struct block_device *s_bdev;
> struct list_head s_instances;
> struct quota_info s_dquot; /* Diskquota specific options */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
