Message ID | 20121121005626.GC10507@quack.suse.cz |
---|---|
State | Accepted, archived |
Headers | show |
Jan Kara <jack@suse.cz> writes: >> Just to be clear, are you saying you would like me to remove the >> mutex_lock/unlock pair from ext4_sync_file? (I had already factored out >> the common code between this new code path and the fsync path in my tree.) > Yes, after some thinking I came to that conclusion. We actually need to > keep i_mutex around ext4_flush_unwritten_io() to avoid livelocks but the > rest doesn't need it. The change should be definitely a separate patch just > in case there's something subtle I missed and we need to bisect in > future... I've attached a patch for that so that blame for bugs goes my way > ;) Compile tested only so far. I'll give it some more testing overnight. Great, thanks Jan! I'll include this in the next posting. Cheers, Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 21-11-12 09:09:41, Jeff Moyer wrote: > Jan Kara <jack@suse.cz> writes: > > >> Just to be clear, are you saying you would like me to remove the > >> mutex_lock/unlock pair from ext4_sync_file? (I had already factored out > >> the common code between this new code path and the fsync path in my tree.) > > Yes, after some thinking I came to that conclusion. We actually need to > > keep i_mutex around ext4_flush_unwritten_io() to avoid livelocks but the > > rest doesn't need it. The change should be definitely a separate patch just > > in case there's something subtle I missed and we need to bisect in > > future... I've attached a patch for that so that blame for bugs goes my way > > ;) Compile tested only so far. I'll give it some more testing overnight. > > Great, thanks Jan! I'll include this in the next posting. OK, patch passed xfstests and a test banging one file with random IO and fsyncs from 8 processes (in data=ordered, data=journal, and nojournal modes). So it seems I didn't miss anything substantial. So ship it! ;) Honza
From 98f02e76b90e278e9688b4311a8889cec7095601 Mon Sep 17 00:00:00 2001 From: Jan Kara <jack@suse.cz> Date: Wed, 21 Nov 2012 01:46:51 +0100 Subject: [PATCH] ext4: Reduce i_mutex usage in ext4_file_sync() ext4_file_sync() needs i_mutex only to avoid livelocks of ext4_flush_unwritten_io() all other code doesn't need it. In particular syncing of inode & metadata in non-journal case is safe (writeback doesn't hold i_mutex either) and forcing of transaction commits doesn't need i_mutex either (there's nothing inode specific in that code apart from grabbing transaction ids from the inode). So shorten the span where i_mutex is held. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/fsync.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c index be1d89f..2268114 100644 --- a/fs/ext4/fsync.c +++ b/fs/ext4/fsync.c @@ -113,8 +113,6 @@ static int __sync_inode(struct inode *inode, int datasync) * * What we do is just kick off a commit and wait on it. This will snapshot the * inode to disk. - * - * i_mutex lock is held when entering and exiting this function */ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync) @@ -133,12 +131,13 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync) ret = filemap_write_and_wait_range(inode->i_mapping, start, end); if (ret) return ret; - mutex_lock(&inode->i_mutex); if (inode->i_sb->s_flags & MS_RDONLY) goto out; + mutex_lock(&inode->i_mutex); ret = ext4_flush_unwritten_io(inode); + mutex_unlock(&inode->i_mutex); if (ret < 0) goto out; @@ -180,7 +179,6 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync) ret = err; } out: - mutex_unlock(&inode->i_mutex); trace_ext4_sync_file_exit(inode, ret); return ret; } -- 1.7.1