xfs
[Top] [All Lists]

[PATCH 2/2] xfs: fix splice/direct-IO deadlock

To: linux-fsdevel@xxxxxxxxxxxxxxx
Subject: [PATCH 2/2] xfs: fix splice/direct-IO deadlock
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 28 Nov 2012 13:12:48 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1354068768-4316-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1354068768-4316-1-git-send-email-david@xxxxxxxxxxxxx>
From: Dave Chinner <dchinner@xxxxxxxxxx>

lockdep reports splice vs direct-io write lock inversions due to
generic_file_splice_write() taking the inode->i_mutex inside
XFS_IOLOCK_EXCL context. These lock contexts are inverted, hence can
deadlock.  Remove the XFS_IOLOCK_EXCL locking context from the outer
function and drive it inwards to the actor function that only locks
the inode when the lock is really needed,

Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
---
 fs/xfs/xfs_file.c |   40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 2e79c54..21b6f0b 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -39,6 +39,7 @@
 #include <linux/dcache.h>
 #include <linux/falloc.h>
 #include <linux/pagevec.h>
+#include <linux/splice.h>
 
 static const struct vm_operations_struct xfs_file_vm_ops;
 
@@ -344,13 +345,33 @@ xfs_file_splice_read(
        return ret;
 }
 
+static ssize_t
+xfs_file_splice_write_actor(
+       struct pipe_inode_info  *pipe,
+       struct splice_desc      *sd)
+{
+       struct file             *out = sd->u.file;
+       struct xfs_inode        *ip = XFS_I(out->f_mapping->host);
+       ssize_t ret;
+
+       xfs_rw_ilock(ip, XFS_IOLOCK_EXCL);
+       ret = file_remove_suid(out);
+       if (!ret) {
+               ret = file_update_time(out);
+               if (!ret)
+                       ret = splice_from_pipe_feed(pipe, sd, pipe_to_file);
+       }
+       xfs_rw_iunlock(ip, XFS_IOLOCK_EXCL);
+
+       return ret;
+}
+
 /*
- * xfs_file_splice_write() does not use xfs_rw_ilock() because
- * generic_file_splice_write() takes the i_mutex itself. This, in theory,
- * couuld cause lock inversions between the aio_write path and the splice path
- * if someone is doing concurrent splice(2) based writes and write(2) based
- * writes to the same inode. The only real way to fix this is to re-implement
- * the generic code here with correct locking orders.
+ * xfs_file_splice_write() does not use the generic file splice write path
+ * because that takes the i_mutex, causing lock inversions with the IOLOCK.
+ * Instead, we call splice_write_to_file() directly with our own actor that 
does
+ * not take the i_mutex. This allows us to use the xfs_rw_ilock() functions 
like
+ * the rest of the code and hence avoid lock inversions and deadlocks.
  */
 STATIC ssize_t
 xfs_file_splice_write(
@@ -373,15 +394,12 @@ xfs_file_splice_write(
        if (XFS_FORCED_SHUTDOWN(ip->i_mount))
                return -EIO;
 
-       xfs_ilock(ip, XFS_IOLOCK_EXCL);
-
        trace_xfs_file_splice_write(ip, count, *ppos, ioflags);
 
-       ret = generic_file_splice_write(pipe, outfilp, ppos, count, flags);
+       ret = splice_write_to_file(pipe, outfilp, ppos, count, flags,
+                                       xfs_file_splice_write_actor);
        if (ret > 0)
                XFS_STATS_ADD(xs_write_bytes, ret);
-
-       xfs_iunlock(ip, XFS_IOLOCK_EXCL);
        return ret;
 }
 
-- 
1.7.10

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