Re: [xfs-masters] Re: [BUG] Lockdep warning with XFS on 2.6.22-rc6
Patch looks good, Dave.
(though, I stuffed up reviewing that bit of code previously:-)
Oh, previous typo: s/inodes at the some time/inodes at the same time/
--Tim
David Chinner wrote:
On Tue, Jun 26, 2007 at 11:35:20AM +0200, Jarek Poplawski wrote:
On 26-06-2007 04:16, David Chinner wrote:
It does both - parent-first/child-second and ascending inode # order,
which is where the problem is. standing alone, these seem fine, but
they don't appear to work when the child has a lower inode number
than the parent.
...
>From xfs_inode.h:
/*
* Flags for lockdep annotations.
*
* XFS_I[O]LOCK_PARENT - for operations that require locking two inodes
* (ie directory operations that require locking a directory inode and
* an entry inode). The first inode gets locked with this flag so it
* gets a lockdep subclass of 1 and the second lock will have a lockdep
* subclass of 0.
*
* XFS_I[O]LOCK_INUMORDER - for locking several inodes at the some time
* with xfs_lock_inodes(). This flag is used as the starting subclass
* and each subsequent lock acquired will increment the subclass by one.
* So the first lock acquired will have a lockdep subclass of 2, the
* second lock will have a lockdep subclass of 3, and so on.
*/
I don't know xfs code, and probably miss something, but it seems
there could be some inconsistency: lockdep warning shows mr_lock/1
taken both before and after mr_lock (i.e. /0). According to the
above comment there should be always 1 before 0...
That just fired some rusty neurons.
#define XFS_IOLOCK_SHIFT16
#define XFS_IOLOCK_PARENT (1 << XFS_IOLOCK_SHIFT)
#define XFS_IOLOCK_INUMORDER(2 << XFS_IOLOCK_SHIFT)
#define XFS_ILOCK_SHIFT 24
#define XFS_ILOCK_PARENT(1 << XFS_ILOCK_SHIFT)
#define XFS_ILOCK_INUMORDER (2 << XFS_ILOCK_SHIFT)
So, in a lock_mode parameter, the upper 8 bits are for the ILOCK lockdep
subclass, and the 16..23 bits are for the IOLOCK lockdep subclass.
Where do we add them?
static inline int
xfs_lock_inumorder(int lock_mode, int subclass)
{
if (lock_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL))
lock_mode |= (subclass + XFS_IOLOCK_INUMORDER) <<
XFS_IOLOCK_SHIFT;
if (lock_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL))
lock_mode |= (subclass + XFS_ILOCK_INUMORDER) <<
XFS_ILOCK_SHIFT;
return lock_mode;
}
OH, look at those nice overflow bugs in that in that code. We shift
the XFS_IOLOCK_INUMORDER and XFS_ILOCK_INUMORDER bits out the far
side of the lock_mode variable result in lock subclasses of 0-3 instead
of 2-5
Bugger, eh?
Patch below should fix this (untested).
Jarek - thanks for pointing what I should have seen earlier.
Cheers,
Dave.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [xfs-masters] Re: [BUG] Lockdep warning with XFS on 2.6.22-rc6
Patch looks good, Dave.
(though, I stuffed up reviewing that bit of code previously:-)
Oh, previous typo: s/inodes at the some time/inodes at the same time/
--Tim
David Chinner wrote:
On Tue, Jun 26, 2007 at 11:35:20AM +0200, Jarek Poplawski wrote:
On 26-06-2007 04:16, David Chinner wrote:
It does both - parent-first/child-second and ascending inode # order,
which is where the problem is. standing alone, these seem fine, but
they don't appear to work when the child has a lower inode number
than the parent.
...
From xfs_inode.h:
/*
* Flags for lockdep annotations.
*
* XFS_I[O]LOCK_PARENT - for operations that require locking two inodes
* (ie directory operations that require locking a directory inode and
* an entry inode). The first inode gets locked with this flag so it
* gets a lockdep subclass of 1 and the second lock will have a lockdep
* subclass of 0.
*
* XFS_I[O]LOCK_INUMORDER - for locking several inodes at the some time
* with xfs_lock_inodes(). This flag is used as the starting subclass
* and each subsequent lock acquired will increment the subclass by one.
* So the first lock acquired will have a lockdep subclass of 2, the
* second lock will have a lockdep subclass of 3, and so on.
*/
I don't know xfs code, and probably miss something, but it seems
there could be some inconsistency: lockdep warning shows mr_lock/1
taken both before and after mr_lock (i.e. /0). According to the
above comment there should be always 1 before 0...
That just fired some rusty neurons.
#define XFS_IOLOCK_SHIFT16
#define XFS_IOLOCK_PARENT (1 XFS_IOLOCK_SHIFT)
#define XFS_IOLOCK_INUMORDER(2 XFS_IOLOCK_SHIFT)
#define XFS_ILOCK_SHIFT 24
#define XFS_ILOCK_PARENT(1 XFS_ILOCK_SHIFT)
#define XFS_ILOCK_INUMORDER (2 XFS_ILOCK_SHIFT)
So, in a lock_mode parameter, the upper 8 bits are for the ILOCK lockdep
subclass, and the 16..23 bits are for the IOLOCK lockdep subclass.
Where do we add them?
static inline int
xfs_lock_inumorder(int lock_mode, int subclass)
{
if (lock_mode (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL))
lock_mode |= (subclass + XFS_IOLOCK_INUMORDER)
XFS_IOLOCK_SHIFT;
if (lock_mode (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL))
lock_mode |= (subclass + XFS_ILOCK_INUMORDER)
XFS_ILOCK_SHIFT;
return lock_mode;
}
OH, look at those nice overflow bugs in that in that code. We shift
the XFS_IOLOCK_INUMORDER and XFS_ILOCK_INUMORDER bits out the far
side of the lock_mode variable result in lock subclasses of 0-3 instead
of 2-5
Bugger, eh?
Patch below should fix this (untested).
Jarek - thanks for pointing what I should have seen earlier.
Cheers,
Dave.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
