I can't find any obvious reason why we would want to nest i_mutex
inside the pipe lock, but two good reasons speak against it:
- with i_mutex inside the pipe lock we have to unlock it every time we
iterate to the next buffer, which prevents i_mutex from guaranteeing
write atomicy similar to write(2), and also is ineffiecient for
filesystems like ocfs2 that have additional cluster locking along
i_mutex.
- the ordering of pipe_lock outside i_mutex makes it very hard to
share code with filesystems that have additional inode-wide locks
that need to be taken in the right order with i_mutex.
In addition to changing the lock order this patch also removes the
useless I_MUTEX_CHILD annotations.
Signed-off-by: Christoph Hellwig <hch@xxxxxx>
---
fs/ocfs2/file.c | 20 +++++++++++---------
fs/splice.c | 5 +++--
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index a77ef6e..c68e111 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2461,6 +2461,12 @@ static ssize_t ocfs2_file_splice_write(struct
pipe_inode_info *pipe,
out->f_path.dentry->d_name.len,
out->f_path.dentry->d_name.name, len);
+ mutex_lock(&inode->i_mutex);
+ ret = ocfs2_rw_lock(inode, 1);
+ if (ret < 0) {
+ mlog_errno(ret);
+ goto out_unlock_inode;
+ }
pipe_lock(pipe);
splice_from_pipe_begin(&sd);
@@ -2469,20 +2475,16 @@ static ssize_t ocfs2_file_splice_write(struct
pipe_inode_info *pipe,
if (ret <= 0)
break;
- mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD);
- ret = ocfs2_rw_lock(inode, 1);
- if (ret < 0)
- mlog_errno(ret);
- else {
- ret = ocfs2_splice_to_file(pipe, out, &sd);
- ocfs2_rw_unlock(inode, 1);
- }
- mutex_unlock(&inode->i_mutex);
+ ret = ocfs2_splice_to_file(pipe, out, &sd);
} while (ret > 0);
splice_from_pipe_end(pipe, &sd);
pipe_unlock(pipe);
+ ocfs2_rw_unlock(inode, 1);
+out_unlock_inode:
+ mutex_unlock(&inode->i_mutex);
+
if (sd.num_spliced)
ret = sd.num_spliced;
diff --git a/fs/splice.c b/fs/splice.c
index fcb459d..4fb6c1f 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1005,6 +1005,8 @@ generic_file_splice_write(struct pipe_inode_info *pipe,
struct file *out,
};
ssize_t ret;
+ mutex_lock(&inode->i_mutex);
+
pipe_lock(pipe);
splice_from_pipe_begin(&sd);
@@ -1013,7 +1015,6 @@ generic_file_splice_write(struct pipe_inode_info *pipe,
struct file *out,
if (ret <= 0)
break;
- mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD);
ret = file_remove_suid(out);
if (!ret) {
ret = file_update_time(out);
@@ -1021,11 +1022,11 @@ generic_file_splice_write(struct pipe_inode_info *pipe,
struct file *out,
ret = splice_from_pipe_feed(pipe, &sd,
pipe_to_file);
}
- mutex_unlock(&inode->i_mutex);
} while (ret > 0);
splice_from_pipe_end(pipe, &sd);
pipe_unlock(pipe);
+ mutex_unlock(&inode->i_mutex);
if (sd.num_spliced)
ret = sd.num_spliced;
--
1.7.10.4
|