Re: Linux 2.4.33-rc1

[Marcelo Tosatti <marcelo@kvack.org>]

Hi Willy,

On Wed, Jun 21, 2006 at 12:23:57AM +0200, Willy Tarreau wrote:
> On Mon, Jun 19, 2006 at 08:45:06PM -0300, Marcelo Tosatti wrote:
> > > 
> > > ---- from here ----
> > > 
> > > 
> > > > +		inode = dentry->d_inode;
> > > > +		if (inode)
> > > > +			atomic_inc(&inode->i_count);
> > > >  		error = vfs_unlink(nd.dentry->d_inode, dentry);
> > > >  	exit2:
> > > >  		dput(dentry);
> > > >  	}
> > > >  	up(&nd.dentry->d_inode->i_sem);
> > > > +	if (inode)
> > > > +		iput(inode);
> > > 
> > > ---- to here ----
> > > 
> > > I believe that nd.dentry->d_inode cannot vanish because it is protected by the
> > > down(->i_sem) before and the up(->i_sem) after. Am I right or am I missing
> > > something important ?
> > 
> > Indeed it can't, but dentry->d_inode will be set to NULL by
> > nfs_unlink->nfs_safe_remove->d_delete. Thus the problem.
> 
> What puzzles me is how are we supposed to up(&nd.dentry->d_inode->i_sem) if
> dentry->d_inode can become NULL ? 

        down(&nd.dentry->d_inode->i_sem);
        dentry = lookup_hash(&nd.last, nd.dentry);
        error = vfs_unlink(nd.dentry->d_inode, dentry);
					       ^^^^^^^

It does vfs_unlink(parent, child). The child is killed.

> simply by keeping a copy of it ? I thought
> that the down() protected the whole thing, but may be that's stupid anyway.

It does not protect from last reference going away, which is what
happens when NFS d_delete's the dentry. dentry_iput() gets called, and
maybe the last reference to "struct inode" might be gone too.

> I've been running rc1 without this patch for a few hours and during kernel
> compiles without a problem, so I'm not sure about what to think about the
> other changes which were apparently harmless too :-/

Use NFS... :)

> Well, if I resume it right, we only need to merge your patch and mine and
> it *should* be OK.
> 
> BTW, I've been reviewing the PaX patch and found *at least* one patch
> that should be merged (fix for oops). I'll send it separately, and it's
> queued in -upstream.

Merged, thanks.