Re: xfs vs. lockdep

[Timothy Shimmin <tes@xxxxxxx>]

--On 10 October 2006 10:47:26 AM +1000 David Chinner <dgc@xxxxxxx> wrote:

> 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.
Yeah, there really seems like something would be wrong if you can't
unlock it before destroying it.
I would have thought you'd need to guarantee that you are the only
one with access to it before destroying it otherwise there'd be problems :)
(Which as you say we do)

This one rings a bell. I seem to recall multiple places where we destroy
without releasing the lock first.
And I vaguely remember Nathan mentioning that this was causing grief
for lockdep:)

--Tim