Skip to contentSkip to conversation

xfs: don't take the i_mutex in ->fsync()

PATCH In review Josef Bacik 2 participants 2 messages Jul 19, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xfs: don't take the i_mutex in ->fsync()

In pushing down the i_mutex into the ->fsync() handlers I introduced a deadlock since xfs will call xfs_file_fsync in the O_SYNC case while still holding the i_mutex case. In talking with Dave he said that xfs doesn't really need to hold the i_mutex in xfs_file_sync() so just drop the taking of i_mutex and we should be good to go. Thanks,

---
 fs/xfs/linux-2.6/xfs_file.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)
fs/xfs/linux-2.6/xfs_file.c +1 -7
index 9183f75..b928cb5 100644
@@ -144,12 +144,8 @@ xfs_file_fsync(
 	if (error)
 		return error;
 
-	mutex_lock(&inode->i_mutex);
-
-	if (XFS_FORCED_SHUTDOWN(mp)) {
-		mutex_unlock(&inode->i_mutex);
+	if (XFS_FORCED_SHUTDOWN(mp))
 		return -XFS_ERROR(EIO);
-	}
 
 	xfs_iflags_clear(ip, XFS_ITRUNCATED);
 
@@ -205,7 +201,6 @@ xfs_file_fsync(
 				XFS_FSYNC_TS_LOG_RES(mp), 0, 0, 0);
 		if (error) {
 			xfs_trans_cancel(tp, 0);
-			mutex_unlock(&inode->i_mutex);
 			return -error;
 		}
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
@@ -255,7 +250,6 @@ xfs_file_fsync(
 	    !log_flushed)
 		xfs_blkdev_issue_flush(mp->m_ddev_targp);
 
-	mutex_unlock(&inode->i_mutex);
 	return -error;
 }
 
Signature · 1 line
1.7.5.2
#1 lore raw
Quoted 5 lines · Josef Bacik

In pushing down the i_mutex into the ->fsync() handlers I introduced a deadlock since xfs will call xfs_file_fsync in the O_SYNC case while still holding the i_mutex case. In talking with Dave he said that xfs doesn't really need to hold the i_mutex in xfs_file_sync() so just drop the taking of i_mutex and we should be good to go. Thanks,

We need some serialized around the xfs_ioend_wait call to prvent new ioends coming in from starving our wait, be that i_mutex or the iolock.

The right fix for you patch for now is fixing xfs_file_aio_write to call xfs_file_fsync correctly, that is removing the filemap_write_and_wait_range call, and instead moving the fsync call into the section where we have dropped the iolock and i_mutex.

Dave and I can then later look into dropping i_mutex in fsync and replacing it with the iolock and/or finding a way to reduce the number of lock roundtrips in the O_SYNC path.