Optimize ext4 DAX overwrites

PATCH Superseded Ritesh Harjani 3 participants 6 messages Aug 22, 2020 – Oct 3, 2020

Subsystems (2)

VFS ext4 file system

lore

Share & tools

lore

Ritesh Harjani

author

2020-08-22 11:35 UTC #0 lore raw

v2 cover Optimize ext4 DAX overwrites

Hello,

RFC -> v2
1. Addressed comments from Jan.
2. Added xfstests results in cover letter.

In case of dax writes, currently we start a journal txn irrespective of whether it's an overwrite or not. In case of an overwrite we don't need to start a jbd2 txn since the blocks are already allocated.
So this patch optimizes away the txn start in case of DAX overwrites. This could significantly boost performance for multi-threaded writes specially random writes (overwrite).
Fio script used to collect perf numbers is mentioned below.

Below numbers were calculated on a QEMU setup on ppc64 box with simulated pmem device.

Didn't observe any new failures with this patch in xfstests "-g quick,dax"

Performance numbers with different threads - (~10x improvement)
==========================================

vanilla_kernel(kIOPS) (randomwrite)
60 +-+------+-------+--------+--------+--------+-------+------+-+

     |        +       +        +        +**      +       +        |   
  55 +-+                                 **                     +-+   
     |                          **       **                       |   
     |                          **       **                       |   
  50 +-+                        **       **                     +-+   
     |                          **       **                       |   
  45 +-+                        **       **                     +-+   
     |                          **       **                       |   
     |                          **       **                       |   
  40 +-+                        **       **                     +-+   
     |                          **       **                       |   
  35 +-+               **       **       **                     +-+   
     |                 **       **       **               **      |   
     |                 **       **       **      **       **      |   
  30 +-+      **       **       **       **      **       **    +-+   
     |        **      +**      +**      +**      **      +**      |   
  25 +-+------**------+**------+**------+**------**------+**----+-+   
              1       2        4        8       12      16            
                                     Threads                                   
patched_kernel(kIOPS) (randomwrite)
  600 +-+-----+--------+--------+-------+--------+-------+------+-+   
      |       +        +        +       +        +       +**      |   
      |                                                   **      |   
  500 +-+                                                 **    +-+   
      |                                                   **      |   
      |                                           **      **      |   
  400 +-+                                         **      **    +-+   
      |                                           **      **      |   
  300 +-+                                **       **      **    +-+   
      |                                  **       **      **      |   
      |                                  **       **      **      |   
  200 +-+                                **       **      **    +-+   
      |                         **       **       **      **      |   
      |                         **       **       **      **      |   
  100 +-+               **      **       **       **      **    +-+   
      |                 **      **       **       **      **      |   
      |       +**      +**      **      +**      +**     +**      |   
    0 +-+-----+**------+**------**------+**------+**-----+**----+-+   
              1        2        4       8       12      16            
                                    Threads

fio script
==========
[global]
rw=randwrite
norandommap=1
invalidate=0
bs=4k
numjobs=16 --> changed this for different thread options
time_based=1
ramp_time=30
runtime=60
group_reporting=1
ioengine=psync
direct=1
size=16G filename=file1.0.0:file1.0.1:file1.0.2:file1.0.3:file1.0.4:file1.0.5:file1.0.6:file1.0.7:file1.0.8:file1.0.9:file1.0.10:file1.0.11:file1.0.12:file1.0.13:file1.0.14:file1.0.15:file1.0.16:file1.0.17:file1.0.18:file1.0.19:file1.0.20:file1.0.21:file1.0.22:file1.0.23:file1.0.24:file1.0.25:file1.0.26:file1.0.27:file1.0.28:file1.0.29:file1.0.30:file1.0.31 file_service_type=random
nrfiles=32
directory=/mnt/

[name]
directory=/mnt/
direct=1

NOTE:
======
1. Looking at ~10x perf delta, I probed a bit deeper to understand what's causing this scalability problem. It seems when we are starting a jbd2 txn then slab alloc code is observing some serious contention around spinlock.

Even though the spinlock contention could be related to some other issue (looking into it internally). But I could still see the perf improvement of close to ~2x on QEMU setup on x86 with simulated pmem device with the patched_kernel v/s vanilla_kernel with same fio workload.

perf report from vanilla_kernel (this is not seen with patched kernel) (ppc64)
=======================================================================

47.86% fio [kernel.vmlinux] [k] do_raw_spin_lock

             |
             ---do_raw_spin_lock
                |
                |--19.43%--_raw_spin_lock
                |          |
                |           --19.31%--0
                |                     |
                |                     |--9.77%--deactivate_slab.isra.61
                |                     |          ___slab_alloc
                |                     |          __slab_alloc
                |                     |          kmem_cache_alloc
                |                     |          jbd2__journal_start
                |                     |          __ext4_journal_start_sb

<...>

2. This problem was reported by Dan Williams at [1]

Links
======
[1]: https://lore.kernel.org/linux-ext4/[email protected]/T/

Ritesh Harjani (3):
ext4: Refactor ext4_overwrite_io() to take ext4_map_blocks as argument
ext4: Extend ext4_overwrite_io() for dax path
ext4: Optimize ext4 DAX overwrites

 fs/ext4/ext4.h  |  2 ++
 fs/ext4/file.c  | 28 ++++++++++++++++++----------
 fs/ext4/inode.c | 11 +++++++++--
 3 files changed, 29 insertions(+), 12 deletions(-)

Signature · 1 line

2.25.4

Ritesh Harjani

author

2020-08-22 11:35 UTC #1 lore raw

PATCH v2 1/3 ext4: Refactor ext4_overwrite_io() to take ext4_map_blocks as argument

Refactor ext4_overwrite_io() to take struct ext4_map_blocks as it's function argument with m_lblk and m_len filled from caller

There should be no functionality change in this patch.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/file.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

fs/ext4/file.c +11 ~~-11~~

index 2a01e31a032c..84f73ed91af2 100644
@@ -188,26 +188,22 @@ ext4_extending_io(struct inode *inode, loff_t offset, size_t len)
 }
 
 /* Is IO overwriting allocated and initialized blocks? */
-static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
+static bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map)
 {
-	struct ext4_map_blocks map;
 	unsigned int blkbits = inode->i_blkbits;
-	int err, blklen;
+	loff_t end = (map->m_lblk + map->m_len) << blkbits;
+	int err, blklen = map->m_len;
 
-	if (pos + len > i_size_read(inode))
+	if (end > i_size_read(inode))
 		return false;
 
-	map.m_lblk = pos >> blkbits;
-	map.m_len = EXT4_MAX_BLOCKS(len, pos, blkbits);
-	blklen = map.m_len;
-
-	err = ext4_map_blocks(NULL, inode, &map, 0);
+	err = ext4_map_blocks(NULL, inode, map, 0);
 	/*
 	 * 'err==len' means that all of the blocks have been preallocated,
 	 * regardless of whether they have been initialized or not. To exclude
 	 * unwritten extents, we need to check m_flags.
 	 */
-	return err == blklen && (map.m_flags & EXT4_MAP_MAPPED);
+	return err == blklen && (map->m_flags & EXT4_MAP_MAPPED);
 }
 
 static ssize_t ext4_generic_write_checks(struct kiocb *iocb,

@@ -407,6 +403,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(file);
+	struct ext4_map_blocks map;
 	loff_t offset;
 	size_t count;
 	ssize_t ret;

@@ -420,6 +417,9 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 	count = ret;
 	if (ext4_extending_io(inode, offset, count))
 		*extend = true;
+
+	map.m_lblk = offset >> inode->i_blkbits;
+	map.m_len = EXT4_MAX_BLOCKS(count, offset, inode->i_blkbits);
 	/*
 	 * Determine whether the IO operation will overwrite allocated
 	 * and initialized blocks.

@@ -427,7 +427,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 	 * in file_modified().
 	 */
 	if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
-	     !ext4_overwrite_io(inode, offset, count))) {
+	     !ext4_overwrite_io(inode, &map))) {
 		inode_unlock_shared(inode);
 		*ilock_shared = false;
 		inode_lock(inode);

Signature · 1 line

2.25.4

Ritesh Harjani

author

2020-08-22 11:35 UTC #4 lore raw

PATCH v2 2/3 ext4: Extend ext4_overwrite_io() for dax path

In reply to Ritesh Harjani

DAX uses ->iomap_begin path which gets called to map/allocate extents. In order to avoid starting journal txn where extent allocation is not required, we need to check if ext4_map_blocks() has returned any mapped extent entry.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/ext4.h |  2 ++
 fs/ext4/file.c | 14 +++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

fs/ext4/ext4.h +2 -0

index 42f5060f3cdf..8a1b468bfb49 100644
@@ -3232,6 +3232,8 @@ extern const struct dentry_operations ext4_dentry_ops;
 extern const struct inode_operations ext4_file_inode_operations;
 extern const struct file_operations ext4_file_operations;
 extern loff_t ext4_llseek(struct file *file, loff_t offset, int origin);
+extern bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map,
+			      bool is_dax);
 
 /* inline.c */
 extern int ext4_get_max_inline_size(struct inode *inode);

fs/ext4/file.c +11 -3

index 84f73ed91af2..6c252498334b 100644
@@ -188,7 +188,8 @@ ext4_extending_io(struct inode *inode, loff_t offset, size_t len)
 }
 
 /* Is IO overwriting allocated and initialized blocks? */
-static bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map)
+bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map,
+		       bool is_dax)
 {
 	unsigned int blkbits = inode->i_blkbits;
 	loff_t end = (map->m_lblk + map->m_len) << blkbits;

@@ -198,12 +199,19 @@ static bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map)
 		return false;
 
 	err = ext4_map_blocks(NULL, inode, map, 0);
+
 	/*
+	 * In case of dax to avoid starting a transaction in ext4_iomap_begin()
+	 * we check if ext4_map_blocks() can return any mapped extent.
+	 *
 	 * 'err==len' means that all of the blocks have been preallocated,
 	 * regardless of whether they have been initialized or not. To exclude
 	 * unwritten extents, we need to check m_flags.
 	 */
-	return err == blklen && (map->m_flags & EXT4_MAP_MAPPED);
+	if (is_dax)
+		return err > 0 && (map->m_flags & EXT4_MAP_MAPPED);
+	else
+		return err == blklen && (map->m_flags & EXT4_MAP_MAPPED);
 }
 
 static ssize_t ext4_generic_write_checks(struct kiocb *iocb,

@@ -427,7 +435,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
 	 * in file_modified().
 	 */
 	if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
-	     !ext4_overwrite_io(inode, &map))) {
+	     !ext4_overwrite_io(inode, &map, false))) {
 		inode_unlock_shared(inode);
 		*ilock_shared = false;
 		inode_lock(inode);

Signature · 1 line

2.25.4

Ritesh Harjani

author

2020-08-22 11:35 UTC #5 lore raw

PATCH v2 3/3 ext4: Optimize ext4 DAX overwrites

In reply to Ritesh Harjani

Currently in case of DAX, we are starting a journal txn everytime for IOMAP_WRITE case. This can be optimized away in case of an overwrite (where the blocks were already allocated).

This could give a significant performance boost for multi-threaded writes specially random writes.
On PPC64 VM with simulated pmem device, ~10x perf improvement could be seen in random writes (overwrite). Also bcoz this optimizes away the spinlock contention during jbd2 slab cache allocation (jbd2_journal_handle) On x86 VM, ~2x perf improvement was observed.

Reported-by Dan Williams <[email protected]>

Suggested-by Jan Kara <[email protected]>

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/inode.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

fs/ext4/inode.c +9 -2

index 10dd470876b3..c18009c91e68 100644
@@ -3437,6 +3437,14 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
 			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
 
+	/*
+	 * In case of DAX write, we check if this is overwrite request, to avoid
+	 * starting a journal txn in ext4_iomap_alloc()
+	 */
+	if ((flags & IOMAP_WRITE) && IS_DAX(inode) &&
+	    ext4_overwrite_io(inode, &map, true))
+		goto out_set;
+
 	if (flags & IOMAP_WRITE)
 		ret = ext4_iomap_alloc(inode, &map, flags);
 	else

@@ -3444,9 +3452,8 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
 
 	if (ret < 0)
 		return ret;
-
+out_set:
 	ext4_set_iomap(inode, iomap, &map, offset, length);
-
 	return 0;
 }

Signature · 1 line

2.25.4

Dan Carpenter

2020-08-24 12:15 UTC #2 lore raw

Re: [PATCHv2 1/3] ext4: Refactor ext4_overwrite_io() to take ext4_map_blocks as argument

In reply to Ritesh Harjani

Hi Ritesh,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.9-rc1]
[cannot apply to ext4/dev next-20200821]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ritesh-Harjani/Optimize-ext4-DAX-overwrites/20200822-193615
base:    9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
config: i386-randconfig-m021-20200822 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

Reported-by Dan Carpenter <[email protected]>

New smatch warnings:
fs/ext4/file.c:194 ext4_overwrite_io() warn: should '(map->m_lblk + map->m_len) << blkbits' be a 64 bit type?

Old smatch warnings:
include/linux/fs.h:867 i_size_write() warn: statement has no effect 31 fs/ext4/file.c:585 ext4_dio_write_iter() warn: inconsistent returns 'inode->i_rwsem'.

# https://github.com/0day-ci/linux/commit/5d171d1d87ee0aca0a992b6843d154b41466e5e5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ritesh-Harjani/Optimize-ext4-DAX-overwrites/20200822-193615 git checkout 5d171d1d87ee0aca0a992b6843d154b41466e5e5
vim +194 fs/ext4/file.c

e9e3bcecf44c04 Eric Sandeen   2011-02-12  189  
213bcd9ccbf04b Jan Kara       2016-11-20  190  /* Is IO overwriting allocated and initialized blocks? */
5d171d1d87ee0a Ritesh Harjani 2020-08-22  191  static bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map)
213bcd9ccbf04b Jan Kara       2016-11-20  192  {
213bcd9ccbf04b Jan Kara       2016-11-20  193  	unsigned int blkbits = inode->i_blkbits;
5d171d1d87ee0a Ritesh Harjani 2020-08-22 @194  	loff_t end = (map->m_lblk + map->m_len) << blkbits;
                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

potential shift wrap?

5d171d1d87ee0a Ritesh Harjani 2020-08-22  195  	int err, blklen = map->m_len;
213bcd9ccbf04b Jan Kara       2016-11-20  196  
5d171d1d87ee0a Ritesh Harjani 2020-08-22  197  	if (end > i_size_read(inode))
213bcd9ccbf04b Jan Kara       2016-11-20  198  		return false;
213bcd9ccbf04b Jan Kara       2016-11-20  199  
5d171d1d87ee0a Ritesh Harjani 2020-08-22  200  	err = ext4_map_blocks(NULL, inode, map, 0);
213bcd9ccbf04b Jan Kara       2016-11-20  201  	/*
213bcd9ccbf04b Jan Kara       2016-11-20  202  	 * 'err==len' means that all of the blocks have been preallocated,
213bcd9ccbf04b Jan Kara       2016-11-20  203  	 * regardless of whether they have been initialized or not. To exclude
213bcd9ccbf04b Jan Kara       2016-11-20  204  	 * unwritten extents, we need to check m_flags.
213bcd9ccbf04b Jan Kara       2016-11-20  205  	 */
5d171d1d87ee0a Ritesh Harjani 2020-08-22  206  	return err == blklen && (map->m_flags & EXT4_MAP_MAPPED);
213bcd9ccbf04b Jan Kara       2016-11-20  207  }
213bcd9ccbf04b Jan Kara       2016-11-20  208

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Theodore Y. Ts'o

maintainer

2020-10-03 03:59 UTC #3 lore raw

Re: [PATCHv2 1/3] ext4: Refactor ext4_overwrite_io() to take ext4_map_blocks as argument

In reply to Ritesh Harjani

Quoted 16 lines · Ritesh Harjani

Refactor ext4_overwrite_io() to take struct ext4_map_blocks as it's function argument with m_lblk and m_len filled from caller
There should be no functionality change in this patch.
Signed-off-by Ritesh Harjani <[email protected]>
---
 fs/ext4/file.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
fs/ext4/file.c +0 -0
index 2a01e31a032c..84f73ed91af2 100644

fs/ext4/file.c ext4_extending_io(struct inode *inode, loff_t offset, size_t len) · line 194

Quoted 10 lines

fs/ext4/file.c +2 -3

@@ -188,26 +188,22 @@ ext4_extending_io(struct inode *inode, loff_t offset, size_t len)
 }
 
 /* Is IO overwriting allocated and initialized blocks? */
-static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
+static bool ext4_overwrite_io(struct inode *inode, struct ext4_map_blocks *map)
 {
-	struct ext4_map_blocks map;
 	unsigned int blkbits = inode->i_blkbits;
-	int err, blklen;ts
+	loff_t end = (map->m_lblk + map->m_len) << blkbits;

As Dan Carpenter has pointed out, we need to cast map->m_lblk to loff_t, since m_lblk is 32 bits, and when this get shifted left by blkbits, we could end up losing bits.

fs/ext4/file.c ext4_extending_io(struct inode *inode, loff_t offset, size_t len) · line 198

Quoted 3 lines

fs/ext4/file.c +1 -1

-	if (pos + len > i_size_read(inode))
+	if (end > i_size_read(inode))
 		return false;

This transformation is not functionally identical.

The problem is that pos is not necessarily a multiple of the file
system blocksize. From below,

fs/ext4/file.c static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from, · line 422

Quoted 2 lines

fs/ext4/file.c +2 -0

+	map.m_lblk = offset >> inode->i_blkbits;
+	map.m_len = EXT4_MAX_BLOCKS(count, offset, inode->i_blkbits);

So what previously was the starting offset of the overwrite, is now offset shifted right by blkbits, and then shifted left back by blkbits.

So unless I'm missing something, this looks not quite right?

- Ted