Re: xfs vs. lockdep

[David Chinner <dgc@sgi.com>]

On Mon, Oct 09, 2006 at 12:58:58PM -0500, Eric Sandeen wrote:
> FC6 kernels are oopsing when lockdep & memory debugging are turned on,
> looks like due to this code:
> 
> xfs_ireclaim(xfs_inode_t *ip)
> {
>    ...
>         /*
>          * Here we do a spurious inode lock in order to coordinate with
>          * xfs_sync().  This is because xfs_sync() references the inodes
>          * in the mount list without taking references on the corresponding
>          * vnodes.  We make that OK here by ensuring that we wait until
>          * the inode is unlocked in xfs_sync() before we go ahead and
>          * free it.  We get both the regular lock and the io lock because
>          * the xfs_sync() code may need to drop the regular one but will
>          * still hold the io lock.
>          */
>         xfs_ilock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
> ...
>         /*
>          * Free all memory associated with the inode.
>          */
>         xfs_idestroy(ip);
> }
> 
> So, lock & free.  This frees memory that lockdep is still pointing to,
> and tries to use later.
> 
> Calling xfs_iunlock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL); just before
> xfs_idestroy seems to solve it, but is this safe...?

It should be - we call xfs_iextract() before the xfs_ilock() call
shown above. That means the inode has been removed from the mount
list when we take the locks. Once the inode has been removed
from the mount list, the only possible current user is xfs_sync_inodes(),
and it will only be referencing the inode if it is currently working
on the inode. If it is working on the inode, then it will be holding
at least one of the inode locks.

Hence by the time we have the lock here in xfs_ireclaim we have guaranteed
that there are no other outstanding references and no new references
can occur. Therefore it should be safe to drop the lock before destroying
it.

There have been other bits of code in XFS where locks have been taken
just before item destroy. IIRC, one even had a comment explaining it
was safe to do this that was longer than just putting the unlock call
in the code. :/

FWIW, we call mrfree() on both the ilock and the iolock, but these are
#defined to null statements. If there is a destructor for the underlying
lock type, we probably should call that in mrfree() so the debugging code
can catch these probelms that only trigger in debug code.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group