ext2: DIO to use iomap

PATCH In review Ritesh Harjani (IBM) 3 participants 18 messages Apr 21, 2023 – Apr 24, 2023

Subsystems (4)

VFS ext4 file system ext2 file system Filesystems iomap

lore

Share & tools

lore

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #0 lore raw

v6 cover ext2: DIO to use iomap

Hello All,

Please find the series which rewrites ext2 direct-io path to use modern iomap interface.

PATCHv5 -> PATCHv6:
===================
1. Patch-2 Added generic_buffers_fsync_noflush() & generic_buffers_fsync() functions.
2. Patch-3 & Patch-4 to use above functions in ext4 & ext2.
3. Added Reviewed-by from Christoph on Patch-9 (iomap: Add DIO tracepoints)

RFCv4 -> PATCHv5:
=================
1. Added trace_iomap_dio_rw_begin tracepoint in __iomap_dio_rw()
2. Added Reviewed-by tags from Christoph

RFCv3 -> RFCV4:
===============
1. Renamed __generic_file_fsync_nolock() from libfs to generic_buffer_fsync() in fs/buffer.c
(Review comment from Christoph)
2. Fixed s/EVENTD/EVENTFD/ in TRACE_IOCB_STRINGS
3. Fixed few data types for parameters in ext2 trace patch (size_t && ssize_t)
4. Killed this patch "Minor refactor of iomap_dio_rw"
5. Changed iomap tracepoint patch and fixed the data types (size_t && ssize_t) (addressed review comments from Christoph)

RFCv2 -> RFCv3:
===============
1. Addressed minor review comments related to extern, parameter naming in function declaration, removing not required braces and shorting overly long lines.
2. Added Reviewed-by from various reviewers.
3. Fixed a warning & couple of compilation errors in Patch-7 (ext2 trace points) related to CFLAGS_trace & second related to unable to find function definition for iov_iter_count(). (requires uio.h file) CFLAGS_trace is required in Makefile so that it can find trace.h file from tracepoint infrastructure.
4. Changed naming of IOCB_STRINGS TO TRACE_IOCB_STRINGS.
5. Shortened naming of tracepoint events for ext2 dio.
6. Added iomap DIO tracepoint events.
7. Disha tested this series internally against Power with "auto" group for 4k and 64k blocksize configuration. Added her "Tested-by" tag in all DIO related patches. No new failures were reported.

Thanks everyone for the review and test. The series is looking good to me now. It has been tested on x86 and Power with different configurations. Please let me know if anything else is required on this.

v2: https://lore.kernel.org/all/[email protected]/

Ritesh Harjani (IBM) (9):
ext2/dax: Fix ext2_setsize when len is page aligned fs/buffer.c: Add generic_buffers_fsync*() implementation
ext4: Use generic_buffers_fsync_noflush() implementation
ext2: Use generic_buffers_fsync() implementation
ext2: Move direct-io to use iomap
fs.h: Add TRACE_IOCB_STRINGS for use in trace points
ext2: Add direct-io trace points
iomap: Remove IOMAP_DIO_NOSYNC unused dio flag
iomap: Add DIO tracepoints

 fs/buffer.c                 |  70 ++++++++++++++++++++
 fs/ext2/Makefile            |   5 +-
 fs/ext2/ext2.h              |   1 +
 fs/ext2/file.c              | 126 +++++++++++++++++++++++++++++++++++-
 fs/ext2/inode.c             |  58 ++++++++++-------
 fs/ext2/trace.c             |   6 ++
 fs/ext2/trace.h             |  94 +++++++++++++++++++++++++++
 fs/ext4/fsync.c             |  33 +++++-----
 fs/iomap/direct-io.c        |   9 ++-
 fs/iomap/trace.c            |   1 +
 fs/iomap/trace.h            |  78 ++++++++++++++++++++++
 include/linux/buffer_head.h |   4 ++
 include/linux/fs.h          |  14 ++++
 include/linux/iomap.h       |   6 --
 14 files changed, 456 insertions(+), 49 deletions(-)
 create mode 100644 fs/ext2/trace.c
 create mode 100644 fs/ext2/trace.h

--
2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #1 lore raw

PATCH v6 1/9 ext2/dax: Fix ext2_setsize when len is page aligned

PAGE_ALIGN(x) macro gives the next highest value which is multiple of pagesize. But if x is already page aligned then it simply returns x. So, if x passed is 0 in dax_zero_range() function, that means the length gets passed as 0 to ->iomap_begin().

In ext2 it then calls ext2_get_blocks -> max_blocks as 0 and hits bug_on here in ext2_get_blocks().

	BUG_ON(maxblocks == 0);

Instead we should be calling dax_truncate_page() here which takes care of it. i.e. it only calls dax_zero_range if the offset is not page/block aligned.

This can be easily triggered with following on fsdax mounted pmem device.

dd if=/dev/zero of=file count=1 bs=512
truncate -s 0 file

[79.525838] EXT2-fs (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
[79.529376] ext2 filesystem being mounted at /mnt1/test supports timestamps until 2038 (0x7fffffff)
[93.793207] ------------[ cut here ]------------
[93.795102] kernel BUG at fs/ext2/inode.c:637!
[93.796904] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[93.798659] CPU: 0 PID: 1192 Comm: truncate Not tainted 6.3.0-rc2-xfstests-00056-g131086faa369 #139
[93.806459] RIP: 0010:ext2_get_blocks.constprop.0+0x524/0x610
<...>
[93.835298] Call Trace:
[93.836253]  <TASK>
[93.837103]  ? lock_acquire+0xf8/0x110
[93.838479]  ? d_lookup+0x69/0xd0
[93.839779]  ext2_iomap_begin+0xa7/0x1c0
[93.841154]  iomap_iter+0xc7/0x150
[93.842425]  dax_zero_range+0x6e/0xa0
[93.843813]  ext2_setsize+0x176/0x1b0
[93.845164]  ext2_setattr+0x151/0x200
[93.846467]  notify_change+0x341/0x4e0
[93.847805]  ? lock_acquire+0xf8/0x110
[93.849143]  ? do_truncate+0x74/0xe0
[93.850452]  ? do_truncate+0x84/0xe0
[93.851739]  do_truncate+0x84/0xe0
[93.852974]  do_sys_ftruncate+0x2b4/0x2f0
[93.854404]  do_syscall_64+0x3f/0x90
[93.855789]  entry_SYSCALL_64_after_hwframe+0x72/0xdc

Reviewed-by Darrick J. Wong <[email protected]>

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

---
 fs/ext2/inode.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

fs/ext2/inode.c +2 -3

index 26f135e7ffce..dc76147e7b07 100644
@@ -1259,9 +1259,8 @@ static int ext2_setsize(struct inode *inode, loff_t newsize)
 	inode_dio_wait(inode);
 
 	if (IS_DAX(inode))
-		error = dax_zero_range(inode, newsize,
-				       PAGE_ALIGN(newsize) - newsize, NULL,
-				       &ext2_iomap_ops);
+		error = dax_truncate_page(inode, newsize, NULL,
+					  &ext2_iomap_ops);
 	else
 		error = block_truncate_page(inode->i_mapping,
 				newsize, ext2_get_block);

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #2 lore raw

PATCH v6 2/9 fs/buffer.c: Add generic_buffers_fsync*() implementation

In reply to Ritesh Harjani (IBM)

Some of the higher layers like iomap takes inode_lock() when calling generic_write_sync().
Also writeback already happens from other paths without inode lock, so it's difficult to say that we really need sync_mapping_buffers() to take any inode locking here. Having said that, let's add generic_buffers_fsync/_noflush() implementation in buffer.c with no inode_lock/unlock() for now so that filesystems like ext2 and ext4's nojournal mode can use it.

Ext4 when got converted to iomap for direct-io already copied it's own variant of __generic_file_fsync() without lock.

This patch adds generic_buffers_fsync()
& generic_buffers_fsync_noflush() implementations for use in filesystems like ext2 & ext4 respectively.

Later we can review other filesystems as well to see if we can make generic_buffers_fsync/_noflush() which does not take any inode_lock() as the default path.

Tested-by Disha Goel <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 fs/buffer.c                 | 70 +++++++++++++++++++++++++++++++++++++
 include/linux/buffer_head.h |  4 +++
 2 files changed, 74 insertions(+)

fs/buffer.c +70 -0

index 9e1e2add541e..05febfbaecdc 100644
@@ -593,6 +593,76 @@ int sync_mapping_buffers(struct address_space *mapping)
 }
 EXPORT_SYMBOL(sync_mapping_buffers);
 
+/**
+ * generic_buffers_fsync_noflush - generic buffer fsync implementation
+ * for simple filesystems with no inode lock
+ *
+ * @file:	file to synchronize
+ * @start:	start offset in bytes
+ * @end:	end offset in bytes (inclusive)
+ * @datasync:	only synchronize essential metadata if true
+ *
+ * This is a generic implementation of the fsync method for simple
+ * filesystems which track all non-inode metadata in the buffers list
+ * hanging off the address_space structure.
+ */
+int generic_buffers_fsync_noflush(struct file *file, loff_t start, loff_t end,
+				  bool datasync)
+{
+	struct inode *inode = file->f_mapping->host;
+	int err;
+	int ret;
+
+	err = file_write_and_wait_range(file, start, end);
+	if (err)
+		return err;
+
+	ret = sync_mapping_buffers(inode->i_mapping);
+	if (!(inode->i_state & I_DIRTY_ALL))
+		goto out;
+	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
+		goto out;
+
+	err = sync_inode_metadata(inode, 1);
+	if (ret == 0)
+		ret = err;
+
+out:
+	/* check and advance again to catch errors after syncing out buffers */
+	err = file_check_and_advance_wb_err(file);
+	if (ret == 0)
+		ret = err;
+	return ret;
+}
+EXPORT_SYMBOL(generic_buffers_fsync_noflush);
+
+/**
+ * generic_buffers_fsync - generic buffer fsync implementation
+ * for simple filesystems with no inode lock
+ *
+ * @file:	file to synchronize
+ * @start:	start offset in bytes
+ * @end:	end offset in bytes (inclusive)
+ * @datasync:	only synchronize essential metadata if true
+ *
+ * This is a generic implementation of the fsync method for simple
+ * filesystems which track all non-inode metadata in the buffers list
+ * hanging off the address_space structure. This also makes sure that
+ * a device cache flush operation is called at the end.
+ */
+int generic_buffers_fsync(struct file *file, loff_t start, loff_t end,
+			  bool datasync)
+{
+	struct inode *inode = file->f_mapping->host;
+	int ret;
+
+	ret = generic_buffers_fsync_noflush(file, start, end, datasync);
+	if (!ret)
+		ret = blkdev_issue_flush(inode->i_sb->s_bdev);
+	return ret;
+}
+EXPORT_SYMBOL(generic_buffers_fsync);
+
 /*
  * Called when we've recently written block `bblock', and it is known that
  * `bblock' was for a buffer_boundary() buffer.  This means that the block at

include/linux/buffer_head.h +4 -0

index 8f14dca5fed7..105b67c2a7a8 100644
@@ -211,6 +211,10 @@ int inode_has_buffers(struct inode *);
 void invalidate_inode_buffers(struct inode *);
 int remove_inode_buffers(struct inode *inode);
 int sync_mapping_buffers(struct address_space *mapping);
+int generic_buffers_fsync_noflush(struct file *file, loff_t start, loff_t end,
+				  bool datasync);
+int generic_buffers_fsync(struct file *file, loff_t start, loff_t end,
+			  bool datasync);
 void clean_bdev_aliases(struct block_device *bdev, sector_t block,
 			sector_t len);
 static inline void clean_bdev_bh_alias(struct buffer_head *bh)

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #3 lore raw

PATCH v6 3/9 ext4: Use generic_buffers_fsync_noflush() implementation

In reply to Ritesh Harjani (IBM)

ext4 when got converted to iomap for dio, it copied __generic_file_fsync implementation to avoid taking inode_lock in order to avoid any deadlock (since iomap takes an inode_lock while calling generic_write_sync()).

The previous patch already added generic_buffers_fsync*() which does not take any inode_lock(). Hence kill the redundant code and use generic_buffers_fsync_noflush() function instead.

Tested-by Disha Goel <[email protected]>

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

---
 fs/ext4/fsync.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

fs/ext4/fsync.c +16 ~~-17~~

index f65fdb27ce14..9cd71d76622d 100644
@@ -28,6 +28,7 @@
 #include <linux/sched.h>
 #include <linux/writeback.h>
 #include <linux/blkdev.h>
+#include <linux/buffer_head.h>
 
 #include "ext4.h"
 #include "ext4_jbd2.h"

@@ -78,21 +79,13 @@ static int ext4_sync_parent(struct inode *inode)
 	return ret;
 }
 
-static int ext4_fsync_nojournal(struct inode *inode, bool datasync,
-				bool *needs_barrier)
+static int ext4_fsync_nojournal(struct file *file, loff_t start, loff_t end,
+				int datasync, bool *needs_barrier)
 {
-	int ret, err;
-
-	ret = sync_mapping_buffers(inode->i_mapping);
-	if (!(inode->i_state & I_DIRTY_ALL))
-		return ret;
-	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
-		return ret;
-
-	err = sync_inode_metadata(inode, 1);
-	if (!ret)
-		ret = err;
+	struct inode *inode = file->f_inode;
+	int ret;
 
+	ret = generic_buffers_fsync_noflush(file, start, end, datasync);
 	if (!ret)
 		ret = ext4_sync_parent(inode);
 	if (test_opt(inode->i_sb, BARRIER))

@@ -148,6 +141,14 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 		goto out;
 	}
 
+	if (!sbi->s_journal) {
+		ret = ext4_fsync_nojournal(file, start, end, datasync,
+					   &needs_barrier);
+		if (needs_barrier)
+			goto issue_flush;
+		goto out;
+	}
+
 	ret = file_write_and_wait_range(file, start, end);
 	if (ret)
 		goto out;

@@ -157,11 +158,9 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	 *  Metadata is in the journal, we wait for proper transaction to
 	 *  commit here.
 	 */
-	if (!sbi->s_journal)
-		ret = ext4_fsync_nojournal(inode, datasync, &needs_barrier);
-	else
-		ret = ext4_fsync_journal(inode, datasync, &needs_barrier);
+	ret = ext4_fsync_journal(inode, datasync, &needs_barrier);
 
+issue_flush:
 	if (needs_barrier) {
 		err = blkdev_issue_flush(inode->i_sb->s_bdev);
 		if (!ret)

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #4 lore raw

PATCH v6 4/9 ext2: Use generic_buffers_fsync() implementation

In reply to Ritesh Harjani (IBM)

Next patch converts ext2 to use iomap interface for DIO. iomap layer can call generic_write_sync() -> ext2_fsync() from iomap_dio_complete while still holding the inode_lock().

Now writeback from other paths doesn't need inode_lock(). It seems there is also no need of an inode_lock() for sync_mapping_buffers(). It uses it's own mapping->private_lock for it's buffer list handling.
Hence this patch is in preparation to move ext2 to iomap. This uses generic_buffers_fsync() which does not take any inode_lock() in ext2_fsync().

Tested-by Disha Goel <[email protected]>

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

---
 fs/ext2/file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/ext2/file.c +2 -1

index 6b4bebe982ca..749163787139 100644
@@ -25,6 +25,7 @@
 #include <linux/quotaops.h>
 #include <linux/iomap.h>
 #include <linux/uio.h>
+#include <linux/buffer_head.h>
 #include "ext2.h"
 #include "xattr.h"
 #include "acl.h"

@@ -153,7 +154,7 @@ int ext2_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	int ret;
 	struct super_block *sb = file->f_mapping->host->i_sb;
 
-	ret = generic_file_fsync(file, start, end, datasync);
+	ret = generic_buffers_fsync(file, start, end, datasync);
 	if (ret == -EIO)
 		/* We don't really know where the IO error happened... */
 		ext2_error(sb, __func__,

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #5 lore raw

PATCH v6 5/9 ext2: Move direct-io to use iomap

In reply to Ritesh Harjani (IBM)

This patch converts ext2 direct-io path to iomap interface.
- This also takes care of DIO_SKIP_HOLES part in which we return -ENOTBLK from ext2_iomap_begin(), in case if the write is done on a hole.
- This fallbacks to buffered-io in case of DIO_SKIP_HOLES or in case of a partial write or if any error is detected in ext2_iomap_end(). We try to return -ENOTBLK in such cases.
- For any unaligned or extending DIO writes, we pass IOMAP_DIO_FORCE_WAIT flag to ensure synchronous writes.
- For extending writes we set IOMAP_F_DIRTY in ext2_iomap_begin because otherwise with dsync writes on devices that support FUA, generic_write_sync won't be called and we might miss inode metadata updates.
- Since ext2 already now uses _nolock vartiant of sync write. Hence there is no inode lock problem with iomap in this patch.
- ext2_iomap_ops are now being shared by DIO, DAX & fiemap path

Tested-by Disha Goel <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 fs/ext2/ext2.h  |   1 +
 fs/ext2/file.c  | 115 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext2/inode.c |  53 ++++++++++++++--------
 3 files changed, 150 insertions(+), 19 deletions(-)

fs/ext2/ext2.h +1 -0

index cb78d7dcfb95..00168447d48c 100644
@@ -753,6 +753,7 @@ extern unsigned long ext2_count_free (struct buffer_head *, unsigned);
 extern struct inode *ext2_iget (struct super_block *, unsigned long);
 extern int ext2_write_inode (struct inode *, struct writeback_control *);
 extern void ext2_evict_inode(struct inode *);
+void ext2_write_failed(struct address_space *mapping, loff_t to);
 extern int ext2_get_block(struct inode *, sector_t, struct buffer_head *, int);
 extern int ext2_setattr (struct mnt_idmap *, struct dentry *, struct iattr *);
 extern int ext2_getattr (struct mnt_idmap *, const struct path *,

fs/ext2/file.c +115 -0

133 lines not on this page · raw message

fs/ext2/inode.c +34 ~~-19~~

91 lines not on this page · raw message

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #6 lore raw

PATCH v6 6/9 fs.h: Add TRACE_IOCB_STRINGS for use in trace points

In reply to Ritesh Harjani (IBM)

Add TRACE_IOCB_STRINGS macro which can be used in the trace point patch to print different flag values with meaningful string output.

Tested-by Disha Goel <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 include/linux/fs.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

include/linux/fs.h +14 -0

index c85916e9f7db..bdc1f7ed2aba 100644
@@ -340,6 +340,20 @@ enum rw_hint {
 /* can use bio alloc cache */
 #define IOCB_ALLOC_CACHE	(1 << 21)
 
+/* for use in trace events */
+#define TRACE_IOCB_STRINGS \
+	{ IOCB_HIPRI, "HIPRI"	}, \
+	{ IOCB_DSYNC, "DSYNC"	}, \
+	{ IOCB_SYNC, "SYNC"	}, \
+	{ IOCB_NOWAIT, "NOWAIT" }, \
+	{ IOCB_APPEND, "APPEND" }, \
+	{ IOCB_EVENTFD, "EVENTFD"}, \
+	{ IOCB_DIRECT, "DIRECT" }, \
+	{ IOCB_WRITE, "WRITE"	}, \
+	{ IOCB_WAITQ, "WAITQ"	}, \
+	{ IOCB_NOIO, "NOIO"	}, \
+	{ IOCB_ALLOC_CACHE, "ALLOC_CACHE" }
+
 struct kiocb {
 	struct file		*ki_filp;
 	loff_t			ki_pos;

Signature · 1 line

2.39.2

Darrick J. Wong

Reviewed-by on v6 6/9

2023-04-21 15:56 UTC #7 lore raw

Re: [PATCHv6 6/9] fs.h: Add TRACE_IOCB_STRINGS for use in trace points

Quoted 6 lines · Ritesh Harjani (IBM)

Add TRACE_IOCB_STRINGS macro which can be used in the trace point patch to print different flag values with meaningful string output.
Tested-by Disha Goel <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>

Looks good, will fix the indentation problems on commit.

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 32 lines

---
 include/linux/fs.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

include/linux/fs.h +14 -0

index c85916e9f7db..bdc1f7ed2aba 100644
@@ -340,6 +340,20 @@ enum rw_hint {
 /* can use bio alloc cache */
 #define IOCB_ALLOC_CACHE	(1 << 21)
 
+/* for use in trace events */
+#define TRACE_IOCB_STRINGS \
+	{ IOCB_HIPRI, "HIPRI"	}, \
+	{ IOCB_DSYNC, "DSYNC"	}, \
+	{ IOCB_SYNC, "SYNC"	}, \
+	{ IOCB_NOWAIT, "NOWAIT" }, \
+	{ IOCB_APPEND, "APPEND" }, \
+	{ IOCB_EVENTFD, "EVENTFD"}, \
+	{ IOCB_DIRECT, "DIRECT" }, \
+	{ IOCB_WRITE, "WRITE"	}, \
+	{ IOCB_WAITQ, "WAITQ"	}, \
+	{ IOCB_NOIO, "NOIO"	}, \
+	{ IOCB_ALLOC_CACHE, "ALLOC_CACHE" }
+
 struct kiocb {
 	struct file		*ki_filp;
 	loff_t			ki_pos;

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #8 lore raw

PATCH v6 7/9 ext2: Add direct-io trace points

In reply to Ritesh Harjani (IBM)

This patch adds the trace point to ext2 direct-io apis in fs/ext2/file.c

Here is how the output looks like

        a.out-467865 [006]  6758.170968: ext2_dio_write_begin: dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 4096 flags DIRECT|WRITE aio 1 ret 0
        a.out-467865 [006]  6758.171061: ext2_dio_write_end:   dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 0 flags DIRECT|WRITE aio 1 ret -529
kworker/3:153-444162 [003]  6758.171252: ext2_dio_write_endio: dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 4096 flags DIRECT|WRITE aio 1 ret 0
        a.out-468222 [001]  6761.628924: ext2_dio_read_begin:  dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 4096 flags DIRECT aio 1 ret 0
        a.out-468222 [001]  6761.629063: ext2_dio_read_end:    dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 0 flags DIRECT aio 1 ret -529
        a.out-468428 [005]  6763.937454: ext2_dio_write_begin: dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 4096 flags DIRECT aio 0 ret 0
        a.out-468428 [005]  6763.937829: ext2_dio_write_endio: dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 4096 flags DIRECT aio 0 ret 0
        a.out-468428 [005]  6763.937847: ext2_dio_write_end:   dev 7:12 ino 0xe isize 0x1000 pos 0x1000 len 0 flags DIRECT aio 0 ret 4096
        a.out-468609 [000]  6765.702878: ext2_dio_read_begin:  dev 7:12 ino 0xe isize 0x1000 pos 0x0 len 4096 flags DIRECT aio 0 ret 0
        a.out-468609 [000]  6765.703243: ext2_dio_read_end:    dev 7:12 ino 0xe isize 0x1000 pos 0x1000 len 0 flags DIRECT aio 0 ret 4096

Reported-and-tested-by Disha Goel <[email protected]>

[Need to add CFLAGS_trace for fixing unable to find trace file problem]

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

---
 fs/ext2/Makefile |  5 ++-
 fs/ext2/file.c   | 10 +++++-
 fs/ext2/trace.c  |  6 ++++
 fs/ext2/trace.h  | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 2 deletions(-)
 create mode 100644 fs/ext2/trace.c
 create mode 100644 fs/ext2/trace.h

fs/ext2/Makefile +4 -1

index 311479d864a7..8860948ef9ca 100644
@@ -6,7 +6,10 @@
 obj-$(CONFIG_EXT2_FS) += ext2.o
 
 ext2-y := balloc.o dir.o file.o ialloc.o inode.o \
-	  ioctl.o namei.o super.o symlink.o
+	  ioctl.o namei.o super.o symlink.o trace.o
+
+# For tracepoints to include our trace.h from tracepoint infrastructure
+CFLAGS_trace.o := -I$(src)
 
 ext2-$(CONFIG_EXT2_FS_XATTR)	 += xattr.o xattr_user.o xattr_trusted.o
 ext2-$(CONFIG_EXT2_FS_POSIX_ACL) += acl.o

fs/ext2/file.c +9 -1

index 98add36c1a59..7a32f202908e 100644
@@ -29,6 +29,7 @@
 #include "ext2.h"
 #include "xattr.h"
 #include "acl.h"
+#include "trace.h"
 
 #ifdef CONFIG_FS_DAX
 static ssize_t ext2_dax_read_iter(struct kiocb *iocb, struct iov_iter *to)

@@ -168,9 +169,11 @@ static ssize_t ext2_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	struct inode *inode = file->f_mapping->host;
 	ssize_t ret;
 
+	trace_ext2_dio_read_begin(iocb, to, 0);
 	inode_lock_shared(inode);
 	ret = iomap_dio_rw(iocb, to, &ext2_iomap_ops, NULL, 0, NULL, 0);
 	inode_unlock_shared(inode);
+	trace_ext2_dio_read_end(iocb, to, ret);
 
 	return ret;
 }

@@ -198,6 +201,7 @@ static int ext2_dio_write_end_io(struct kiocb *iocb, ssize_t size,
 		mark_inode_dirty(inode);
 	}
 out:
+	trace_ext2_dio_write_endio(iocb, size, error);
 	return error;
 }

@@ -214,7 +218,9 @@ static ssize_t ext2_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	unsigned long blocksize = inode->i_sb->s_blocksize;
 	loff_t offset = iocb->ki_pos;
 	loff_t count = iov_iter_count(from);
+	ssize_t status = 0;
 
+	trace_ext2_dio_write_begin(iocb, from, 0);
 	inode_lock(inode);
 	ret = generic_write_checks(iocb, from);
 	if (ret <= 0)

@@ -242,7 +248,6 @@ static ssize_t ext2_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	/* handle case for partial write and for fallback to buffered write */
 	if (ret >= 0 && iov_iter_count(from)) {
 		loff_t pos, endbyte;
-		ssize_t status;
 		int ret2;
 
 		iocb->ki_flags &= ~IOCB_DIRECT;

@@ -268,6 +273,9 @@ static ssize_t ext2_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 out_unlock:
 	inode_unlock(inode);
+	if (status)
+		trace_ext2_dio_write_buff_end(iocb, from, status);
+	trace_ext2_dio_write_end(iocb, from, ret);
 	return ret;
 }

fs/ext2/trace.c new file +6 -0

new file mode 100644
index 000000000000..b01cdf6526fd
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "ext2.h"
+#include <linux/uio.h>
+
+#define CREATE_TRACE_POINTS
+#include "trace.h"

fs/ext2/trace.h new file +94 -0

94 lines not on this page · raw message

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:47 UTC #9 lore raw

PATCH v6 8/9 iomap: Remove IOMAP_DIO_NOSYNC unused dio flag

In reply to Ritesh Harjani (IBM)

IOMAP_DIO_NOSYNC earlier was added for use in btrfs. But it seems for aio dsync writes this is not useful anyway. For aio dsync case, we we queue the request and return -EIOCBQUEUED. Now, since IOMAP_DIO_NOSYNC doesn't let iomap_dio_complete() to call generic_write_sync(), hence we may lose the sync write.

Hence kill this flag as it is not in use by any FS now.

Tested-by Disha Goel <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Darrick J. Wong <[email protected]>

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

---
 fs/iomap/direct-io.c  | 2 +-
 include/linux/iomap.h | 6 ------
 2 files changed, 1 insertion(+), 7 deletions(-)

fs/iomap/direct-io.c +1 -1

index f771001574d0..36ab1152dbea 100644
@@ -541,7 +541,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		}
 
 		/* for data sync or sync, we need sync completion processing */
-		if (iocb_is_dsync(iocb) && !(dio_flags & IOMAP_DIO_NOSYNC)) {
+		if (iocb_is_dsync(iocb)) {
 			dio->flags |= IOMAP_DIO_NEED_SYNC;
 
 		       /*

include/linux/iomap.h +0 -6

index 0f8123504e5e..e2b836c2e119 100644
@@ -377,12 +377,6 @@ struct iomap_dio_ops {
  */
 #define IOMAP_DIO_PARTIAL		(1 << 2)
 
-/*
- * The caller will sync the write if needed; do not sync it within
- * iomap_dio_rw.  Overrides IOMAP_DIO_FORCE_WAIT.
- */
-#define IOMAP_DIO_NOSYNC		(1 << 3)
-
 ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		const struct iomap_ops *ops, const struct iomap_dio_ops *dops,
 		unsigned int dio_flags, void *private, size_t done_before);

Signature · 1 line

2.39.2

Darrick J. Wong

Reviewed-by on v6 8/9

2023-04-21 15:56 UTC #10 lore raw

Re: [PATCHv6 8/9] iomap: Remove IOMAP_DIO_NOSYNC unused dio flag

Quoted 6 lines · Ritesh Harjani (IBM)

IOMAP_DIO_NOSYNC earlier was added for use in btrfs. But it seems for aio dsync writes this is not useful anyway. For aio dsync case, we we queue the request and return -EIOCBQUEUED. Now, since IOMAP_DIO_NOSYNC doesn't let iomap_dio_complete() to call generic_write_sync(), hence we may lose the sync write.

Quoted 6 lines

Hence kill this flag as it is not in use by any FS now.
Tested-by Disha Goel <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
Reviewed-by Darrick J. Wong <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 38 lines

---
 fs/iomap/direct-io.c  | 2 +-
 include/linux/iomap.h | 6 ------
 2 files changed, 1 insertion(+), 7 deletions(-)

fs/iomap/direct-io.c +1 -1

index f771001574d0..36ab1152dbea 100644
@@ -541,7 +541,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		}
 
 		/* for data sync or sync, we need sync completion processing */
-		if (iocb_is_dsync(iocb) && !(dio_flags & IOMAP_DIO_NOSYNC)) {
+		if (iocb_is_dsync(iocb)) {
 			dio->flags |= IOMAP_DIO_NEED_SYNC;
 
 		       /*

include/linux/iomap.h +0 -6

index 0f8123504e5e..e2b836c2e119 100644
@@ -377,12 +377,6 @@ struct iomap_dio_ops {
  */
 #define IOMAP_DIO_PARTIAL		(1 << 2)
 
-/*
- * The caller will sync the write if needed; do not sync it within
- * iomap_dio_rw.  Overrides IOMAP_DIO_FORCE_WAIT.
- */
-#define IOMAP_DIO_NOSYNC		(1 << 3)
-
 ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		const struct iomap_ops *ops, const struct iomap_dio_ops *dops,
 		unsigned int dio_flags, void *private, size_t done_before);

Signature · 1 line

2.39.2

Ritesh Harjani (IBM)

author reviewer

2023-04-21 09:48 UTC #11 lore raw

PATCH v6 9/9 iomap: Add DIO tracepoints

In reply to Ritesh Harjani (IBM)

Add trace_iomap_dio_rw_begin, trace_iomap_dio_rw_queued and trace_iomap_dio_complete tracepoint.
trace_iomap_dio_rw_queued is mostly only to know that the request was queued and -EIOCBQUEUED was returned. It is mostly trace_iomap_dio_rw_begin & trace_iomap_dio_complete which has all the details.

      a.out-2073  [006]   134.225717: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x0 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT|WRITE dio_flags DIO_FORCE_WAIT aio 1
      a.out-2073  [006]   134.226234: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT|WRITE aio 1 error 0 ret 4096
      a.out-2074  [006]   136.225975: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 1
      a.out-2074  [006]   136.226173: iomap_dio_rw_queued:  dev 7:7 ino 0xe size 0x1000 offset 0x1000 length 0x0
ksoftirqd/3-31    [003]   136.226389: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 1 error 0 ret 4096
      a.out-2075  [003]   141.674969: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT|WRITE dio_flags  aio 1
      a.out-2075  [003]   141.676085: iomap_dio_rw_queued:  dev 7:7 ino 0xe size 0x1000 offset 0x1000 length 0x0
kworker/2:0-27    [002]   141.676432: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT|WRITE aio 1 error 0 ret 4096
      a.out-2077  [006]   143.443746: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 1
      a.out-2077  [006]   143.443866: iomap_dio_rw_queued:  dev 7:7 ino 0xe size 0x1000 offset 0x1000 length 0x0
ksoftirqd/5-41    [005]   143.444134: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 1 error 0 ret 4096
      a.out-2078  [007]   146.716833: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 0
      a.out-2078  [007]   146.717639: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 0 error 0 ret 4096
      a.out-2079  [006]   148.972605: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 0
      a.out-2079  [006]   148.973099: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 0 error 0 ret 4096

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 fs/iomap/direct-io.c |  7 +++-
 fs/iomap/trace.c     |  1 +
 fs/iomap/trace.h     | 78 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

fs/iomap/direct-io.c +6 -1

index 36ab1152dbea..019cc87d0fb3 100644
@@ -130,6 +130,7 @@ ssize_t iomap_dio_complete(struct iomap_dio *dio)
 	if (ret > 0)
 		ret += dio->done_before;
 
+	trace_iomap_dio_complete(iocb, dio->error, ret);
 	kfree(dio);
 
 	return ret;

@@ -493,6 +494,8 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct blk_plug plug;
 	struct iomap_dio *dio;
 
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before);
+
 	if (!iomi.len)
 		return NULL;

@@ -650,8 +653,10 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	 */
 	dio->wait_for_completion = wait_for_completion;
 	if (!atomic_dec_and_test(&dio->ref)) {
-		if (!wait_for_completion)
+		if (!wait_for_completion) {
+			trace_iomap_dio_rw_queued(inode, iomi.pos, iomi.len);
 			return ERR_PTR(-EIOCBQUEUED);
+		}
 
 		for (;;) {
 			set_current_state(TASK_UNINTERRUPTIBLE);

fs/iomap/trace.c +1 -0

index da217246b1a9..728d5443daf5 100644
@@ -3,6 +3,7 @@
  * Copyright (c) 2019 Christoph Hellwig
  */
 #include <linux/iomap.h>
+#include <linux/uio.h>
 
 /*
  * We include this last to have the helpers above available for the trace

fs/iomap/trace.h +78 -0

96 lines not on this page · raw message

Signature · 1 line

2.39.2

Darrick J. Wong

Reviewed-by on v6 9/9

2023-04-21 15:56 UTC #12 lore raw

Re: [PATCHv6 9/9] iomap: Add DIO tracepoints

Quoted 89 lines · Ritesh Harjani (IBM)

      a.out-2073  [006]   134.225717: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x0 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT|WRITE dio_flags DIO_FORCE_WAIT aio 1
      a.out-2073  [006]   134.226234: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT|WRITE aio 1 error 0 ret 4096
      a.out-2074  [006]   136.225975: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 1
      a.out-2074  [006]   136.226173: iomap_dio_rw_queued:  dev 7:7 ino 0xe size 0x1000 offset 0x1000 length 0x0
ksoftirqd/3-31    [003]   136.226389: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 1 error 0 ret 4096
      a.out-2075  [003]   141.674969: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT|WRITE dio_flags  aio 1
      a.out-2075  [003]   141.676085: iomap_dio_rw_queued:  dev 7:7 ino 0xe size 0x1000 offset 0x1000 length 0x0
kworker/2:0-27    [002]   141.676432: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT|WRITE aio 1 error 0 ret 4096
      a.out-2077  [006]   143.443746: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 1
      a.out-2077  [006]   143.443866: iomap_dio_rw_queued:  dev 7:7 ino 0xe size 0x1000 offset 0x1000 length 0x0
ksoftirqd/5-41    [005]   143.444134: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 1 error 0 ret 4096
      a.out-2078  [007]   146.716833: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 0
      a.out-2078  [007]   146.717639: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 0 error 0 ret 4096
      a.out-2079  [006]   148.972605: iomap_dio_rw_begin:   dev 7:7 ino 0xe size 0x1000 offset 0x0 length 0x1000 done_before 0x0 flags DIRECT dio_flags  aio 0
      a.out-2079  [006]   148.973099: iomap_dio_complete:   dev 7:7 ino 0xe size 0x1000 offset 0x1000 flags DIRECT aio 0 error 0 ret 4096

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 fs/iomap/direct-io.c |  7 +++-
 fs/iomap/trace.c     |  1 +
 fs/iomap/trace.h     | 78 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

fs/iomap/direct-io.c +6 -1

index 36ab1152dbea..019cc87d0fb3 100644
@@ -130,6 +130,7 @@ ssize_t iomap_dio_complete(struct iomap_dio *dio)
 	if (ret > 0)
 		ret += dio->done_before;
 
+	trace_iomap_dio_complete(iocb, dio->error, ret);
 	kfree(dio);
 
 	return ret;

@@ -493,6 +494,8 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct blk_plug plug;
 	struct iomap_dio *dio;
 
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before);
+
 	if (!iomi.len)
 		return NULL;

@@ -650,8 +653,10 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	 */
 	dio->wait_for_completion = wait_for_completion;
 	if (!atomic_dec_and_test(&dio->ref)) {
-		if (!wait_for_completion)
+		if (!wait_for_completion) {
+			trace_iomap_dio_rw_queued(inode, iomi.pos, iomi.len);
 			return ERR_PTR(-EIOCBQUEUED);
+		}
 
 		for (;;) {
 			set_current_state(TASK_UNINTERRUPTIBLE);

fs/iomap/trace.c +1 -0

index da217246b1a9..728d5443daf5 100644
@@ -3,6 +3,7 @@
  * Copyright (c) 2019 Christoph Hellwig
  */
 #include <linux/iomap.h>
+#include <linux/uio.h>
 
 /*
  * We include this last to have the helpers above available for the trace

fs/iomap/trace.h +1 -0

index f6ea9540d082..448b82d16c0b 100644
@@ -83,6 +83,7 @@ DEFINE_RANGE_EVENT(iomap_writepage);
 DEFINE_RANGE_EVENT(iomap_release_folio);
 DEFINE_RANGE_EVENT(iomap_invalidate_folio);
 DEFINE_RANGE_EVENT(iomap_dio_invalidate_fail);
+DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
 
 #define IOMAP_TYPE_STRINGS \
 	{ IOMAP_HOLE,		"HOLE" }, \

fs/iomap/trace.h DEFINE_RANGE_EVENT(iomap_dio_invalidate_fail); · line 114

Quoted 7 lines

fs/iomap/trace.h +4 -0

@@ -107,6 +108,11 @@ DEFINE_RANGE_EVENT(iomap_dio_invalidate_fail);
 	{ IOMAP_F_BUFFER_HEAD,	"BH" }, \
 	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }
 
+#define IOMAP_DIO_STRINGS \
+	{IOMAP_DIO_FORCE_WAIT, "DIO_FORCE_WAIT" }, \
+	{IOMAP_DIO_OVERWRITE_ONLY, "DIO_OVERWRITE_ONLY" }, \
+	{IOMAP_DIO_PARTIAL, "DIO_PARTIAL" }

I'll fix these indentation ^ problems on commit.

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 86 lines

fs/iomap/trace.h +73 -0

+
 DECLARE_EVENT_CLASS(iomap_class,
 	TP_PROTO(struct inode *inode, struct iomap *iomap),
 	TP_ARGS(inode, iomap),

@@ -183,6 +189,78 @@ TRACE_EVENT(iomap_iter,
 		   (void *)__entry->caller)
 );
 
+TRACE_EVENT(iomap_dio_rw_begin,
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,
+		 unsigned int dio_flags, size_t done_before),
+	TP_ARGS(iocb, iter, dio_flags, done_before),
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(ino_t,	ino)
+		__field(loff_t, isize)
+		__field(loff_t, pos)
+		__field(size_t,	count)
+		__field(size_t,	done_before)
+		__field(int,	ki_flags)
+		__field(unsigned int,	dio_flags)
+		__field(bool,	aio)
+	),
+	TP_fast_assign(
+		__entry->dev = file_inode(iocb->ki_filp)->i_sb->s_dev;
+		__entry->ino = file_inode(iocb->ki_filp)->i_ino;
+		__entry->isize = file_inode(iocb->ki_filp)->i_size;
+		__entry->pos = iocb->ki_pos;
+		__entry->count = iov_iter_count(iter);
+		__entry->done_before = done_before;
+		__entry->ki_flags = iocb->ki_flags;
+		__entry->dio_flags = dio_flags;
+		__entry->aio = !is_sync_kiocb(iocb);
+	),
+	TP_printk("dev %d:%d ino 0x%lx size 0x%llx offset 0x%llx length 0x%zx done_before 0x%zx flags %s dio_flags %s aio %d",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->ino,
+		  __entry->isize,
+		  __entry->pos,
+		  __entry->count,
+		  __entry->done_before,
+		  __print_flags(__entry->ki_flags, "|", TRACE_IOCB_STRINGS),
+		  __print_flags(__entry->dio_flags, "|", IOMAP_DIO_STRINGS),
+		  __entry->aio)
+);
+
+TRACE_EVENT(iomap_dio_complete,
+	TP_PROTO(struct kiocb *iocb, int error, ssize_t ret),
+	TP_ARGS(iocb, error, ret),
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(ino_t,	ino)
+		__field(loff_t, isize)
+		__field(loff_t, pos)
+		__field(int,	ki_flags)
+		__field(bool,	aio)
+		__field(int,	error)
+		__field(ssize_t, ret)
+	),
+	TP_fast_assign(
+		__entry->dev = file_inode(iocb->ki_filp)->i_sb->s_dev;
+		__entry->ino = file_inode(iocb->ki_filp)->i_ino;
+		__entry->isize = file_inode(iocb->ki_filp)->i_size;
+		__entry->pos = iocb->ki_pos;
+		__entry->ki_flags = iocb->ki_flags;
+		__entry->aio = !is_sync_kiocb(iocb);
+		__entry->error = error;
+		__entry->ret = ret;
+	),
+	TP_printk("dev %d:%d ino 0x%lx size 0x%llx offset 0x%llx flags %s aio %d error %d ret %zd",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->ino,
+		  __entry->isize,
+		  __entry->pos,
+		  __print_flags(__entry->ki_flags, "|", TRACE_IOCB_STRINGS),
+		  __entry->aio,
+		  __entry->error,
+		  __entry->ret)
+);
+
 #endif /* _IOMAP_TRACE_H */
 
 #undef TRACE_INCLUDE_PATH

Signature · 1 line

2.39.2

Jan Kara

reviewer

2023-04-21 11:23 UTC #13 lore raw

In reply to Ritesh Harjani (IBM)

Hello Ritesh,

Quoted 4 lines · Ritesh Harjani (IBM)

Hello All,
Please find the series which rewrites ext2 direct-io path to use modern iomap interface.

The patches now all look good to me. I'd like to discuss a bit how to merge them. The series has an ext4 cleanup (patch 3) and three iomap patches (patches 6, 8 and 9). Darrick, do you want to take the iomap patches through your tree?

The only dependency is that patch 7 for ext2 is dependent on definitions from patch 6 so I'd have to pull your branch into my tree. Or I can take all the iomap patches through my tree but for that it would be nice to have Darrick's acks.

I can take the ext4 patch through my tree unless Ted objects.

I guess I won't rush this for the coming merge window (unless Linus decides to do rc8) but once we settle on the merge strategy I'll push out some branch on which we can base further ext2 iomap conversion work.

Honza

Quoted 78 lines

PATCHv5 -> PATCHv6:
===================
1. Patch-2 Added generic_buffers_fsync_noflush() & generic_buffers_fsync() functions.
2. Patch-3 & Patch-4 to use above functions in ext4 & ext2.
3. Added Reviewed-by from Christoph on Patch-9 (iomap: Add DIO tracepoints)
RFCv4 -> PATCHv5:
=================
1. Added trace_iomap_dio_rw_begin tracepoint in __iomap_dio_rw()
2. Added Reviewed-by tags from Christoph
RFCv3 -> RFCV4:
===============
1. Renamed __generic_file_fsync_nolock() from libfs to generic_buffer_fsync() in fs/buffer.c
(Review comment from Christoph)
2. Fixed s/EVENTD/EVENTFD/ in TRACE_IOCB_STRINGS
3. Fixed few data types for parameters in ext2 trace patch (size_t && ssize_t)
4. Killed this patch "Minor refactor of iomap_dio_rw"
5. Changed iomap tracepoint patch and fixed the data types (size_t && ssize_t) (addressed review comments from Christoph)
RFCv2 -> RFCv3:
===============
1. Addressed minor review comments related to extern, parameter naming in function declaration, removing not required braces and shorting overly long lines.
2. Added Reviewed-by from various reviewers.
3. Fixed a warning & couple of compilation errors in Patch-7 (ext2 trace points) related to CFLAGS_trace & second related to unable to find function definition for iov_iter_count(). (requires uio.h file) CFLAGS_trace is required in Makefile so that it can find trace.h file from tracepoint infrastructure.
4. Changed naming of IOCB_STRINGS TO TRACE_IOCB_STRINGS.
5. Shortened naming of tracepoint events for ext2 dio.
6. Added iomap DIO tracepoint events.
7. Disha tested this series internally against Power with "auto" group for 4k and 64k blocksize configuration. Added her "Tested-by" tag in all DIO related patches. No new failures were reported.
Thanks everyone for the review and test. The series is looking good to me now. It has been tested on x86 and Power with different configurations. Please let me know if anything else is required on this.
v2: https://lore.kernel.org/all/[email protected]/
Ritesh Harjani (IBM) (9):
ext2/dax: Fix ext2_setsize when len is page aligned fs/buffer.c: Add generic_buffers_fsync*() implementation
ext4: Use generic_buffers_fsync_noflush() implementation
ext2: Use generic_buffers_fsync() implementation
ext2: Move direct-io to use iomap
fs.h: Add TRACE_IOCB_STRINGS for use in trace points
ext2: Add direct-io trace points
iomap: Remove IOMAP_DIO_NOSYNC unused dio flag
iomap: Add DIO tracepoints
 fs/buffer.c                 |  70 ++++++++++++++++++++
 fs/ext2/Makefile            |   5 +-
 fs/ext2/ext2.h              |   1 +
 fs/ext2/file.c              | 126 +++++++++++++++++++++++++++++++++++-
 fs/ext2/inode.c             |  58 ++++++++++-------
 fs/ext2/trace.c             |   6 ++
 fs/ext2/trace.h             |  94 +++++++++++++++++++++++++++
 fs/ext4/fsync.c             |  33 +++++-----
 fs/iomap/direct-io.c        |   9 ++-
 fs/iomap/trace.c            |   1 +
 fs/iomap/trace.h            |  78 ++++++++++++++++++++++
 include/linux/buffer_head.h |   4 ++
 include/linux/fs.h          |  14 ++++
 include/linux/iomap.h       |   6 --
 14 files changed, 456 insertions(+), 49 deletions(-)
 create mode 100644 fs/ext2/trace.c
 create mode 100644 fs/ext2/trace.h
--
2.39.2

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Ritesh Harjani (IBM)

author reviewer

2023-04-21 12:06 UTC #14 lore raw

Quoted 9 lines · Jan Kara

Hello Ritesh,
Quoted 4 lines · Ritesh Harjani (IBM)
Hello All,
Please find the series which rewrites ext2 direct-io path to use modern iomap interface.
The patches now all look good to me. I'd like to discuss a bit how to merge

Thanks Jan,

Quoted 1 line

them. The series has an ext4 cleanup (patch 3) and three iomap patches

Also Patch-3 is on top of ext4 journalled data patch series of yours, otheriwse we might see a minor merge conflict.

https://lore.kernel.org/all/[email protected]/

Quoted 5 lines

(patches 6, 8 and 9). Darrick, do you want to take the iomap patches through your tree?
The only dependency is that patch 7 for ext2 is dependent on definitions from patch 6

That's right. Patch 6 defines TRACE_IOCB_STRINGS definition which both ext2 and iomap tracepoints depend upon.

Quoted 5 lines

so I'd have to pull your branch into my tree. Or I can take
all the iomap patches through my tree but for that it would be nice to have Darrick's acks.
I can take the ext4 patch through my tree unless Ted objects.

Sure, we might have to merge with Ted's ext4 tree as well to avoid the merge conflict I mentioned above.

Quoted 3 lines

I guess I won't rush this for the coming merge window (unless Linus decides to do rc8) but once we settle on the merge strategy I'll push out some

Ok.

Quoted 2 lines

branch on which we can base further ext2 iomap conversion work.

Sure, will this branch also gets reflected in linux-next for wider testing?

-ritesh

Darrick J. Wong

2023-04-21 15:41 UTC #15 lore raw

Quoted 15 lines · Ritesh Harjani

Quoted 9 lines · Jan Kara
Hello Ritesh,
Quoted 4 lines · Ritesh Harjani (IBM)
Hello All,
Please find the series which rewrites ext2 direct-io path to use modern iomap interface.
The patches now all look good to me. I'd like to discuss a bit how to merge
Thanks Jan,

Quoted 9 lines

Quoted 1 line
them. The series has an ext4 cleanup (patch 3) and three iomap patches
Also Patch-3 is on top of ext4 journalled data patch series of yours, otheriwse we might see a minor merge conflict.
https://lore.kernel.org/all/[email protected]/
Quoted 2 lines
(patches 6, 8 and 9). Darrick, do you want to take the iomap patches through your tree?

Hmm. I could do that for 6.4 since the first one should be trivially verifiable and so far Linus hasn't objected to patches that add tracepoints being thrown into for-next right at the start of the merge window.

--D

Quoted 27 lines

Quoted 2 lines
The only dependency is that patch 7 for ext2 is dependent on definitions from patch 6
That's right. Patch 6 defines TRACE_IOCB_STRINGS definition which both ext2 and iomap tracepoints depend upon.
Quoted 5 lines
so I'd have to pull your branch into my tree. Or I can take
all the iomap patches through my tree but for that it would be nice to have Darrick's acks.
I can take the ext4 patch through my tree unless Ted objects.
Sure, we might have to merge with Ted's ext4 tree as well to avoid the merge conflict I mentioned above.
Quoted 3 lines
I guess I won't rush this for the coming merge window (unless Linus decides to do rc8) but once we settle on the merge strategy I'll push out some
Ok.
Quoted 2 lines
branch on which we can base further ext2 iomap conversion work.
Sure, will this branch also gets reflected in linux-next for wider testing?
-ritesh

Jan Kara

reviewer

2023-04-24 09:43 UTC #16 lore raw

Quoted 23 lines · Darrick J. Wong

Quoted 22 lines · Ritesh Harjani
Quoted 9 lines · Jan Kara
Hello Ritesh,
Quoted 4 lines · Ritesh Harjani (IBM)
Hello All,
Please find the series which rewrites ext2 direct-io path to use modern iomap interface.
The patches now all look good to me. I'd like to discuss a bit how to merge
Thanks Jan,
Quoted 1 line
them. The series has an ext4 cleanup (patch 3) and three iomap patches
Also Patch-3 is on top of ext4 journalled data patch series of yours, otheriwse we might see a minor merge conflict.
https://lore.kernel.org/all/[email protected]/

Quoted 7 lines

Quoted 2 lines
Quoted 2 lines
(patches 6, 8 and 9). Darrick, do you want to take the iomap patches through your tree?
Hmm. I could do that for 6.4 since the first one should be trivially verifiable and so far Linus hasn't objected to patches that add tracepoints being thrown into for-next right at the start of the merge window.

Great! Then I'll wait a bit until rc1 when your iomap tree + Ted's ext4 tree should have landed in Linus' tree and push the ext2+ext4 patches into my tree to a branch based on rc1 which should deal with all the merge troubles.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Jan Kara

reviewer

2023-04-24 09:45 UTC #17 lore raw

In reply to Ritesh Harjani (IBM)

Quoted 7 lines · Ritesh Harjani

Quoted 1 line · Jan Kara
them. The series has an ext4 cleanup (patch 3) and three iomap patches
Also Patch-3 is on top of ext4 journalled data patch series of yours, otheriwse we might see a minor merge conflict.
https://lore.kernel.org/all/[email protected]/

Yes, I have noticed :)

Quoted 9 lines

Quoted 2 lines
I guess I won't rush this for the coming merge window (unless Linus decides to do rc8) but once we settle on the merge strategy I'll push out some
Ok.
Quoted 2 lines
branch on which we can base further ext2 iomap conversion work.
Sure, will this branch also gets reflected in linux-next for wider testing?

Yes, the branch I'll push the ext2+ext4 patches to will be included in linux-next (I'll merge it to for-next branch).

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR