Messages in this thread | | | Date | Wed, 27 Jun 2007 16:42:08 +1000 | From | Tim Shimmin <> | Subject | 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_SHIFT 16 > #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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |