Re: d_splice_alias() problem.

Neil Brown
2004-05-03T23:29:22Z

On Monday May 3, gnb@melbourne.sgi.com wrote:
> Neil Brown wrote:
> >
> > This problem can be resolved by making sure that an inode never has
> > both a connected and a disconnected dentry.
> >
> > This is already the case for directories (as they must only have one
> > dentry), but it is not the case for non-directories.
> >
> > The following patch tries to address this. It is a "technology
> > preview" in that the only testing I have done is that it compiles OK.
> >
> > Please consider reviewing it to see if it makes sense.
>
> It does, and it fixes one of the dcache bugs that was tripping my debug
> code. Here are a couple more.
>
> * Logic bug in d_splice_alias() forgets to clear the DCACHE_DISCONNECTED
> flag when a lookup connects a disconnected dentry. Fix is (relative
> to Neil's patch):

I seem to recall that this was intentional. When I first wrote the
code I wanted to leave the splicing code to just splice things, and
when the code that cared about disconnected-ness (in knfsd) discovered
that something really was connected, it would clear the bit
(find_exported_dentry does this).
However if we want to ensure that there is at-most one disconnected
dentry for an inode, we do need to set the bit more aggressively.
I'll try and review the code with that in mind. Thanks.

>
>
> * Dentry_stat.nr_unused can be be spuriously decremented when dput()
> races with __dget_unlocked(). Eventual result is nr_unused<0
> and kswapd loops. This is the problem I mentioned earlier. Note
> that this is not an NFS-specific problem. Fix is:
>
> --- linux.orig/fs/dcache.c Mon May 3 21:46:30 2004
> +++ linux/fs/dcache.c Mon May 3 21:49:07 2004
> @@ -255,8 +255,8 @@
>
> static inline struct dentry * __dget_locked(struct dentry *dentry)
> {
> - atomic_inc(&dentry->d_count);
> - if (atomic_read(&dentry->d_count) == 1) {
> + if (atomic_inc(&dentry->d_count) == 1) {

One problem with this is that (in include/asm-i386/atomic.h at least):
static __inline__ void atomic_inc(atomic_t *v)

atomic_inc returns "void".

NeilBrown