From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: [PATCH 2/2] xfs: fix splice/direct-IO deadlock Date: Wed, 28 Nov 2012 13:12:48 +1100 Message-ID: <1354068768-4316-3-git-send-email-david@fromorbit.com> References: <1354068768-4316-1-git-send-email-david@fromorbit.com> Cc: xfs@oss.sgi.com To: linux-fsdevel@vger.kernel.org Return-path: Received: from ipmail04.adl6.internode.on.net ([150.101.137.141]:31029 "EHLO ipmail04.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750936Ab2K1CNX (ORCPT ); Tue, 27 Nov 2012 21:13:23 -0500 In-Reply-To: <1354068768-4316-1-git-send-email-david@fromorbit.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: From: Dave Chinner 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 --- 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 #include #include +#include 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