Skip to contentSkip to conversation

ext2: DIO to use iomap

RFC Superseded Ritesh Harjani (IBM) 5 participants 33 messages Apr 13, 2023 – Apr 17, 2023

Subsystems (4)
VFS ext4 file system ext2 file system Filesystems iomap

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Ritesh Harjani (IBM)

author reviewer
#0 lore raw

v3 cover ext2: DIO to use iomap

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

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 now 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) (10):
ext2/dax: Fix ext2_setsize when len is page aligned
libfs: Add __generic_file_fsync_nolock implementation
ext4: Use __generic_file_fsync_nolock implementation
ext2: Use __generic_file_fsync_nolock 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: Minor refactor of iomap_dio_rw
iomap: Add trace points for DIO path

 fs/ext2/Makefile      |   5 +-
 fs/ext2/ext2.h        |   1 +
 fs/ext2/file.c        | 127 +++++++++++++++++++++++++++++++++++++++++-
 fs/ext2/inode.c       |  58 +++++++++++--------
 fs/ext2/trace.c       |   6 ++
 fs/ext2/trace.h       |  94 +++++++++++++++++++++++++++++++
 fs/ext4/fsync.c       |  31 +++++------
 fs/iomap/direct-io.c  |  16 ++++--
 fs/iomap/trace.c      |   1 +
 fs/iomap/trace.h      |  90 ++++++++++++++++++++++++++++++
 fs/libfs.c            |  43 ++++++++++++++
 include/linux/fs.h    |  16 ++++++
 include/linux/iomap.h |   6 --
 13 files changed, 444 insertions(+), 50 deletions(-)
 create mode 100644 fs/ext2/trace.c
 create mode 100644 fs/ext2/trace.h

--
2.39.2

author reviewer
#1 lore raw

RFC v3 1/10 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
author reviewer
#2 lore raw

RFC v3 2/10 libfs: Add __generic_file_fsync_nolock 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 a _nolock variant of this function in libfs 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. Hence let's add a helper API and use it both in ext2 and ext4.

Later we can review other filesystems as well to see if we can make _nolock as the default path if inode_lock() is not necessary here.

Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/libfs.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 45 insertions(+)
fs/libfs.c +43 -0
index 4eda519c3002..054f2e0ab3cb 100644
@@ -1110,6 +1110,49 @@ struct dentry *generic_fh_to_parent(struct super_block *sb, struct fid *fid,
 }
 EXPORT_SYMBOL_GPL(generic_fh_to_parent);
 
+/**
+ * __generic_file_fsync_nolock - generic 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_file_fsync_nolock(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_file_fsync_nolock);
+
 /**
  * __generic_file_fsync - generic fsync implementation for simple filesystems
  *
include/linux/fs.h +2 -0
index c85916e9f7db..9ca3813f43e2 100644
@@ -2935,6 +2935,8 @@ extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
 extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
 		const void __user *from, size_t count);
 
+int __generic_file_fsync_nolock(struct file *file, loff_t start, loff_t end,
+				bool datasync);
 extern int __generic_file_fsync(struct file *, loff_t, loff_t, int);
 extern int generic_file_fsync(struct file *, loff_t, loff_t, int);
Signature · 1 line
2.39.2
#3 lore raw

Re: [RFCv3 02/10] libfs: Add __generic_file_fsync_nolock implementation

Still no fan of the naming and placement here. This is specific to the fs/buffer.c infrastructure.

reviewer
#4 lore raw

Re: [RFCv3 02/10] libfs: Add __generic_file_fsync_nolock implementation

Quoted 2 lines · Christoph Hellwig

Still no fan of the naming and placement here. This is specific to the fs/buffer.c infrastructure.

I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and creating the new function there if it makes you happier. But I think function names should be consistent (hence the new function would be named __generic_file_fsync_nolock()). I agree the name is not ideal and would use cleanup (along with transitioning everybody to not take i_rwsem) but I don't want to complicate this series by touching 13+ callsites of generic_file_fsync() and __generic_file_fsync(). That's for a separate series.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#5 lore raw

Re: [RFCv3 02/10] libfs: Add __generic_file_fsync_nolock implementation

Quoted 4 lines · Jan Kara
Quoted 2 lines · Christoph Hellwig

Still no fan of the naming and placement here. This is specific to the fs/buffer.c infrastructure.

Quoted 8 lines

I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and creating the new function there if it makes you happier. But I think function names should be consistent (hence the new function would be named __generic_file_fsync_nolock()). I agree the name is not ideal and would use cleanup (along with transitioning everybody to not take i_rwsem) but I don't want to complicate this series by touching 13+ callsites of generic_file_fsync() and __generic_file_fsync(). That's for a separate series.

I would not change the existing function. Just do the right thing for the new helper and slowly migrate over without complicating this series.

reviewer
#6 lore raw

Re: [RFCv3 02/10] libfs: Add __generic_file_fsync_nolock implementation

Quoted 14 lines · Christoph Hellwig
Quoted 4 lines · Jan Kara
Quoted 2 lines · Christoph Hellwig

Still no fan of the naming and placement here. This is specific to the fs/buffer.c infrastructure.

Quoted 8 lines

I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and creating the new function there if it makes you happier. But I think function names should be consistent (hence the new function would be named __generic_file_fsync_nolock()). I agree the name is not ideal and would use cleanup (along with transitioning everybody to not take i_rwsem) but I don't want to complicate this series by touching 13+ callsites of generic_file_fsync() and __generic_file_fsync(). That's for a separate series.

Quoted 2 lines

I would not change the existing function. Just do the right thing for the new helper and slowly migrate over without complicating this series.

OK, I can live with that temporary naming inconsistency I guess. So the function will be __buffer_file_fsync()?

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author reviewer
#7 lore raw

Re: [RFCv3 02/10] libfs: Add __generic_file_fsync_nolock implementation

Quoted 15 lines · Jan Kara
Quoted 14 lines · Christoph Hellwig
Quoted 12 lines · Jan Kara
Quoted 2 lines · Christoph Hellwig

Still no fan of the naming and placement here. This is specific to the fs/buffer.c infrastructure.

I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and creating the new function there if it makes you happier. But I think function names should be consistent (hence the new function would be named __generic_file_fsync_nolock()). I agree the name is not ideal and would use cleanup (along with transitioning everybody to not take i_rwsem) but I don't want to complicate this series by touching 13+ callsites of generic_file_fsync() and __generic_file_fsync(). That's for a separate series.

Quoted 5 lines
Quoted 2 lines

I would not change the existing function. Just do the right thing for the new helper and slowly migrate over without complicating this series.

OK, I can live with that temporary naming inconsistency I guess. So the function will be __buffer_file_fsync()?

This name was suggested before, so if that's ok I will go with this - "generic_buffer_fsync()". It's definition will lie in fs/buffer.c and it's declaration in include/linux/buffer_head.h

Is that ok?

-ritesh

Quoted 5 lines

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

reviewer
#8 lore raw

Re: [RFCv3 02/10] libfs: Add __generic_file_fsync_nolock implementation

Quoted 20 lines · Ritesh Harjani
Quoted 18 lines · Jan Kara
Quoted 16 lines · Christoph Hellwig
Quoted 4 lines · Jan Kara
Quoted 2 lines · Christoph Hellwig

Still no fan of the naming and placement here. This is specific to the fs/buffer.c infrastructure.

Quoted 8 lines

I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and creating the new function there if it makes you happier. But I think function names should be consistent (hence the new function would be named __generic_file_fsync_nolock()). I agree the name is not ideal and would use cleanup (along with transitioning everybody to not take i_rwsem) but I don't want to complicate this series by touching 13+ callsites of generic_file_fsync() and __generic_file_fsync(). That's for a separate series.

I would not change the existing function. Just do the right thing for the new helper and slowly migrate over without complicating this series.

Quoted 8 lines
Quoted 2 lines

OK, I can live with that temporary naming inconsistency I guess. So the function will be __buffer_file_fsync()?

This name was suggested before, so if that's ok I will go with this - "generic_buffer_fsync()". It's definition will lie in fs/buffer.c and it's declaration in include/linux/buffer_head.h

Is that ok?

Yes, that is fine by me. And I suppose this variant will also issue the cache flush, won't it? But then we also need __generic_buffer_fsync() without issuing the cache flush for ext4 (we need to sync parent before issuing a cache flush) and FAT.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author reviewer
#9 lore raw

Re: [RFCv3 02/10] libfs: Add __generic_file_fsync_nolock implementation

Quoted 24 lines · Jan Kara
Quoted 23 lines · Ritesh Harjani
Quoted 20 lines · Jan Kara
Quoted 14 lines · Christoph Hellwig
Quoted 4 lines · Jan Kara
Quoted 2 lines · Christoph Hellwig

Still no fan of the naming and placement here. This is specific to the fs/buffer.c infrastructure.

Quoted 8 lines

I'm fine with moving generic_file_fsync() & friends to fs/buffer.c and creating the new function there if it makes you happier. But I think function names should be consistent (hence the new function would be named __generic_file_fsync_nolock()). I agree the name is not ideal and would use cleanup (along with transitioning everybody to not take i_rwsem) but I don't want to complicate this series by touching 13+ callsites of generic_file_fsync() and __generic_file_fsync(). That's for a separate series.

Quoted 2 lines

I would not change the existing function. Just do the right thing for the new helper and slowly migrate over without complicating this series.

OK, I can live with that temporary naming inconsistency I guess. So the function will be __buffer_file_fsync()?

Quoted 8 lines
Quoted 5 lines

This name was suggested before, so if that's ok I will go with this - "generic_buffer_fsync()". It's definition will lie in fs/buffer.c and it's declaration in include/linux/buffer_head.h

Is that ok?

Yes, that is fine by me. And I suppose this variant will also issue the cache flush, won't it?

No. We don't issue cache flush (REQ_PREFLUSH) in generic_buffer_fsync(), neither __generic_file_fsync() does that.

Quoted 3 lines

But then we also need __generic_buffer_fsync()
without issuing the cache flush for ext4 (we need to sync parent before issuing a cache flush) and FAT.

Yes, we do take care of that by -

<simplified logic>
ret = generic_buffer_fsync()
if (!ret)
ret = ext4_sync_parent(inode)
if (test_opt(inode->i_sb, BARRIER))
blkdev_issue_flush()

Am I missing anything. I have sent a [v5] with all of the comments addressed. Could you please take a look and let me know if it looks good or is there anything required?

[v5]: https://lore.kernel.org/linux-ext4/[email protected]/T/#t

-ritesh

author reviewer
#10 lore raw

RFC v3 3/10 ext4: Use __generic_file_fsync_nolock 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 it's _nolock variant. Hence kill the redundant code and use __generic_file_fsync_nolock() function instead.

Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/ext4/fsync.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
fs/ext4/fsync.c +15 -16
index 027a7d7037a0..bf1d1b7f4ec7 100644
@@ -78,21 +78,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_file_fsync_nolock(file, start, end, datasync);
 	if (!ret)
 		ret = ext4_sync_parent(inode);
 	if (test_opt(inode->i_sb, BARRIER))

@@ -148,6 +140,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;

@@ -166,13 +166,12 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	 *  (they were dirtied by commit).  But that's OK - the blocks are
 	 *  safe in-journal, which is all fsync() needs to ensure.
 	 */
-	if (!sbi->s_journal)
-		ret = ext4_fsync_nojournal(inode, datasync, &needs_barrier);
-	else if (ext4_should_journal_data(inode))
+	if (ext4_should_journal_data(inode))
 		ret = ext4_force_commit(inode->i_sb);
 	else
 		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
author reviewer
#11 lore raw

RFC v3 4/10 ext2: Use __generic_file_fsync_nolock 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_file_fsync_nolock() variant in ext2_fsync().

Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/ext2/file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/ext2/file.c +3 -1
index 6b4bebe982ca..1d0bc3fc88bb 100644
@@ -153,7 +153,9 @@ 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_file_fsync_nolock(file, start, end, datasync);
+	if (!ret)
+		ret = blkdev_issue_flush(sb->s_bdev);
 	if (ret == -EIO)
 		/* We don't really know where the IO error happened... */
 		ext2_error(sb, __func__,
Signature · 1 line
2.39.2
author reviewer
#12 lore raw

RFC v3 5/10 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]>
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
author reviewer
#13 lore raw

RFC v3 6/10 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 9ca3813f43e2..6903fc15987a 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, "EVENTD"}, \
+	{ 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
maintainer
#14 lore raw

Re: [RFCv3 06/10] fs.h: Add TRACE_IOCB_STRINGS for use in trace points

Quoted 7 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]>

---

Fine, but fs.h is such a dumping ground already I hope we can split more stuff out of it going forward...

Quoted 9 lines
 include/linux/fs.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
include/linux/fs.h +0 -0
index 9ca3813f43e2..6903fc15987a 100644
@@ -340,6 +340,20 @@ enum rw_hint {
 /* can use bio alloc cache */

include/linux/fs.h enum rw_hint { · line 350

Quoted 10 lines
include/linux/fs.h +8 -0
 #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, "EVENTD"}, \

s/EVENTD/EVENTFD/

author reviewer
#15 lore raw

Re: [RFCv3 06/10] fs.h: Add TRACE_IOCB_STRINGS for use in trace points

Christian Brauner <[email protected]> writes:

Hi Christian

Thanks for your review!

Quoted 10 lines · Ritesh Harjani (IBM)
Quoted 7 lines

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]>

---

Fine, but fs.h is such a dumping ground already

Ok, 3205 lines in fs.h.

Quoted 1 line

I hope we can split more stuff out of it going forward...

Any first thoughts/suggestions like what?

Quoted 11 lines
Quoted 11 lines
 include/linux/fs.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
include/linux/fs.h +0 -0
index 9ca3813f43e2..6903fc15987a 100644
@@ -340,6 +340,20 @@ enum rw_hint {
 /* can use bio alloc cache */
 #define IOCB_ALLOC_CACHE	(1 << 21)
Quoted 10 lines

include/linux/fs.h enum rw_hint { · line 350

Quoted 8 lines
include/linux/fs.h +8 -0
+/* 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, "EVENTD"}, \

s/EVENTD/EVENTFD/

Oops an oversight. Thanks for catching it.

-ritesh

author reviewer
#16 lore raw

RFC v3 7/10 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

         xfs_io-4099  [004]    81.431800: ext2_dio_write_iter_start: dev 7:7 ino 0xc isize 0x4000 pos 0x4000 count 4096 flags DIRECT aio 0 ret=0
         xfs_io-4099  [004]    81.432002: ext2_dio_write_end_io: dev 7:7 ino 0xc isize 0x5000 pos 0x4000 count 4096 flags DIRECT aio 0 ret=0
         xfs_io-4099  [004]    81.432014: ext2_dio_write_iter_dio_end: dev 7:7 ino 0xc isize 0x5000 pos 0x5000 count 0 flags DIRECT aio 0 ret=4096
aio-dio-fcntl-r-4103  [005]    81.461123: ext2_dio_write_iter_start: dev 7:7 ino 0xc isize 0x400 pos 0x200 count 512 flags DIRECT|WRITE aio 1 ret=0
aio-dio-fcntl-r-4103  [005]    81.462099: ext2_dio_write_end_io: dev 7:7 ino 0xc isize 0x400 pos 0x200 count 512 flags DIRECT|WRITE aio 1 ret=0
aio-dio-fcntl-r-4103  [005]    81.462133: ext2_dio_write_iter_dio_end: dev 7:7 ino 0xc isize 0x400 pos 0x400 count 0 flags DIRECT|WRITE aio 1 ret=512
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 243395616599..902eaabc5c62 100644
@@ -28,6 +28,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)

@@ -169,9 +170,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;
 }

@@ -199,6 +202,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;
 }
 

@@ -215,7 +219,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)

@@ -243,7 +249,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;

@@ -269,6 +274,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
#17 lore raw

Re: [RFCv3 07/10] ext2: Add direct-io trace points

So why do you add tracepoints in ext2 in addition to iomap?

author reviewer
#18 lore raw

Re: [RFCv3 07/10] ext2: Add direct-io trace points

Quoted 1 line · Christoph Hellwig

So why do you add tracepoints in ext2 in addition to iomap?

1. It's very convenient to look into all ext2 filesystem activity by just "trace-cmd -e ext2"

2. For cases in ext2 which fallbacks to buffered-io routine. Adding a trace point specifically for ext2 can cover this path as well as to how much got written via buffered-io path or in case there was an error while doing that.

3. As of now there was no tracepoint added to ext2. Going forward it will be quick and convenient to add any trace event for debugging/observing other call paths too while development.

-ritesh

author reviewer
#19 lore raw

RFC v3 8/10 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]>
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
Reviewed-by on v3 8/10
#20 lore raw

Re: [RFCv3 08/10] iomap: Remove IOMAP_DIO_NOSYNC unused dio flag

Quoted 11 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.

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]>
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
author reviewer
#21 lore raw

RFC v3 9/10 iomap: Minor refactor of iomap_dio_rw

In reply to Ritesh Harjani (IBM)

The next patch brings in the tracepoint patch for iomap DIO functions. This is a small refactor change for having a single out path.

Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/iomap/direct-io.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
fs/iomap/direct-io.c +8 -3
index 36ab1152dbea..5871956ee880 100644
@@ -679,11 +679,16 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		unsigned int dio_flags, void *private, size_t done_before)
 {
 	struct iomap_dio *dio;
+	ssize_t ret = 0;
 
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
-	if (IS_ERR_OR_NULL(dio))
-		return PTR_ERR_OR_ZERO(dio);
-	return iomap_dio_complete(dio);
+	if (IS_ERR_OR_NULL(dio)) {
+		ret = PTR_ERR_OR_ZERO(dio);
+		goto out;
+	}
+	ret = iomap_dio_complete(dio);
+out:
+	return ret;
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
Signature · 1 line
2.39.2
Reviewed-by on v3 9/10
#22 lore raw

Re: [RFCv3 09/10] iomap: Minor refactor of iomap_dio_rw

Quoted 5 lines · Ritesh Harjani (IBM)

The next patch brings in the tracepoint patch for iomap DIO functions. This is a small refactor change for having a single out path.

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

IMHO this could've been part of the next patch instead of separate, but eh, whatever, looks good to me.

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

--D

Quoted 31 lines
---
 fs/iomap/direct-io.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
fs/iomap/direct-io.c +8 -3
index 36ab1152dbea..5871956ee880 100644
@@ -679,11 +679,16 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		unsigned int dio_flags, void *private, size_t done_before)
 {
 	struct iomap_dio *dio;
+	ssize_t ret = 0;
 
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
-	if (IS_ERR_OR_NULL(dio))
-		return PTR_ERR_OR_ZERO(dio);
-	return iomap_dio_complete(dio);
+	if (IS_ERR_OR_NULL(dio)) {
+		ret = PTR_ERR_OR_ZERO(dio);
+		goto out;
+	}
+	ret = iomap_dio_complete(dio);
+out:
+	return ret;
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
Signature · 1 line
2.39.2
#23 lore raw

Re: [RFCv3 09/10] iomap: Minor refactor of iomap_dio_rw

Quoted 9 lines · Darrick J. Wong
Quoted 5 lines · Ritesh Harjani (IBM)

The next patch brings in the tracepoint patch for iomap DIO functions. This is a small refactor change for having a single out path.

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

IMHO this could've been part of the next patch instead of separate, but eh, whatever, looks good to me.

Yeah, without the next patch this is a bit pointless. And also a harder to review without seeing why it is done.

author reviewer
#24 lore raw

RFC v3 10/10 iomap: Add trace points for DIO path

In reply to Ritesh Harjani (IBM)

This patch adds trace point events for iomap DIO path.

<e.g. iomap dio trace>

     xfs_io-8815  [000]   526.790418: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x0 pos 0x0 count 4096 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 0
     xfs_io-8815  [000]   526.790978: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x1000 pos 0x1000 flags DIRECT aio 0 error 0 ret 4096
     xfs_io-8815  [000]   526.790988: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x1000 pos 0x1000 count 0 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 4096
        fsx-8827  [005]   526.939345: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 61440 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret 0
        fsx-8827  [005]   526.939459: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 0 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret -529
ksoftirqd/5-41    [005]   526.939564: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x922f8 pos 0x5e000 flags NOWAIT|DIRECT|ALLOC_CACHE aio 1 error 0 ret 61440
Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/iomap/direct-io.c |  3 ++
 fs/iomap/trace.c     |  1 +
 fs/iomap/trace.h     | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
fs/iomap/direct-io.c +3 -0
index 5871956ee880..bb7a6dfbc8b3 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;

@@ -681,6 +682,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct iomap_dio *dio;
 	ssize_t ret = 0;
 
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
 	if (IS_ERR_OR_NULL(dio)) {

@@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	ret = iomap_dio_complete(dio);
 out:
+	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
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 +90 -0

96 lines not on this page · raw message

Signature · 1 line
2.39.2
#25 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

Quoted 63 lines · Ritesh Harjani (IBM)

This patch adds trace point events for iomap DIO path.

<e.g. iomap dio trace>

     xfs_io-8815  [000]   526.790418: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x0 pos 0x0 count 4096 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 0
     xfs_io-8815  [000]   526.790978: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x1000 pos 0x1000 flags DIRECT aio 0 error 0 ret 4096
     xfs_io-8815  [000]   526.790988: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x1000 pos 0x1000 count 0 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 4096
        fsx-8827  [005]   526.939345: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 61440 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret 0
        fsx-8827  [005]   526.939459: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 0 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret -529
ksoftirqd/5-41    [005]   526.939564: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x922f8 pos 0x5e000 flags NOWAIT|DIRECT|ALLOC_CACHE aio 1 error 0 ret 61440
Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/iomap/direct-io.c |  3 ++
 fs/iomap/trace.c     |  1 +
 fs/iomap/trace.h     | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
fs/iomap/direct-io.c +3 -0
index 5871956ee880..bb7a6dfbc8b3 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;

@@ -681,6 +682,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct iomap_dio *dio;
 	ssize_t ret = 0;
 
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
 	if (IS_ERR_OR_NULL(dio)) {

@@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	ret = iomap_dio_complete(dio);
 out:
+	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
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 +0 -0
index f6ea9540d082..dcb4dd4db5fb 100644

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 189

Quoted 7 lines
fs/iomap/trace.h +4 -0
@@ -183,6 +183,96 @@ TRACE_EVENT(iomap_iter,
 		   (void *)__entry->caller)
 );
 
+#define TRACE_IOMAP_DIO_STRINGS \
+	{IOMAP_DIO_FORCE_WAIT, "DIO_FORCE_WAIT" }, \
+	{IOMAP_DIO_OVERWRITE_ONLY, "DIO_OVERWRITE_ONLY" }, \
+	{IOMAP_DIO_PARTIAL, "DIO_PARTIAL" }

Can you make the strings line up too, please?

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 193

Quoted 4 lines
fs/iomap/trace.h +4 -0
+
+DECLARE_EVENT_CLASS(iomap_dio_class,
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,
+		 unsigned int dio_flags, u64 done_before, int ret),

We're passing in ssize_t values for @ret, shouldn't the types match?

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 200

Quoted 7 lines
fs/iomap/trace.h +7 -0
+	TP_ARGS(iocb, iter, dio_flags, done_before, ret),
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(ino_t,	ino)
+		__field(loff_t, isize)
+		__field(loff_t, pos)
+		__field(u64,	count)

What's the difference between "length" as used in the other tracepoints and "count" here?

Quoted 9 lines
fs/iomap/trace.h +9 -0
+		__field(u64,	done_before)
+		__field(int,	ki_flags)
+		__field(unsigned int,	dio_flags)
+		__field(bool,	aio)
+		__field(int, ret)
+	),
+	TP_fast_assign(
+		__entry->dev = file_inode(iocb->ki_filp)->i_sb->s_dev;
+		__entry->ino = file_inode(iocb->ki_filp)->i_ino;

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 219

Quoted 10 lines
fs/iomap/trace.h +10 -0
+		__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->dio_flags = dio_flags;
+		__entry->ki_flags = iocb->ki_flags;
+		__entry->aio = !is_sync_kiocb(iocb);
+		__entry->ret = ret;
+	),
+	TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %llu "

count and done_before are lengths of file operations, in bytes, right?

Everywhere else we use 0x%llx for that.

Quoted 24 lines
fs/iomap/trace.h +24 -0
+		  "flags %s dio_flags %s done_before %llu aio %d ret %d",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->ino,
+		  __entry->isize,
+		  __entry->pos,
+		  __entry->count,
+		  __print_flags(__entry->ki_flags, "|", TRACE_IOCB_STRINGS),
+		  __print_flags(__entry->dio_flags, "|", TRACE_IOMAP_DIO_STRINGS),
+		  __entry->done_before,
+		  __entry->aio,
+		  __entry->ret)
+)
+
+#define DEFINE_DIO_RW_EVENT(name)					\
+DEFINE_EVENT(iomap_dio_class, name,					\
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,		\
+		 unsigned int dio_flags, u64 done_before,		\
+		 int ret),						\
+	TP_ARGS(iocb, iter, dio_flags, done_before, ret))
+DEFINE_DIO_RW_EVENT(iomap_dio_rw_begin);
+DEFINE_DIO_RW_EVENT(iomap_dio_rw_end);
+
+TRACE_EVENT(iomap_dio_complete,
+	TP_PROTO(struct kiocb *iocb, int error, int ret),

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 253

Quoted 10 lines
fs/iomap/trace.h +10 -0
+	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(int,	ret)

Same comment about @ret and ssize_t here.

--D

Quoted 28 lines
fs/iomap/trace.h +22 -0
+	),
+	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 isize 0x%llx pos 0x%llx flags %s aio %d error %d ret %d",
+		  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
author reviewer
#26 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

Quoted 67 lines · Darrick J. Wong
Quoted 66 lines · Ritesh Harjani (IBM)

This patch adds trace point events for iomap DIO path.

<e.g. iomap dio trace>

     xfs_io-8815  [000]   526.790418: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x0 pos 0x0 count 4096 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 0
     xfs_io-8815  [000]   526.790978: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x1000 pos 0x1000 flags DIRECT aio 0 error 0 ret 4096
     xfs_io-8815  [000]   526.790988: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x1000 pos 0x1000 count 0 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 4096
        fsx-8827  [005]   526.939345: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 61440 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret 0
        fsx-8827  [005]   526.939459: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 0 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret -529
ksoftirqd/5-41    [005]   526.939564: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x922f8 pos 0x5e000 flags NOWAIT|DIRECT|ALLOC_CACHE aio 1 error 0 ret 61440
Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/iomap/direct-io.c |  3 ++
 fs/iomap/trace.c     |  1 +
 fs/iomap/trace.h     | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
fs/iomap/direct-io.c +3 -0
index 5871956ee880..bb7a6dfbc8b3 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;

@@ -681,6 +682,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct iomap_dio *dio;
 	ssize_t ret = 0;
 
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
 	if (IS_ERR_OR_NULL(dio)) {

@@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	ret = iomap_dio_complete(dio);
 out:
+	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
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 +0 -0
index f6ea9540d082..dcb4dd4db5fb 100644
@@ -183,6 +183,96 @@ TRACE_EVENT(iomap_iter,
 		   (void *)__entry->caller)
 );
Quoted 7 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 189

Quoted 4 lines
fs/iomap/trace.h +4 -0
+#define TRACE_IOMAP_DIO_STRINGS \
+	{IOMAP_DIO_FORCE_WAIT, "DIO_FORCE_WAIT" }, \
+	{IOMAP_DIO_OVERWRITE_ONLY, "DIO_OVERWRITE_ONLY" }, \
+	{IOMAP_DIO_PARTIAL, "DIO_PARTIAL" }

Can you make the strings line up too, please?

Ok near other _STRINGS macro. Sure, will do that.

Quoted 7 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 193

Quoted 4 lines
fs/iomap/trace.h +4 -0
+
+DECLARE_EVENT_CLASS(iomap_dio_class,
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,
+		 unsigned int dio_flags, u64 done_before, int ret),

We're passing in ssize_t values for @ret, shouldn't the types match?

Yes, I missed to correct that. Will make it loff_t. This should be fixed in ext2 trace point macro too.

(ssize_t can vary based on 32 bit v/s 64 bit, so while printing it as %llx it gives warning on 32bit. Hence will use loff_t for ret)

Quoted 11 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 200

Quoted 7 lines
fs/iomap/trace.h +7 -0
+	TP_ARGS(iocb, iter, dio_flags, done_before, ret),
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(ino_t,	ino)
+		__field(loff_t, isize)
+		__field(loff_t, pos)
+		__field(u64,	count)

What's the difference between "length" as used in the other tracepoints and "count" here?

Yup let me make it length which will be a more consistent naming. I chose count just because of (iov_iter_count(iter)).

Quoted 13 lines
Quoted 13 lines
fs/iomap/trace.h +13 -0
+		__field(u64,	done_before)
+		__field(int,	ki_flags)
+		__field(unsigned int,	dio_flags)
+		__field(bool,	aio)
+		__field(int, 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->count = iov_iter_count(iter);
+		__entry->done_before = done_before;
Quoted 8 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 219

Quoted 6 lines
fs/iomap/trace.h +6 -0
+		__entry->dio_flags = dio_flags;
+		__entry->ki_flags = iocb->ki_flags;
+		__entry->aio = !is_sync_kiocb(iocb);
+		__entry->ret = ret;
+	),
+	TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %llu "

count and done_before are lengths of file operations, in bytes, right?

Yes, that's right.

Quoted 3 lines

Everywhere else we use 0x%llx for that.

Yup I had noticed that, but I guess I missed it.
Thanks for catching it. I will fix it.

Quoted 28 lines
Quoted 28 lines
fs/iomap/trace.h +28 -0
+		  "flags %s dio_flags %s done_before %llu aio %d ret %d",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->ino,
+		  __entry->isize,
+		  __entry->pos,
+		  __entry->count,
+		  __print_flags(__entry->ki_flags, "|", TRACE_IOCB_STRINGS),
+		  __print_flags(__entry->dio_flags, "|", TRACE_IOMAP_DIO_STRINGS),
+		  __entry->done_before,
+		  __entry->aio,
+		  __entry->ret)
+)
+
+#define DEFINE_DIO_RW_EVENT(name)					\
+DEFINE_EVENT(iomap_dio_class, name,					\
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,		\
+		 unsigned int dio_flags, u64 done_before,		\
+		 int ret),						\
+	TP_ARGS(iocb, iter, dio_flags, done_before, ret))
+DEFINE_DIO_RW_EVENT(iomap_dio_rw_begin);
+DEFINE_DIO_RW_EVENT(iomap_dio_rw_end);
+
+TRACE_EVENT(iomap_dio_complete,
+	TP_PROTO(struct kiocb *iocb, int error, int ret),
+	TP_ARGS(iocb, error, ret),
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(ino_t,	ino)
Quoted 8 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 253

Quoted 6 lines
fs/iomap/trace.h +6 -0
+		__field(loff_t, isize)
+		__field(loff_t, pos)
+		__field(int,	ki_flags)
+		__field(bool,	aio)
+		__field(int,	error)
+		__field(int,	ret)

Same comment about @ret and ssize_t here.

Got it.

Thanks for the review!
-ritesh

Quoted 31 lines

--D

Quoted 28 lines
fs/iomap/trace.h +22 -1
+	),
+	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 isize 0x%llx pos 0x%llx flags %s aio %d error %d ret %d",
+		  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
--

2.39.2

#27 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

Quoted 88 lines · Ritesh Harjani
Quoted 67 lines · Darrick J. Wong
Quoted 66 lines · Ritesh Harjani (IBM)

This patch adds trace point events for iomap DIO path.

<e.g. iomap dio trace>

     xfs_io-8815  [000]   526.790418: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x0 pos 0x0 count 4096 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 0
     xfs_io-8815  [000]   526.790978: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x1000 pos 0x1000 flags DIRECT aio 0 error 0 ret 4096
     xfs_io-8815  [000]   526.790988: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x1000 pos 0x1000 count 0 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 4096
        fsx-8827  [005]   526.939345: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 61440 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret 0
        fsx-8827  [005]   526.939459: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 0 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret -529
ksoftirqd/5-41    [005]   526.939564: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x922f8 pos 0x5e000 flags NOWAIT|DIRECT|ALLOC_CACHE aio 1 error 0 ret 61440
Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/iomap/direct-io.c |  3 ++
 fs/iomap/trace.c     |  1 +
 fs/iomap/trace.h     | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
fs/iomap/direct-io.c +3 -0
index 5871956ee880..bb7a6dfbc8b3 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;

@@ -681,6 +682,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct iomap_dio *dio;
 	ssize_t ret = 0;
 
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
 	if (IS_ERR_OR_NULL(dio)) {

@@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	ret = iomap_dio_complete(dio);
 out:
+	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
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 +0 -0
index f6ea9540d082..dcb4dd4db5fb 100644
@@ -183,6 +183,96 @@ TRACE_EVENT(iomap_iter,
 		   (void *)__entry->caller)
 );
Quoted 7 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 189

Quoted 4 lines
fs/iomap/trace.h +4 -0
+#define TRACE_IOMAP_DIO_STRINGS \
+	{IOMAP_DIO_FORCE_WAIT, "DIO_FORCE_WAIT" }, \
+	{IOMAP_DIO_OVERWRITE_ONLY, "DIO_OVERWRITE_ONLY" }, \
+	{IOMAP_DIO_PARTIAL, "DIO_PARTIAL" }

Can you make the strings line up too, please?

Ok near other _STRINGS macro. Sure, will do that.

Quoted 7 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 193

Quoted 4 lines
fs/iomap/trace.h +4 -0
+
+DECLARE_EVENT_CLASS(iomap_dio_class,
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,
+		 unsigned int dio_flags, u64 done_before, int ret),

We're passing in ssize_t values for @ret, shouldn't the types match?

Quoted 5 lines

Yes, I missed to correct that. Will make it loff_t. This should be fixed in ext2 trace point macro too.

(ssize_t can vary based on 32 bit v/s 64 bit, so while printing it as %llx it gives warning on 32bit. Hence will use loff_t for ret)

How about %zd?

--D

Quoted 121 lines
Quoted 12 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 200

Quoted 8 lines
fs/iomap/trace.h +7 -0
+	TP_ARGS(iocb, iter, dio_flags, done_before, ret),
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(ino_t,	ino)
+		__field(loff_t, isize)
+		__field(loff_t, pos)
+		__field(u64,	count)

What's the difference between "length" as used in the other tracepoints and "count" here?

Yup let me make it length which will be a more consistent naming. I chose count just because of (iov_iter_count(iter)).

Quoted 13 lines
Quoted 13 lines
fs/iomap/trace.h +13 -0
+		__field(u64,	done_before)
+		__field(int,	ki_flags)
+		__field(unsigned int,	dio_flags)
+		__field(bool,	aio)
+		__field(int, 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->count = iov_iter_count(iter);
+		__entry->done_before = done_before;
Quoted 8 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 219

Quoted 6 lines
fs/iomap/trace.h +6 -0
+		__entry->dio_flags = dio_flags;
+		__entry->ki_flags = iocb->ki_flags;
+		__entry->aio = !is_sync_kiocb(iocb);
+		__entry->ret = ret;
+	),
+	TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %llu "

count and done_before are lengths of file operations, in bytes, right?

Yes, that's right.

Quoted 3 lines

Everywhere else we use 0x%llx for that.

Yup I had noticed that, but I guess I missed it.
Thanks for catching it. I will fix it.

Quoted 28 lines
Quoted 28 lines
fs/iomap/trace.h +28 -0
+		  "flags %s dio_flags %s done_before %llu aio %d ret %d",
+		  MAJOR(__entry->dev), MINOR(__entry->dev),
+		  __entry->ino,
+		  __entry->isize,
+		  __entry->pos,
+		  __entry->count,
+		  __print_flags(__entry->ki_flags, "|", TRACE_IOCB_STRINGS),
+		  __print_flags(__entry->dio_flags, "|", TRACE_IOMAP_DIO_STRINGS),
+		  __entry->done_before,
+		  __entry->aio,
+		  __entry->ret)
+)
+
+#define DEFINE_DIO_RW_EVENT(name)					\
+DEFINE_EVENT(iomap_dio_class, name,					\
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,		\
+		 unsigned int dio_flags, u64 done_before,		\
+		 int ret),						\
+	TP_ARGS(iocb, iter, dio_flags, done_before, ret))
+DEFINE_DIO_RW_EVENT(iomap_dio_rw_begin);
+DEFINE_DIO_RW_EVENT(iomap_dio_rw_end);
+
+TRACE_EVENT(iomap_dio_complete,
+	TP_PROTO(struct kiocb *iocb, int error, int ret),
+	TP_ARGS(iocb, error, ret),
+	TP_STRUCT__entry(
+		__field(dev_t,	dev)
+		__field(ino_t,	ino)
Quoted 8 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 253

Quoted 6 lines
fs/iomap/trace.h +6 -0
+		__field(loff_t, isize)
+		__field(loff_t, pos)
+		__field(int,	ki_flags)
+		__field(bool,	aio)
+		__field(int,	error)
+		__field(int,	ret)

Same comment about @ret and ssize_t here.

Got it.

Thanks for the review!
-ritesh

Quoted 31 lines

--D

Quoted 28 lines
fs/iomap/trace.h +22 -1
+	),
+	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 isize 0x%llx pos 0x%llx flags %s aio %d error %d ret %d",
+		  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
--

2.39.2

author reviewer
#28 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

Quoted 89 lines · Darrick J. Wong
Quoted 88 lines · Ritesh Harjani
Quoted 67 lines · Darrick J. Wong
Quoted 66 lines · Ritesh Harjani (IBM)

This patch adds trace point events for iomap DIO path.

<e.g. iomap dio trace>

     xfs_io-8815  [000]   526.790418: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x0 pos 0x0 count 4096 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 0
     xfs_io-8815  [000]   526.790978: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x1000 pos 0x1000 flags DIRECT aio 0 error 0 ret 4096
     xfs_io-8815  [000]   526.790988: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x1000 pos 0x1000 count 0 flags DIRECT dio_flags DIO_FORCE_WAIT done_before 0 aio 0 ret 4096
        fsx-8827  [005]   526.939345: iomap_dio_rw_begin:   dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 61440 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret 0
        fsx-8827  [005]   526.939459: iomap_dio_rw_end:     dev 7:7 ino 0xc isize 0x922f8 pos 0x4f000 count 0 flags NOWAIT|DIRECT|ALLOC_CACHE dio_flags  done_before 0 aio 1 ret -529
ksoftirqd/5-41    [005]   526.939564: iomap_dio_complete:   dev 7:7 ino 0xc isize 0x922f8 pos 0x5e000 flags NOWAIT|DIRECT|ALLOC_CACHE aio 1 error 0 ret 61440
Tested-by Disha Goel <[email protected]>
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
---
 fs/iomap/direct-io.c |  3 ++
 fs/iomap/trace.c     |  1 +
 fs/iomap/trace.h     | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
fs/iomap/direct-io.c +3 -0
index 5871956ee880..bb7a6dfbc8b3 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;

@@ -681,6 +682,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	struct iomap_dio *dio;
 	ssize_t ret = 0;
 
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
 	if (IS_ERR_OR_NULL(dio)) {

@@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	ret = iomap_dio_complete(dio);
 out:
+	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iomap_dio_rw);
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 +0 -0
index f6ea9540d082..dcb4dd4db5fb 100644
@@ -183,6 +183,96 @@ TRACE_EVENT(iomap_iter,
 		   (void *)__entry->caller)
 );
Quoted 7 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 189

Quoted 4 lines
fs/iomap/trace.h +4 -0
+#define TRACE_IOMAP_DIO_STRINGS \
+	{IOMAP_DIO_FORCE_WAIT, "DIO_FORCE_WAIT" }, \
+	{IOMAP_DIO_OVERWRITE_ONLY, "DIO_OVERWRITE_ONLY" }, \
+	{IOMAP_DIO_PARTIAL, "DIO_PARTIAL" }

Can you make the strings line up too, please?

Ok near other _STRINGS macro. Sure, will do that.

Quoted 7 lines

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 193

Quoted 4 lines
fs/iomap/trace.h +4 -0
+
+DECLARE_EVENT_CLASS(iomap_dio_class,
+	TP_PROTO(struct kiocb *iocb, struct iov_iter *iter,
+		 unsigned int dio_flags, u64 done_before, int ret),

We're passing in ssize_t values for @ret, shouldn't the types match?

Quoted 7 lines
Quoted 5 lines

Yes, I missed to correct that. Will make it loff_t. This should be fixed in ext2 trace point macro too.

(ssize_t can vary based on 32 bit v/s 64 bit, so while printing it as %llx it gives warning on 32bit. Hence will use loff_t for ret)

How about %zd?

Aah yes. My bad, I wanted to look into print-format specifiers, but missed it.

Documentation/core-api/printk-formats.rst

		size_t			%zu or %zx
		ssize_t			%zd or %zx

Will send the next revision soon with the comments addressed then.

Thanks!
-ritesh

#29 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

In reply to Ritesh Harjani (IBM)

fs/iomap/direct-io.c iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, · line 694

Quoted 9 lines
fs/iomap/direct-io.c +2 -0
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
 	if (IS_ERR_OR_NULL(dio)) {

@@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	ret = iomap_dio_complete(dio);
 out:
+	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);

The trace_iomap_dio_rw_end tracepoint heere seems a bit weird, and we'll miss it for file systems using __iomap_dio_rw directly.

I'd instead add a trace_iomap_dio_rw_queued for the case where __iomap_dio_rw returns ERR_PTR(-EIOCBQUEUED), as otherwise we're nicely covered by the complete trace points.

fs/iomap/trace.h TRACE_EVENT(iomap_iter, · line 227

Quoted 1 line
fs/iomap/trace.h +1 -0
+		  __print_flags(__entry->dio_flags, "|", TRACE_IOMAP_DIO_STRINGS),

Please avoid the overly lone line here.

author reviewer
#30 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

Quoted 4 lines · Christoph Hellwig
Quoted 4 lines
(quoted diff) +1 -0
+	trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
 	dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
 			     done_before);
 	if (IS_ERR_OR_NULL(dio)) {
Quoted 8 lines

Review iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, · line 694

Quoted 5 lines
(quoted diff) +1 -0
@@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	ret = iomap_dio_complete(dio);
 out:
+	trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);

The trace_iomap_dio_rw_end tracepoint heere seems a bit weird, and we'll miss it for file systems using __iomap_dio_rw directly.

Sorry, yes you are right.

Quoted 5 lines

I'd instead add a trace_iomap_dio_rw_queued for the case where __iomap_dio_rw returns ERR_PTR(-EIOCBQUEUED), as otherwise we're nicely covered by the complete trace points.

How about this below change? Does this look good to you? It should cover all error types and both entry and exit.

And should I fold this entire change in 1 patch or should I split the refactoring of common out routine into a seperate one?

fs/iomap/direct-io.c +15 -5
index 5871956ee880..e412fdc4ee86 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,12 +494,15 @@ __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, ret);
        if (!iomi.len)
-               return NULL;
+               goto out;
 
        dio = kmalloc(sizeof(*dio), GFP_KERNEL);
-       if (!dio)
-               return ERR_PTR(-ENOMEM);
+       if (!dio) {
+               ret = -ENOMEM;
+               goto out;
+       }
 
        dio->iocb = iocb;
        atomic_set(&dio->ref, 1);

@@ -650,8 +654,11 @@ __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)
-                       return ERR_PTR(-EIOCBQUEUED);
+               if (!wait_for_completion) {
+                       ret = -EIOCBQUEUED;
+                       goto out;
+               }
+
 
                for (;;) {
                        set_current_state(TASK_UNINTERRUPTIBLE);

@@ -663,10 +670,13 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
                __set_current_state(TASK_RUNNING);
        }
 
+       trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
        return dio;
 
 out_free_dio:
        kfree(dio);
+out:
+       trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
        if (ret)
                return ERR_PTR(ret);
        return NULL;
Quoted 3 lines

Review iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, · line 695

Quoted 1 line
(quoted diff) +1 -0
+		  __print_flags(__entry->dio_flags, "|", TRACE_IOMAP_DIO_STRINGS),

Please avoid the overly lone line here.

Somehow my checkpatch never gave a warning about it.
I will check why was that. But yes, I have anyways made the name to IOMAP_DIO_STRINGS similar to other namings used in fs/iomap/trace.h

-ritesh

#31 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

Quoted 2 lines · Ritesh Harjani

How about this below change? Does this look good to you? It should cover all error types and both entry and exit.

I don't think it is very useful. The complete tracepoint is the end of the I/O. Having a separate end one doesn't make sense. That's why I suggested a queued one for the asynchronous case.

author reviewer
#32 lore raw

Re: [RFCv3 10/10] iomap: Add trace points for DIO path

Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Ritesh Harjani

How about this below change? Does this look good to you? It should cover all error types and both entry and exit.

I don't think it is very useful. The complete tracepoint is the end of the I/O. Having a separate end one doesn't make sense. That's why I suggested a queued one for the asynchronous case.

Ok, does this look good then?

fs/iomap/direct-io.c +7 -2
index 36ab1152dbea..859efb5de1bf 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;

@@ -650,8 +651,12 @@ __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)
-                       return ERR_PTR(-EIOCBQUEUED);
+               if (!wait_for_completion) {
+                       ret = -EIOCBQUEUED;
+                       trace_iomap_dio_rw_queued(iocb, iter, dio_flags,
+                                                 done_before, ret);
+                       return ERR_PTR(ret);
+               }
 
                for (;;) {
                        set_current_state(TASK_UNINTERRUPTIBLE);

-ritesh