Re: xfs vs. lockdep

[Timothy Shimmin <tes@sgi.com>]

--On 10 October 2006 10:47:26 AM +1000 David Chinner <dgc@sgi.com> 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