Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Ritesh Harjani <riteshh@linux.ibm.com>
To:     jack@suse.cz, tytso@mit.edu, linux-ext4@vger.kernel.org
Cc:     linux-fsdevel@vger.kernel.org, mbobrowski@mbobrowski.org,
        riteshh@linux.ibm.com
Subject: [RFCv3 4/4] ext4: Move to shared iolock even without dioread_nolock mount opt
Date:   Wed, 20 Nov 2019 10:30:24 +0530
In-Reply-To: <20191120050024.11161-1-riteshh@linux.ibm.com>
References: <20191120050024.11161-1-riteshh@linux.ibm.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Message-Id: <20191120050024.11161-5-riteshh@linux.ibm.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

We were using shared locking only in case of dioread_nolock
mount option in case of DIO overwrites. This mount condition
is not needed anymore with current code, since:-

1. No race between buffered writes & DIO overwrites.
Since buffIO writes takes exclusive locks & DIO overwrites
will take share locking. Also DIO path will make sure
to flush and wait for any dirty page cache data.

2. No race between buffered reads & DIO overwrites, since there
is no block allocation that is possible with DIO overwrites.
So no stale data exposure should happen. Same is the case
between DIO reads & DIO overwrites.

3. Also other paths like truncate is protected,
since we wait there for any DIO in flight to be over.

4. In case of buffIO writes followed by DIO reads:
Since here also we take exclusive locks in ext4_write_begin/end().
There is no risk of exposing any stale data in this case.
Since after ext4_write_end, iomap_dio_rw() will wait to flush &
wait for any dirty page cache data.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/ext4/file.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 18cbf9fa52c6..b97efc89cd63 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -383,6 +383,17 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
 	.end_io = ext4_dio_write_end_io,
 };
 
+static bool ext4_dio_should_shared_lock(struct inode *inode)
+{
+	if (!S_ISREG(inode->i_mode))
+		return false;
+	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
+		return false;
+	if (ext4_should_journal_data(inode))
+		return false;
+	return true;
+}
+
 /*
  * The intention here is to start with shared lock acquired then see if any
  * condition requires an exclusive inode lock. If yes, then we restart the
@@ -394,8 +405,8 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
  * - For extending writes case we don't take the shared lock, since it requires
  *   updating inode i_disksize and/or orphan handling with exclusive lock.
  *
- * - shared locking will only be true mostly in case of overwrites with
- *   dioread_nolock mode. Otherwise we will switch to excl. iolock mode.
+ * - shared locking will only be true mostly in case of overwrites.
+ *   Otherwise we will switch to excl. iolock mode.
  */
 static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 				 unsigned int *iolock, bool *unaligned_io,
@@ -433,15 +444,14 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 		*extend = true;
 	/*
 	 * Determine whether the IO operation will overwrite allocated
-	 * and initialized blocks. If so, check to see whether it is
-	 * possible to take the dioread_nolock path.
+	 * and initialized blocks.
 	 *
 	 * We need exclusive i_rwsem for changing security info
 	 * in file_modified().
 	 */
 	if (*iolock == EXT4_IOLOCK_SHARED &&
 	    (!IS_NOSEC(inode) || *unaligned_io || *extend ||
-	     !ext4_should_dioread_nolock(inode) ||
+	     !ext4_dio_should_shared_lock(inode) ||
 	     !ext4_overwrite_io(inode, offset, count))) {
 		ext4_iunlock(inode, *iolock);
 		*iolock = EXT4_IOLOCK_EXCL;
@@ -485,7 +495,10 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		iolock = EXT4_IOLOCK_EXCL;
 	}
 
-	if (iolock == EXT4_IOLOCK_SHARED && !ext4_should_dioread_nolock(inode))
+	/*
+	 * Check if we should continue with shared iolock
+	 */
+	if (iolock == EXT4_IOLOCK_SHARED && !ext4_dio_should_shared_lock(inode))
 		iolock = EXT4_IOLOCK_EXCL;
 
 	if (iocb->ki_flags & IOCB_NOWAIT) {
-- 
2.21.0