Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Zhongwei Cai <sunrise_l@sjtu.edu.cn>
To:     tytso@mit.edu, adilger.kernel@dilger.ca
Cc:     linux-ext4@vger.kernel.org, mingkaidong@gmail.com,
        Zhongwei Cai <sunrise_l@sjtu.edu.cn>
Subject: [PATCH] ext4: remove unnecessary ext4_inode_datasync_dirty in read path
Date:   Tue,  2 Nov 2021 10:42:58 +0800
Message-Id: <20211102024258.210439-1-sunrise_l@sjtu.edu.cn>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

ext4_inode_datasync_dirty will call read_lock(&journal->j_state_lock) in
journal mode, which is unnecessary in read path (As far as I know, the
IOMAP_F_DIRTY flag set in the if branch is only used in write path,
making it unnecessary in read path. Please correct me if I'm wrong).
and will cause cache contention overhead under high concurrency
especially in DAX mode. The unnecessary ext4_inode_datasync_dirty can be
eliminated by passing flags into ext4_set_iomap and checking it.

Performance tests are shown below. Workloads include simply reading files,
fileserver in filebench and readrandom/readsequence in RocksDB under
nosync mode.

Sixteen thread performance under ext4-DAX:
 Throughput (Kop/s) | original |  +patch  | improvement
 -------------------+----------+----------+--------------
 Read 4KB block     |   11456  |   27651  |  +141.37%
 fileserver         |     339  |     343  |  +1.18%
 readrandom         |    1807  |    1837  |  +1.66%
 readsequence       |   29724  |   30102  |  +1.27%

Signed-off-by: Zhongwei Cai <sunrise_l@sjtu.edu.cn>
---
 fs/ext4/inode.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 0f06305167d5..72ec2074ef54 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3274,7 +3274,7 @@ static bool ext4_inode_datasync_dirty(struct inode *inode)
 
 static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
 			   struct ext4_map_blocks *map, loff_t offset,
-			   loff_t length)
+			   loff_t length, int flags)
 {
 	u8 blkbits = inode->i_blkbits;
 
@@ -3284,8 +3284,8 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
 	 * there is no other metadata changes being made or are pending.
 	 */
 	iomap->flags = 0;
-	if (ext4_inode_datasync_dirty(inode) ||
-	    offset + length > i_size_read(inode))
+	if ((flags & IOMAP_WRITE) && (ext4_inode_datasync_dirty(inode) ||
+	    offset + length > i_size_read(inode)))
 		iomap->flags |= IOMAP_F_DIRTY;
 
 	if (map->m_flags & EXT4_MAP_NEW)
@@ -3423,7 +3423,7 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	if (ret < 0)
 		return ret;
 out:
-	ext4_set_iomap(inode, iomap, &map, offset, length);
+	ext4_set_iomap(inode, iomap, &map, offset, length, flags);
 
 	return 0;
 }
@@ -3543,7 +3543,7 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
 		delalloc = ext4_iomap_is_delalloc(inode, &map);
 
 set_iomap:
-	ext4_set_iomap(inode, iomap, &map, offset, length);
+	ext4_set_iomap(inode, iomap, &map, offset, length, flags);
 	if (delalloc && iomap->type == IOMAP_HOLE)
 		iomap->type = IOMAP_DELALLOC;
 
-- 
2.26.0