xfs
[Top] [All Lists]

[PATCH 2/5] splice: nest i_mutex outside pipe_lock

To: Al Viro <viro@xxxxxxxxxxxxxxxxxx>, Jens Axboe <axboe@xxxxxxxxx>, Mark Fasheh <mfasheh@xxxxxxxx>, Joel Becker <jlbec@xxxxxxxxxxxx>
Subject: [PATCH 2/5] splice: nest i_mutex outside pipe_lock
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 12 Dec 2013 10:15:01 -0800
Cc: linux-fsdevel@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
References: <20131212181459.994196463@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: quilt/0.60-1
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


<Prev in Thread] Current Thread [Next in Thread>