I like the idea but the i_lock_state thing needs work. See comments below.
Christoph Hellwig wrote:
XFS inodes are locked via the xfs_ilock family of functions which
internally use a rw_semaphore wrapper into an abstraction called
mrlock_t. The mrlock_t should be purely internal to xfs_ilock functions
but leaks through to the callers via various lock state asserts.
This patch:
- adds new xfs_isilocked abstraction to make the lock state asserts
fits into the xfs_ilock API family
- opencodes the mrlock wrappers in the xfs_ilock family of functions
- makes the state tracking debug-only and merged into a single state
word
- remove superflous flags to the xfs_ilock family of functions
This kills 8 bytes per inode for non-debug builds, which would e.g.
be the space for ACL caching on 32bit systems.
Signed-off-by: Christoph Hellwig <hch@xxxxxx>
*/
void
-xfs_ilock(xfs_inode_t *ip,
- uint lock_flags)
+xfs_ilock(
+ xfs_inode_t *ip,
+ uint lock_flags)
{
/*
* You can't set both SHARED and EXCL for the same lock,
@@ -608,16 +608,19 @@ xfs_ilock(xfs_inode_t *ip,
ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_DEP_MASK)) == 0);
if (lock_flags & XFS_IOLOCK_EXCL) {
- mrupdate_nested(&ip->i_iolock, XFS_IOLOCK_DEP(lock_flags));
+ down_write_nested(&ip->i_iolock, XFS_IOLOCK_DEP(lock_flags));
} else if (lock_flags & XFS_IOLOCK_SHARED) {
- mraccess_nested(&ip->i_iolock, XFS_IOLOCK_DEP(lock_flags));
+ down_read_nested(&ip->i_iolock, XFS_IOLOCK_DEP(lock_flags));
}
if (lock_flags & XFS_ILOCK_EXCL) {
- mrupdate_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
+ down_write_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
} else if (lock_flags & XFS_ILOCK_SHARED) {
- mraccess_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
+ down_read_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
}
xfs_ilock_trace(ip, 1, lock_flags, (inst_t *)__return_address);
+#ifdef DEBUG
+ ip->i_lock_state |= (lock_flags & (XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
+#endif
This seems racy - if we only acquire one of ilock/iolock exclusive and another
thread acquires the other lock exclusive then we'll race setting i_lock_state.
}
/*
@@ -634,11 +637,12 @@ xfs_ilock(xfs_inode_t *ip,
*
*/
int
-xfs_ilock_nowait(xfs_inode_t *ip,
- uint lock_flags)
+xfs_ilock_nowait(
+ xfs_inode_t *ip,
+ uint lock_flags)
{
- int iolocked;
- int ilocked;
+ int iolocked;
+ int ilocked;
/*
* You can't set both SHARED and EXCL for the same lock,
@@ -653,35 +657,36 @@ xfs_ilock_nowait(xfs_inode_t *ip,
iolocked = 0;
if (lock_flags & XFS_IOLOCK_EXCL) {
- iolocked = mrtryupdate(&ip->i_iolock);
- if (!iolocked) {
+ iolocked = down_write_trylock(&ip->i_iolock);
+ if (!iolocked)
return 0;
- }
} else if (lock_flags & XFS_IOLOCK_SHARED) {
- iolocked = mrtryaccess(&ip->i_iolock);
- if (!iolocked) {
+ iolocked = down_read_trylock(&ip->i_iolock);
+ if (!iolocked)
return 0;
- }
}
+
if (lock_flags & XFS_ILOCK_EXCL) {
- ilocked = mrtryupdate(&ip->i_lock);
- if (!ilocked) {
- if (iolocked) {
- mrunlock(&ip->i_iolock);
- }
- return 0;
- }
+ ilocked = down_write_trylock(&ip->i_lock);
+ if (!ilocked)
+ goto out_ilock_fail;
} else if (lock_flags & XFS_ILOCK_SHARED) {
- ilocked = mrtryaccess(&ip->i_lock);
- if (!ilocked) {
- if (iolocked) {
- mrunlock(&ip->i_iolock);
- }
- return 0;
- }
+ ilocked = down_read_trylock(&ip->i_lock);
+ if (!ilocked)
+ goto out_ilock_fail;
}
xfs_ilock_trace(ip, 2, lock_flags, (inst_t *)__return_address);
+#ifdef DEBUG
+ ip->i_lock_state |= (lock_flags & (XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
+#endif
Same deal, setting i_lock_state looks racy.
return 1;
+
+ out_ilock_fail:
+ if (lock_flags & XFS_IOLOCK_EXCL)
+ up_write(&ip->i_iolock);
+ else if (lock_flags & XFS_IOLOCK_SHARED)
+ up_read(&ip->i_iolock);
+ return 0;
}
/*
@@ -697,8 +702,9 @@ xfs_ilock_nowait(xfs_inode_t *ip,
*
*/
void
-xfs_iunlock(xfs_inode_t *ip,
- uint lock_flags)
+xfs_iunlock(
+ xfs_inode_t *ip,
+ uint lock_flags)
{
/*
* You can't set both SHARED and EXCL for the same lock,
@@ -711,35 +717,33 @@ xfs_iunlock(xfs_inode_t *ip,
(XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_IUNLOCK_NONOTIFY |
XFS_LOCK_DEP_MASK)) == 0);
+ ASSERT(ip->i_lock_state & lock_flags);
This assertion will always fail when *_SHARED flags are present in
lock_flags.
ASSERT(lock_flags != 0);
- if (lock_flags & (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)) {
- ASSERT(!(lock_flags & XFS_IOLOCK_SHARED) ||
- (ismrlocked(&ip->i_iolock, MR_ACCESS)));
- ASSERT(!(lock_flags & XFS_IOLOCK_EXCL) ||
- (ismrlocked(&ip->i_iolock, MR_UPDATE)));
- mrunlock(&ip->i_iolock);
- }
-
- if (lock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)) {
- ASSERT(!(lock_flags & XFS_ILOCK_SHARED) ||
- (ismrlocked(&ip->i_lock, MR_ACCESS)));
- ASSERT(!(lock_flags & XFS_ILOCK_EXCL) ||
- (ismrlocked(&ip->i_lock, MR_UPDATE)));
- mrunlock(&ip->i_lock);
+ if (lock_flags & XFS_IOLOCK_EXCL)
+ up_write(&ip->i_iolock);
+ else if (lock_flags & XFS_IOLOCK_SHARED)
+ up_read(&ip->i_iolock);
+
+ if (lock_flags & XFS_ILOCK_EXCL)
+ up_write(&ip->i_lock);
+ else if (lock_flags & (XFS_ILOCK_SHARED))
+ up_read(&ip->i_lock);
+ if ((lock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)) &&
+ !(lock_flags & XFS_IUNLOCK_NONOTIFY) && ip->i_itemp) {
/*
* Let the AIL know that this item has been unlocked in case
* it is in the AIL and anyone is waiting on it. Don't do
* this if the caller has asked us not to.
*/
- if (!(lock_flags & XFS_IUNLOCK_NONOTIFY) &&
- ip->i_itemp != NULL) {
- xfs_trans_unlocked_item(ip->i_mount,
- (xfs_log_item_t*)(ip->i_itemp));
- }
+ xfs_trans_unlocked_item(ip->i_mount,
+ (xfs_log_item_t *)ip->i_itemp);
}
xfs_ilock_trace(ip, 3, lock_flags, (inst_t *)__return_address);
+#ifdef DEBUG
+ ip->i_lock_state &= ~(lock_flags & (XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
+#endif
This seems racy - another thread could have acquired the ilock/iolock and set
the flags before we get here and remove the flags. Unsetting the flags before
we release the lock would still be racy if both iolock and ilock are released
at the same time by different threads.
}
/*
@@ -747,21 +751,42 @@ xfs_iunlock(xfs_inode_t *ip,
* if it is being demoted.
*/
void
-xfs_ilock_demote(xfs_inode_t *ip,
- uint lock_flags)
+xfs_ilock_demote(
+ xfs_inode_t *ip,
+ uint lock_flags)
{
ASSERT(lock_flags & (XFS_IOLOCK_EXCL|XFS_ILOCK_EXCL));
ASSERT((lock_flags & ~(XFS_IOLOCK_EXCL|XFS_ILOCK_EXCL)) == 0);
+ ASSERT(ip->i_lock_state & lock_flags);
- if (lock_flags & XFS_ILOCK_EXCL) {
- ASSERT(ismrlocked(&ip->i_lock, MR_UPDATE));
- mrdemote(&ip->i_lock);
- }
- if (lock_flags & XFS_IOLOCK_EXCL) {
- ASSERT(ismrlocked(&ip->i_iolock, MR_UPDATE));
- mrdemote(&ip->i_iolock);
- }
+ if (lock_flags & XFS_ILOCK_EXCL)
+ downgrade_write(&ip->i_lock);
+ if (lock_flags & XFS_IOLOCK_EXCL)
+ downgrade_write(&ip->i_iolock);
+
+#ifdef DEBUG
+ ip->i_lock_state &= ~lock_flags;
+#endif
+}
+
+#ifdef DEBUG
+/*
+ * Debug-only routine, without additional rw_semaphore APIs, we can
+ * now only answer requests regarding whether we hold the lock for write
+ * (reader state is outside our visibility, we only track writer state).
+ *
+ * Note: means !xfs_isilocked would give false positives, so don't do that.
+ */
+int
+xfs_isilocked(
+ xfs_inode_t *ip,
+ uint lock_flags)
+{
+ if (lock_flags & (XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL))
+ return (ip->i_lock_state & lock_flags);
+ return 1;
}
+#endif
Splitting i_lock_state into two separate fields - one for each lock - and
unsetting the fields prior to releasing/demoting the lock might be enough
to solve all these races.
|