Message-ID: <20230804182952.477247-1-bfoster@redhat.com>
Date:   Fri,  4 Aug 2023 14:29:52 -0400
From:   Brian Foster <bfoster@...hat.com>
To:     linux-ext4@...r.kernel.org
Cc:     tytso@....edu, Ritesh Harjani <ritesh.list@...il.com>,
        Jan Kara <jack@...e.cz>
Subject: [PATCH] ext4: drop dio overwrite only flag and associated warning

The commit referenced below opened up concurrent unaligned dio under
shared locking for pure overwrites. In doing so, it enabled use of
the IOMAP_DIO_OVERWRITE_ONLY flag and added a warning on unexpected
-EAGAIN returns as an extra precaution, since ext4 does not retry
writes in such cases. The flag itself is advisory in this case since
ext4 checks for unaligned I/Os and uses appropriate locking up
front, rather than on a retry in response to -EAGAIN.

As it turns out, the warning check is susceptible to false positives
because there are scenarios where -EAGAIN is expected from the
storage layer without necessarily having IOCB_NOWAIT set on the
iocb. For example, io_uring can set IOCB_HIPRI, which the iomap/dio
layer turns into REQ_POLLED|REQ_NOWAIT on the bio, which then can
result in an -EAGAIN result if the block layer is unable to allocate
a request, etc. syzbot has also reported an instance of this warning
and while the source of the -EAGAIN in that case is not currently
known, it is confirmed that the iomap dio overwrite flag is also not
set.

Since this flag is precautionary, avoid the false positive warning
and future whack-a-mole games with -EAGAIN returns by removing it
and the associated warning. Update the comments to document when
concurrent unaligned dio writes are allowed and why the associated
flag is not used.

Reported-by: syzbot+5050ad0fb47527b1808a@...kaller.appspotmail.com
Fixes: 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")
Signed-off-by: Brian Foster <bfoster@...hat.com>
---

Hi all,

This addresses some false positives associated with the warning for the
recently merged patch. I considered leaving the flag and more tightly
associating the warning to it (instead of IOCB_NOWAIT), but ISTM that is
still flakey and I'd rather not play whack-a-mole when the assumption is
shown to be wrong.

I'm still waiting on a syzbot test of this patch, but local tests look
Ok and I'm away for a few days after today so wanted to get this on the
list. Thoughts, reviews, flames appreciated.

Brian

 fs/ext4/file.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index c457c8517f0f..73a4b711be02 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -476,6 +476,11 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 	 * required to change security info in file_modified(), for extending
 	 * I/O, any form of non-overwrite I/O, and unaligned I/O to unwritten
 	 * extents (as partial block zeroing may be required).
+	 *
+	 * Note that unaligned writes are allowed under shared lock so long as
+	 * they are pure overwrites. Otherwise, concurrent unaligned writes risk
+	 * data corruption due to partial block zeroing in the dio layer, and so
+	 * the I/O must occur exclusively.
 	 */
 	if (*ilock_shared &&
 	    ((!IS_NOSEC(inode) || *extend || !overwrite ||
@@ -492,21 +497,12 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 
 	/*
 	 * Now that locking is settled, determine dio flags and exclusivity
-	 * requirements. Unaligned writes are allowed under shared lock so long
-	 * as they are pure overwrites. Set the iomap overwrite only flag as an
-	 * added precaution in this case. Even though this is unnecessary, we
-	 * can detect and warn on unexpected -EAGAIN if an unsafe unaligned
-	 * write is ever submitted.
-	 *
-	 * Otherwise, concurrent unaligned writes risk data corruption due to
-	 * partial block zeroing in the dio layer, and so the I/O must occur
-	 * exclusively. The inode lock is already held exclusive if the write is
-	 * non-overwrite or extending, so drain all outstanding dio and set the
-	 * force wait dio flag.
+	 * requirements. We don't use DIO_OVERWRITE_ONLY because we enforce
+	 * behavior already. The inode lock is already held exclusive if the
+	 * write is non-overwrite or extending, so drain all outstanding dio and
+	 * set the force wait dio flag.
 	 */
-	if (*ilock_shared && unaligned_io) {
-		*dio_flags = IOMAP_DIO_OVERWRITE_ONLY;
-	} else if (!*ilock_shared && (unaligned_io || *extend)) {
+	if (!*ilock_shared && (unaligned_io || *extend)) {
 		if (iocb->ki_flags & IOCB_NOWAIT) {
 			ret = -EAGAIN;
 			goto out;
@@ -608,7 +604,6 @@ 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,
 			   dio_flags, NULL, 0);
-	WARN_ON_ONCE(ret == -EAGAIN && !(iocb->ki_flags & IOCB_NOWAIT));
 	if (ret == -ENOTBLK)
 		ret = 0;
 
-- 
2.41.0

Powered by blists - more mailing lists