From patchwork Mon Jul 13 07:46:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoph Hellwig X-Patchwork-Id: 11659021 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 2AFBF13B4 for ; Mon, 13 Jul 2020 07:51:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 12EC720702 for ; Mon, 13 Jul 2020 07:51:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="A4BsM8yb" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729021AbgGMHvB (ORCPT ); Mon, 13 Jul 2020 03:51:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51040 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725818AbgGMHvB (ORCPT ); Mon, 13 Jul 2020 03:51:01 -0400 Received: from casper.infradead.org (casper.infradead.org [IPv6:2001:8b0:10b:1236::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C6486C061755; Mon, 13 Jul 2020 00:51:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=Content-Transfer-Encoding:MIME-Version: References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To: Content-Type:Content-ID:Content-Description; bh=hykAUMgpdata3SiriROHkxMEOf/Azb0hlzJQjPO3wYg=; b=A4BsM8ybPqUfXVx62Drwe/Jujf wqxr26Ne2OZ9n1+Y29edT9Ihi9/3yjAa3O8lsdObJCdoLIqlvoicozKagpEW+km6W1Kd5L5j4WXgb 6JdA63PFf/vJaWt9Rzi9eMUEC7ydvOawKy3gXCZktTPjGUNVmKH6CWRMPGrL3BldlSj2hDoODaan3 EjGsj1FftqH+P94yameCoeY9/KDKCoRQhz8r/DOFbOmM2dXEiDt+hyjOr0I+4bEm9px1etzUg8XYv zm/dDFoAnJcZhSHXRN7su3d2AZ/fyR0vkuheMnh1w1gDjB3+5D3KGYeuemqBz9HHAqdgHsKXg2EKj 7XVMQB4Q==; Received: from 089144201169.atnat0010.highway.a1.net ([89.144.201.169] helo=localhost) by casper.infradead.org with esmtpsa (Exim 4.92.3 #3 (Red Hat Linux)) id 1jutEl-0001HX-SP; Mon, 13 Jul 2020 07:50:56 +0000 From: Christoph Hellwig To: Dave Chinner , Goldwyn Rodrigues Cc: Damien Le Moal , Naohiro Aota , Johannes Thumshirn , Matthew Wilcox , linux-btrfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, cluster-devel@redhat.com, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org, Dave Chinner , "Darrick J . Wong" Subject: [PATCH 1/2] iomap: Only invalidate page cache pages on direct IO writes Date: Mon, 13 Jul 2020 09:46:32 +0200 Message-Id: <20200713074633.875946-2-hch@lst.de> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200713074633.875946-1-hch@lst.de> References: <20200713074633.875946-1-hch@lst.de> MIME-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by casper.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org From: Dave Chinner The historic requirement for XFS to invalidate cached pages on direct IO reads has been lost in the twisty pages of history - it was inherited from Irix, which implemented page cache invalidation on read as a method of working around problems synchronising page cache state with uncached IO. XFS has carried this ever since. In the initial linux ports it was necessary to get mmap and DIO to play "ok" together and not immediately corrupt data. This was the state of play until the linux kernel had infrastructure to track unwritten extents and synchronise page faults with allocations and unwritten extent conversions (->page_mkwrite infrastructure). IOws, the page cache invalidation on DIO read was necessary to prevent trivial data corruptions. This didn't solve all the problems, though. There were peformance problems if we didn't invalidate the entire page cache over the file on read - we couldn't easily determine if the cached pages were over the range of the IO, and invalidation required taking a serialising lock (i_mutex) on the inode. This serialising lock was an issue for XFS, as it was the only exclusive lock in the direct Io read path. Hence if there were any cached pages, we'd just invalidate the entire file in one go so that subsequent IOs didn't need to take the serialising lock. This was a problem that prevented ranged invalidation from being particularly useful for avoiding the remaining coherency issues. This was solved with the conversion of i_mutex to i_rwsem and the conversion of the XFS inode IO lock to use i_rwsem. Hence we could now just do ranged invalidation and the performance problem went away. However, page cache invalidation was still needed to serialise sub-page/sub-block zeroing via direct IO against buffered IO because bufferhead state attached to the cached page could get out of whack when direct IOs were issued. We've removed bufferheads from the XFS code, and we don't carry any extent state on the cached pages anymore, and so this problem has gone away, too. IOWs, it would appear that we don't have any good reason to be invalidating the page cache on DIO reads anymore. Hence remove the invalidation on read because it is unnecessary overhead, not needed to maintain coherency between mmap/buffered access and direct IO anymore, and prevents anyone from using direct IO reads from intentionally invalidating the page cache of a file. Signed-off-by: Dave Chinner Reviewed-by: Darrick J. Wong Reviewed-by: Matthew Wilcox (Oracle) Signed-off-by: Christoph Hellwig --- fs/iomap/direct-io.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index ec7b78e6fecaf9..190967e87b69e4 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -475,23 +475,22 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, if (ret) goto out_free_dio; - /* - * Try to invalidate cache pages for the range we're direct - * writing. If this invalidation fails, tough, the write will - * still work, but racing two incompatible write paths is a - * pretty crazy thing to do, so we don't support it 100%. - */ - ret = invalidate_inode_pages2_range(mapping, - pos >> PAGE_SHIFT, end >> PAGE_SHIFT); - if (ret) - dio_warn_stale_pagecache(iocb->ki_filp); - ret = 0; + if (iov_iter_rw(iter) == WRITE) { + /* + * Try to invalidate cache pages for the range we are writing. + * If this invalidation fails, tough, the write will still work, + * but racing two incompatible write paths is a pretty crazy + * thing to do, so we don't support it 100%. + */ + if (invalidate_inode_pages2_range(mapping, pos >> PAGE_SHIFT, + end >> PAGE_SHIFT)) + dio_warn_stale_pagecache(iocb->ki_filp); - if (iov_iter_rw(iter) == WRITE && !wait_for_completion && - !inode->i_sb->s_dio_done_wq) { - ret = sb_init_dio_done_wq(inode->i_sb); - if (ret < 0) - goto out_free_dio; + if (!wait_for_completion && !inode->i_sb->s_dio_done_wq) { + ret = sb_init_dio_done_wq(inode->i_sb); + if (ret < 0) + goto out_free_dio; + } } inode_dio_begin(inode); From patchwork Mon Jul 13 07:46:33 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoph Hellwig X-Patchwork-Id: 11659025 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 2CF45618 for ; Mon, 13 Jul 2020 07:51:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 149662072D for ; Mon, 13 Jul 2020 07:51:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="uwZGM77V" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729119AbgGMHvF (ORCPT ); Mon, 13 Jul 2020 03:51:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51052 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725818AbgGMHvE (ORCPT ); Mon, 13 Jul 2020 03:51:04 -0400 Received: from casper.infradead.org (casper.infradead.org [IPv6:2001:8b0:10b:1236::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6FFBBC061755; Mon, 13 Jul 2020 00:51:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=Content-Transfer-Encoding:MIME-Version: References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To: Content-Type:Content-ID:Content-Description; bh=fEeOSsxpAAIZfme2loA+JMZ4rfGCvVv1qQyMxpOsEW0=; b=uwZGM77VT1fzGFhoU7JqwR+ANN L2ujdv0H6O8lP3bQeMzOJXwAb2TaYZRScGtRYxmHK/54C19yORWToz0HJiMPK4birv06qpE9H6Yd5 4mIXkLsp8ULze8EBCai2Iusbhx6en/4O+FmmGR1xkydGIJMyhr6whvxCLQwrIGAQzsGvwTx+nRafM 8sbPWiXj/lKz0Qod5Cm5ftnMIqKqeJDCjNa1gmV2/a4wx4PE0m3PLoDHHf/8F0Q/UrSpz5w6Rq27G 4h/tswzc0kzFtVGzwHrMM/ez8s3lRoe7cerXx5olFcXusg2UFCvop98KgxUcgBeNzr1caE0cDPsuS hJmakUkQ==; Received: from [2001:4bb8:188:5f50:c70:4a89:bc61:2] (helo=localhost) by casper.infradead.org with esmtpsa (Exim 4.92.3 #3 (Red Hat Linux)) id 1jutEo-0001Hf-2U; Mon, 13 Jul 2020 07:50:58 +0000 From: Christoph Hellwig To: Dave Chinner , Goldwyn Rodrigues Cc: Damien Le Moal , Naohiro Aota , Johannes Thumshirn , Matthew Wilcox , linux-btrfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, cluster-devel@redhat.com, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org Subject: [PATCH 2/2] iomap: fall back to buffered writes for invalidation failures Date: Mon, 13 Jul 2020 09:46:33 +0200 Message-Id: <20200713074633.875946-3-hch@lst.de> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200713074633.875946-1-hch@lst.de> References: <20200713074633.875946-1-hch@lst.de> MIME-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by casper.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org Failing to invalid the page cache means data in incoherent, which is a very bad state for the system. Always fall back to buffered I/O through the page cache if we can't invalidate mappings. Signed-off-by: Christoph Hellwig Reviewed-by: Goldwyn Rodrigues --- fs/ext4/file.c | 2 ++ fs/gfs2/file.c | 3 ++- fs/iomap/direct-io.c | 13 ++++++++----- fs/iomap/trace.h | 1 + fs/xfs/xfs_file.c | 4 ++-- fs/zonefs/super.c | 7 +++++-- 6 files changed, 20 insertions(+), 10 deletions(-) diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 2a01e31a032c4c..0da6c2a2c32c1e 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -544,6 +544,8 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from) iomap_ops = &ext4_iomap_overwrite_ops; ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops, is_sync_kiocb(iocb) || unaligned_io || extend); + if (ret == -EREMCHG) + ret = 0; if (extend) ret = ext4_handle_inode_extension(inode, offset, ret, count); diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index fe305e4bfd3734..c7907d40c61d17 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -814,7 +814,8 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from) ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL, is_sync_kiocb(iocb)); - + if (ret == -EREMCHG) + ret = 0; out: gfs2_glock_dq(&gh); out_uninit: diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 190967e87b69e4..62626235cdbe8d 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -10,6 +10,7 @@ #include #include #include +#include "trace.h" #include "../internal.h" @@ -478,13 +479,15 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, if (iov_iter_rw(iter) == WRITE) { /* * Try to invalidate cache pages for the range we are writing. - * If this invalidation fails, tough, the write will still work, - * but racing two incompatible write paths is a pretty crazy - * thing to do, so we don't support it 100%. + * If this invalidation fails, let the caller fall back to + * buffered I/O. */ if (invalidate_inode_pages2_range(mapping, pos >> PAGE_SHIFT, - end >> PAGE_SHIFT)) - dio_warn_stale_pagecache(iocb->ki_filp); + end >> PAGE_SHIFT)) { + trace_iomap_dio_invalidate_fail(inode, pos, count); + ret = -EREMCHG; + goto out_free_dio; + } if (!wait_for_completion && !inode->i_sb->s_dio_done_wq) { ret = sb_init_dio_done_wq(inode->i_sb); diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h index 5693a39d52fb63..fdc7ae388476f5 100644 --- a/fs/iomap/trace.h +++ b/fs/iomap/trace.h @@ -74,6 +74,7 @@ DEFINE_EVENT(iomap_range_class, name, \ DEFINE_RANGE_EVENT(iomap_writepage); DEFINE_RANGE_EVENT(iomap_releasepage); DEFINE_RANGE_EVENT(iomap_invalidatepage); +DEFINE_RANGE_EVENT(iomap_dio_invalidate_fail); #define IOMAP_TYPE_STRINGS \ { IOMAP_HOLE, "HOLE" }, \ diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 00db81eac80d6c..551cca39fa3ba6 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -553,8 +553,8 @@ xfs_file_dio_aio_write( xfs_iunlock(ip, iolock); /* - * No fallback to buffered IO on errors for XFS, direct IO will either - * complete fully or fail. + * No partial fallback to buffered IO on errors for XFS, direct IO will + * either complete fully or fail. */ ASSERT(ret < 0 || ret == count); return ret; diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c index 07bc42d62673ce..793850454b752f 100644 --- a/fs/zonefs/super.c +++ b/fs/zonefs/super.c @@ -786,8 +786,11 @@ static ssize_t zonefs_file_write_iter(struct kiocb *iocb, struct iov_iter *from) if (iocb->ki_pos >= ZONEFS_I(inode)->i_max_size) return -EFBIG; - if (iocb->ki_flags & IOCB_DIRECT) - return zonefs_file_dio_write(iocb, from); + if (iocb->ki_flags & IOCB_DIRECT) { + ret = zonefs_file_dio_write(iocb, from); + if (ret != -EREMCHG) + return ret; + } return zonefs_file_buffered_write(iocb, from); }