ext2: DIO to use iomap
Subsystems (4)
Ritesh Harjani (IBM)
reviewerv2 cover ext2: DIO to use iomap
Please find the series which moves ext2 direct-io to use modern iomap interface.
Here are some more details -
1. Patch-1: Fixes a kernel bug_on problem with ext2 dax code (found during code review and testing).
2. Patch-2: Adds a __generic_file_fsync_nolock implementation as we had discussed.
3. Patch-3 & Patch-4: Moves ext4 nojournal and ext2 to use _nolock method.
4. Patch-5: This is the main patch which moves ext2 direct-io to use iomap. (more details can be found in the patch)
5. Patch-6: Kills IOMAP_DIO_NOSYNC flag as it is not in use by any filesystem.
6. Patch-7: adds IOCB_STRINGS macro for use in trace events for better trace output of iocb flags.
7. Patch-8: Add ext2 trace point for DIO.
Testing:
=========
This passes ext2 "auto" group testing for fstests. There were no new failures with this patches.
Ritesh Harjani (IBM) (8):
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
iomap: Remove IOMAP_DIO_NOSYNC unused dio flag fs.h: Add IOCB_STRINGS for use in trace points
ext2: Add direct-io trace points
fs/ext2/Makefile | 2 +- fs/ext2/ext2.h | 1 + fs/ext2/file.c | 127 +++++++++++++++++++++++++++++++++++++++++- fs/ext2/inode.c | 57 +++++++++++-------- fs/ext2/trace.c | 5 ++ fs/ext2/trace.h | 61 ++++++++++++++++++++ fs/ext4/fsync.c | 31 +++++------ fs/iomap/direct-io.c | 2 +- fs/libfs.c | 43 ++++++++++++++ include/linux/fs.h | 15 +++++ include/linux/iomap.h | 6 -- 11 files changed, 303 insertions(+), 47 deletions(-) create mode 100644 fs/ext2/trace.c create mode 100644 fs/ext2/trace.h
--
2.39.2
Ritesh Harjani (IBM)
reviewerRFC v2 1/8 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
--- fs/ext2/inode.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
fs/ext2/inode.c +2 -3
@@ -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)
reviewerRFC v2 2/8 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.
--- fs/libfs.c | 43 +++++++++++++++++++++++++++++++++++++++++++ include/linux/fs.h | 1 + 2 files changed, 44 insertions(+)
fs/libfs.c +43 -0
@@ -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, +int 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 +1 -0
@@ -2935,6 +2935,7 @@ 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); +extern int __generic_file_fsync_nolock (struct file *,loff_t ,loff_t ,int );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
Ritesh Harjani (IBM)
reviewerRFC v2 3/8 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.
--- fs/ext4/fsync.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)
fs/ext4/fsync.c +15 -16
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 4/8 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().
--- fs/ext2/file.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
fs/ext2/file.c +3 -1
@@ -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
Ritesh Harjani (IBM)
reviewerRFC v2 5/8 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 not being shared by DIO, DAX & fiemap path
--- fs/ext2/ext2.h | 1 + fs/ext2/file.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++ fs/ext2/inode.c | 52 ++++++++++++++-------- 3 files changed, 149 insertions(+), 19 deletions(-)
fs/ext2/ext2.h +1 -0
@@ -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 *); +extern 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 +33 -19
90 lines not on this page · raw message
Signature · 1 line
2.39.2
Ritesh Harjani (IBM)
reviewerRFC v2 6/8 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.
--- 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
@@ -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
@@ -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)
reviewerRFC v2 7/8 fs.h: Add IOCB_STRINGS for use in trace points
In reply to Ritesh Harjani (IBM)Add IOCB_STRINGS macro which can be used in the trace point patch to print different flag values with meaningful string output.
--- include/linux/fs.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
include/linux/fs.h +14 -0
@@ -340,6 +340,20 @@ enum rw_hint {/* can use bio alloc cache */ #define IOCB_ALLOC_CACHE (1 << 21) +/* for use in trace events */ +#define 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
Ritesh Harjani (IBM)
reviewerRFC v2 8/8 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--- fs/ext2/Makefile | 2 +- fs/ext2/file.c | 10 +++++++- fs/ext2/trace.c | 5 ++++ fs/ext2/trace.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 fs/ext2/trace.c create mode 100644 fs/ext2/trace.h
fs/ext2/Makefile +1 -1
@@ -6,7 +6,7 @@ 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 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
@@ -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_iter_start (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_iter_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); } +trace_ext2_dio_write_end_io (iocb,NULL , size);return 0 ; }
@@ -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_iter_start (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_iter_buff_end (iocb, from, status); +trace_ext2_dio_write_iter_dio_end (iocb, from, ret);return ret; }
fs/ext2/trace.c new file +5 -0
@@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 +#include "ext2.h" + +#define CREATE_TRACE_POINTS +#include "trace.h"
fs/ext2/trace.h new file +61 -0
61 lines not on this page · raw message
Signature · 1 line
2.39.2
Re: [RFCv2 2/8] libfs: Add __generic_file_fsync_nolock implementation
In reply to Ritesh Harjani (IBM)Quoted 3 lines · Ritesh Harjani (IBM)
+/**
+ *__generic_file_fsync_nolock- generic fsync implementation for simple
+ * filesystems with no inode lock
No reallz need for the __ prefix in the name.
Quoted 1 line
+extern int
__generic_file_fsync_nolock(struct file *, loff_t, loff_t, int);
No need for the extern. And at least I personally prefer to spell out the parameter names to make the prototype much more readable.
Re: [RFCv2 6/8] iomap: Remove IOMAP_DIO_NOSYNC unused dio flag
In reply to Ritesh Harjani (IBM)Looks good:
Re: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Ritesh Harjani (IBM)Quoted 2 lines · Ritesh Harjani (IBM)
This patch adds the trace point to ext2 direct-io apis in
fs/ext2/file.c
Wouldn't it make more sense to add this to iomap instead?
Re: [RFCv2 7/8] fs.h: Add IOCB_STRINGS for use in trace points
In reply to Ritesh Harjani (IBM)Looks good:
Re: [RFCv2 5/8] ext2: Move direct-io to use iomap
In reply to Ritesh Harjani (IBM)Quoted 1 line · Ritesh Harjani (IBM)
+extern void
ext2_write_failed(struct address_space *mapping, loff_t to);
No need for the extern.
Quoted 16 lines
fs/ext2/file.c +16
-0+/* 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 ; + pos = iocb->ki_pos; + status =generic_perform_write (iocb, from); +if (unlikely (status <0 )) { + ret = status; + goto out_unlock; + } + + iocb->ki_pos += status; + ret += status;
fs/ext2/file.c int ext2_fsync(struct file *file, loff_t start, loff_t end, int datasync) · line 268
Quoted 10 lines
fs/ext2/file.c +10
-0+ endbyte = pos + status -1 ; + ret2 =filemap_write_and_wait_range (inode->i_mapping, pos, + endbyte); +if (!ret2) +invalidate_mapping_pages (inode->i_mapping, + pos >>PAGE_SHIFT , + endbyte >>PAGE_SHIFT ); +if (ret >0 ) +generic_write_sync (iocb, ret); + }
Nit, but to me it would seem cleaner if all the fallback handling was moved into a separate helper function. Or in fact by not using generic_file_write_iter even for buffered I/O and at doing the pre-I/O checks and the final generic_write_sync in common code in ext2 for direct and buffered I/O.
fs/ext2/inode.c static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, · line 820
Quoted 7 lines
fs/ext2/inode.c +7
-0+/* +* For writes that could fill holes inside i_size on a +* DIO_SKIP_HOLES filesystem we forbid block creations: only +* overwrites are permitted. +*/ +if ((flags &IOMAP_DIRECT ) && (first_block << blkbits <i_size_read (inode))) + create =0 ;
No need for braes around the < operation, but I think you might need them around the shift.
Also an overly long line here.
fs/ext2/inode.c static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, · line 827
Quoted 1 line
fs/ext2/inode.c +1
-0+if ((flags &IOMAP_WRITE ) && (offset + length >i_size_read (inode)))
No need for the second set of inner braces here either.
kernel test robot
build botRe: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Ritesh Harjani (IBM)kernel test robot noticed the following build errors:
Hi Ritesh,
[This is a private test report for your RFC patch.] kernel test robot noticed the following build errors:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on xfs-linux/for-next jack-fs/for_next linus/master v6.3-rc6 next-20230406] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev patch link: https://lore.kernel.org/r/f9825fab612761bee205046ce6e6e4caf25642ee.1681188927.git.ritesh.list%40gmail.com patch subject: [RFCv2 8/8] ext2: Add direct-io trace points config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230411/[email protected]/config) compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 git checkout 4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash fs/ext2/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
In file included from include/trace/define_trace.h:102,
from fs/ext2/trace.h:61,
from fs/ext2/trace.c:5:
fs/ext2/./trace.h: In function 'trace_raw_output_ext2_dio_class':
>> fs/ext2/./trace.h:34:19: warning: format '%ld' expects argument of type 'long int', but argument 8 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
34 | TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %ld flags %s aio %d ret=%d",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/trace/trace_events.h:203:34: note: in definition of macro 'DECLARE_EVENT_CLASS'
203 | trace_event_printf(iter, print); \
| ^~~~~
fs/ext2/./trace.h:34:9: note: in expansion of macro 'TP_printk'
34 | TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %ld flags %s aio %d ret=%d",
| ^~~~~~~~~
In file included from include/trace/trace_events.h:237:
fs/ext2/./trace.h:34:72: note: format string is defined here
34 | TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %ld flags %s aio %d ret=%d",
| ~~^
| |
| long int
| %d
fs/ext2/./trace.h: In function 'trace_event_raw_event_ext2_dio_class':
>> fs/ext2/./trace.h:29:41: error: implicit declaration of function 'iov_iter_count' [-Werror=implicit-function-declaration]
29 | __entry->count = iter ? iov_iter_count(iter) : ret;
| ^~~~~~~~~~~~~~
include/trace/trace_events.h:402:11: note: in definition of macro 'DECLARE_EVENT_CLASS'
402 | { assign; } \
| ^~~~~~
fs/ext2/./trace.h:24:9: note: in expansion of macro 'TP_fast_assign'
24 | TP_fast_assign(
| ^~~~~~~~~~~~~~cc1: some warnings being treated as errors
vim +/iov_iter_count +29 fs/ext2/./trace.h
10
11 DECLARE_EVENT_CLASS(ext2_dio_class,
12 TP_PROTO(struct kiocb *iocb, struct iov_iter *iter, int ret),
13 TP_ARGS(iocb, iter, ret),
14 TP_STRUCT__entry(
15 __field(dev_t, dev)
16 __field(ino_t, ino)
17 __field(loff_t, isize)
18 __field(loff_t, pos)
19 __field(size_t, count)
20 __field(int, ki_flags)
21 __field(int, aio)
22 __field(int, ret)
23 ),
24 TP_fast_assign(
25 __entry->dev = file_inode(iocb->ki_filp)->i_sb->s_dev;
26 __entry->ino = file_inode(iocb->ki_filp)->i_ino;
27 __entry->isize = file_inode(iocb->ki_filp)->i_size;
28 __entry->pos = iocb->ki_pos;
> 29 __entry->count = iter ? iov_iter_count(iter) : ret;
30 __entry->ki_flags = iocb->ki_flags;
31 __entry->aio = !is_sync_kiocb(iocb);
32 __entry->ret = iter ? ret : 0;
33 ),
> 34 TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %ld flags %s aio %d ret=%d",
35 MAJOR(__entry->dev), MINOR(__entry->dev),
36 __entry->ino,
37 __entry->isize,
38 __entry->pos,
39 __entry->count,
40 __print_flags(__entry->ki_flags, "|", IOCB_STRINGS),
41 __entry->aio,
42 __entry->ret)
43 )
44 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
kernel test robot
build botRe: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Ritesh Harjani (IBM)kernel test robot noticed the following build warnings:
Hi Ritesh,
[This is a private test report for your RFC patch.] kernel test robot noticed the following build warnings:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on xfs-linux/for-next jack-fs/for_next linus/master v6.3-rc6 next-20230411] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev patch link: https://lore.kernel.org/r/f9825fab612761bee205046ce6e6e4caf25642ee.1681188927.git.ritesh.list%40gmail.com patch subject: [RFCv2 8/8] ext2: Add direct-io trace points config: i386-randconfig-a006-20230410 (https://download.01.org/0day-ci/archive/20230411/[email protected]/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 git checkout 4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ext2/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/./trace.h:61:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:237:
>> fs/ext2/./trace.h:39:5: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
__entry->count,
^~~~~~~~~~~~~~~
include/trace/stages/stage3_trace_output.h:6:17: note: expanded from macro '__entry'
#define __entry field
^
include/trace/stages/stage3_trace_output.h:9:43: note: expanded from macro 'TP_printk'
#define TP_printk(fmt, args...) fmt "\n", args
~~~ ^
include/trace/trace_events.h:203:27: note: expanded from macro 'DECLARE_EVENT_CLASS'
trace_event_printf(iter, print); \
^~~~~
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/./trace.h:61:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:419:
fs/ext2/./trace.h:29:27: error: implicit declaration of function 'iov_iter_count' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
__entry->count = iter ? iov_iter_count(iter) : ret;
^
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/./trace.h:61:
In file included from include/trace/define_trace.h:103:
In file included from include/trace/perf.h:75:
fs/ext2/./trace.h:29:27: error: implicit declaration of function 'iov_iter_count' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
__entry->count = iter ? iov_iter_count(iter) : ret;
^1 warning and 2 errors generated.
vim +39 fs/ext2/./trace.h
10
11 DECLARE_EVENT_CLASS(ext2_dio_class,
12 TP_PROTO(struct kiocb *iocb, struct iov_iter *iter, int ret),
13 TP_ARGS(iocb, iter, ret),
14 TP_STRUCT__entry(
15 __field(dev_t, dev)
16 __field(ino_t, ino)
17 __field(loff_t, isize)
18 __field(loff_t, pos)
19 __field(size_t, count)
20 __field(int, ki_flags)
21 __field(int, aio)
22 __field(int, ret)
23 ),
24 TP_fast_assign(
25 __entry->dev = file_inode(iocb->ki_filp)->i_sb->s_dev;
26 __entry->ino = file_inode(iocb->ki_filp)->i_ino;
27 __entry->isize = file_inode(iocb->ki_filp)->i_size;
28 __entry->pos = iocb->ki_pos;
29 __entry->count = iter ? iov_iter_count(iter) : ret;
30 __entry->ki_flags = iocb->ki_flags;
31 __entry->aio = !is_sync_kiocb(iocb);
32 __entry->ret = iter ? ret : 0;
33 ),
34 TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %ld flags %s aio %d ret=%d",
35 MAJOR(__entry->dev), MINOR(__entry->dev),
36 __entry->ino,
37 __entry->isize,
38 __entry->pos,
> 39 __entry->count,
40 __print_flags(__entry->ki_flags, "|", IOCB_STRINGS),
41 __entry->aio,
42 __entry->ret)
43 )
44 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Re: [RFCv2 2/8] libfs: Add __generic_file_fsync_nolock implementation
In reply to Christoph HellwigQuoted 6 lines · Christoph Hellwig
Quoted 3 lines · Ritesh Harjani (IBM)
+/**
+ *__generic_file_fsync_nolock- generic fsync implementation for simple
+ * filesystems with no inode lockNo reallz need for the __ prefix in the name.
It kind of makes sense though.
generic_file_fsync does the flush__generic_file_fsync doesn't do the flush __generic_file_fsync_nolock doesn't do the flush and doesn't lock/unlock
Quoted 4 lines
Quoted 1 line
+extern int
__generic_file_fsync_nolock(struct file *, loff_t, loff_t, int);No need for the extern. And at least I personally prefer to spell out the parameter names to make the prototype much more readable.
Agreed, although I make an exception for the 'struct file *'. Naming that parameter adds no value, but a plain int is just obscene.
int __generic_file_fsync_nolock(struct file *, loff_t start, loff_t end,
bool datasync);
(yes, the other variants don't use a bool for datasync, but they should)
kernel test robot
build botRe: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Ritesh Harjani (IBM)kernel test robot noticed the following build errors:
Hi Ritesh,
[This is a private test report for your RFC patch.] kernel test robot noticed the following build errors:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on xfs-linux/for-next jack-fs/for_next linus/master v6.3-rc6 next-20230411] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev patch link: https://lore.kernel.org/r/f9825fab612761bee205046ce6e6e4caf25642ee.1681188927.git.ritesh.list%40gmail.com patch subject: [RFCv2 8/8] ext2: Add direct-io trace points config: arm-randconfig-r024-20230409 (https://download.01.org/0day-ci/archive/20230411/[email protected]/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 2c57868e2e877f73c339796c3374ae660bb77f0d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 git checkout 4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash fs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/trace.h:61:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:237:
>> fs/ext2/trace.h:39:5: warning: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
__entry->count,
^~~~~~~~~~~~~~~
include/trace/stages/stage3_trace_output.h:6:17: note: expanded from macro '__entry'
#define __entry field
^
include/trace/stages/stage3_trace_output.h:9:43: note: expanded from macro 'TP_printk'
#define TP_printk(fmt, args...) fmt "\n", args
~~~ ^
include/trace/trace_events.h:203:27: note: expanded from macro 'DECLARE_EVENT_CLASS'
trace_event_printf(iter, print); \
^~~~~
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/trace.h:61:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:419:
>> fs/ext2/trace.h:29:27: error: call to undeclared function 'iov_iter_count'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
__entry->count = iter ? iov_iter_count(iter) : ret;
^
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/trace.h:61:
In file included from include/trace/define_trace.h:103:
In file included from include/trace/perf.h:75:
>> fs/ext2/trace.h:29:27: error: call to undeclared function 'iov_iter_count'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
__entry->count = iter ? iov_iter_count(iter) : ret;
^1 warning and 2 errors generated.
vim +/iov_iter_count +29 fs/ext2/trace.h
10
11 DECLARE_EVENT_CLASS(ext2_dio_class,
12 TP_PROTO(struct kiocb *iocb, struct iov_iter *iter, int ret),
13 TP_ARGS(iocb, iter, ret),
14 TP_STRUCT__entry(
15 __field(dev_t, dev)
16 __field(ino_t, ino)
17 __field(loff_t, isize)
18 __field(loff_t, pos)
19 __field(size_t, count)
20 __field(int, ki_flags)
21 __field(int, aio)
22 __field(int, ret)
23 ),
24 TP_fast_assign(
25 __entry->dev = file_inode(iocb->ki_filp)->i_sb->s_dev;
26 __entry->ino = file_inode(iocb->ki_filp)->i_ino;
27 __entry->isize = file_inode(iocb->ki_filp)->i_size;
28 __entry->pos = iocb->ki_pos;
> 29 __entry->count = iter ? iov_iter_count(iter) : ret;
30 __entry->ki_flags = iocb->ki_flags;
31 __entry->aio = !is_sync_kiocb(iocb);
32 __entry->ret = iter ? ret : 0;
33 ),
34 TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %ld flags %s aio %d ret=%d",
35 MAJOR(__entry->dev), MINOR(__entry->dev),
36 __entry->ino,
37 __entry->isize,
38 __entry->pos,
> 39 __entry->count,
40 __print_flags(__entry->ki_flags, "|", IOCB_STRINGS),
41 __entry->aio,
42 __entry->ret)
43 )
44 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
kernel test robot
build botRe: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Ritesh Harjani (IBM)kernel test robot noticed the following build errors:
Hi Ritesh,
[This is a private test report for your RFC patch.] kernel test robot noticed the following build errors:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on xfs-linux/for-next jack-fs/for_next linus/master v6.3-rc6 next-20230411] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev patch link: https://lore.kernel.org/r/f9825fab612761bee205046ce6e6e4caf25642ee.1681188927.git.ritesh.list%40gmail.com patch subject: [RFCv2 8/8] ext2: Add direct-io trace points config: x86_64-randconfig-a002-20230410 (https://download.01.org/0day-ci/archive/20230411/[email protected]/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ritesh-Harjani-IBM/ext2-dax-Fix-ext2_setsize-when-len-is-page-aligned/20230411-132421 git checkout 4fff7c561c76e8ee730b2b5ec288aeeb65ca449d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/./trace.h:61:
In file included from include/trace/define_trace.h:102:
In file included from include/trace/trace_events.h:419:
>> fs/ext2/./trace.h:29:27: error: implicit declaration of function 'iov_iter_count' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
__entry->count = iter ? iov_iter_count(iter) : ret;
^
In file included from fs/ext2/trace.c:5:
In file included from fs/ext2/./trace.h:61:
In file included from include/trace/define_trace.h:103:
In file included from include/trace/perf.h:75:
>> fs/ext2/./trace.h:29:27: error: implicit declaration of function 'iov_iter_count' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
__entry->count = iter ? iov_iter_count(iter) : ret;
^2 errors generated.
vim +/iov_iter_count +29 fs/ext2/./trace.h
10
11 DECLARE_EVENT_CLASS(ext2_dio_class,
12 TP_PROTO(struct kiocb *iocb, struct iov_iter *iter, int ret),
13 TP_ARGS(iocb, iter, ret),
14 TP_STRUCT__entry(
15 __field(dev_t, dev)
16 __field(ino_t, ino)
17 __field(loff_t, isize)
18 __field(loff_t, pos)
19 __field(size_t, count)
20 __field(int, ki_flags)
21 __field(int, aio)
22 __field(int, ret)
23 ),
24 TP_fast_assign(
25 __entry->dev = file_inode(iocb->ki_filp)->i_sb->s_dev;
26 __entry->ino = file_inode(iocb->ki_filp)->i_ino;
27 __entry->isize = file_inode(iocb->ki_filp)->i_size;
28 __entry->pos = iocb->ki_pos;
> 29 __entry->count = iter ? iov_iter_count(iter) : ret;
30 __entry->ki_flags = iocb->ki_flags;
31 __entry->aio = !is_sync_kiocb(iocb);
32 __entry->ret = iter ? ret : 0;
33 ),
34 TP_printk("dev %d:%d ino 0x%lx isize 0x%llx pos 0x%llx count %ld flags %s aio %d ret=%d",
35 MAJOR(__entry->dev), MINOR(__entry->dev),
36 __entry->ino,
37 __entry->isize,
38 __entry->pos,
39 __entry->count,
40 __print_flags(__entry->ki_flags, "|", IOCB_STRINGS),
41 __entry->aio,
42 __entry->ret)
43 )
44 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Re: [RFCv2 1/8] ext2/dax: Fix ext2_setsize when len is page aligned
In reply to Ritesh Harjani (IBM)Quoted 45 lines · Ritesh Harjani (IBM)
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 indax_zero_range()function, that means the length gets passed as 0 to ->iomap_begin().In ext2 it then calls
ext2_get_blocks->max_blocksas 0 and hitsbug_onhere inext2_get_blocks().BUG_ON(maxblocks == 0);Instead we should be calling
dax_truncate_page()here which takes care of it. i.e. it only callsdax_zero_rangeif 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
Quoted 1 line
Signed-off-by Ritesh Harjani (IBM) <[email protected]>
Would seem to make sense...
--D
Quoted 23 lines
--- fs/ext2/inode.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)fs/ext2/inode.c +2
-3@@ -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
Re: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Christoph HellwigQuoted 5 lines · Christoph Hellwig
Quoted 2 lines · Ritesh Harjani (IBM)
This patch adds the trace point to ext2 direct-io apis in
fs/ext2/file.cWouldn't it make more sense to add this to iomap instead?
Yes please. :)
--D
Ritesh Harjani (IBM)
reviewerRe: [RFCv2 8/8] ext2: Add direct-io trace points
Quoted 8 lines · Darrick J. Wong
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Ritesh Harjani (IBM)
This patch adds the trace point to ext2 direct-io apis in
fs/ext2/file.cWouldn't it make more sense to add this to iomap instead?
Yes please. :)
Sure. Let me also add a trace event for iomap DIO as well in the next revision. However I would like to keep ext2 dio trace point as is :)
-ritesh
Ritesh Harjani (IBM)
reviewerRe: [RFCv2 2/8] libfs: Add __generic_file_fsync_nolock implementation
In reply to Matthew WilcoxQuoted 6 lines · Matthew Wilcox
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Ritesh Harjani (IBM)
+/**
+ *__generic_file_fsync_nolock- generic fsync implementation for simple
+ * filesystems with no inode lock
Quoted 7 lines
Quoted 1 line
No reallz need for the __ prefix in the name.
It kind of makes sense though.
generic_file_fsyncdoes the flush__generic_file_fsyncdoesn't do the flush__generic_file_fsync_nolockdoesn't do the flush and doesn't lock/unlock
Yes.
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
+extern int
__generic_file_fsync_nolock(struct file *, loff_t, loff_t, int);No need for the extern. And at least I personally prefer to spell out the parameter names to make the prototype much more readable.
Quoted 7 lines
Agreed, although I make an exception for the 'struct file *'. Naming that parameter adds no value, but a plain int is just obscene.
int __generic_file_fsync_nolock(struct file *,
loff_tstart,loff_tend,bool datasync);(yes, the other variants don't use a bool for datasync, but they should)
Sure. Will make the change.
-ritesh
Ritesh Harjani (IBM)
reviewerRe: [RFCv2 5/8] ext2: Move direct-io to use iomap
In reply to Christoph HellwigQuoted 5 lines · Christoph Hellwig
Quoted 1 line · Ritesh Harjani (IBM)
+extern void
ext2_write_failed(struct address_space *mapping, loff_t to);No need for the extern.
Sure will drop it.
Quoted 24 lines
Quoted 24 lines
fs/ext2/file.c +24
-0+/* 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 ; + pos = iocb->ki_pos; + status =generic_perform_write (iocb, from); +if (unlikely (status <0 )) { + ret = status; + goto out_unlock; + } + + iocb->ki_pos += status; + ret += status; + endbyte = pos + status -1 ; + ret2 =filemap_write_and_wait_range (inode->i_mapping, pos, + endbyte); +if (!ret2) +invalidate_mapping_pages (inode->i_mapping, + pos >>PAGE_SHIFT , + endbyte >>PAGE_SHIFT ); +if (ret >0 )
Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ generic_write_sync(iocb, ret); + }Nit, but to me it would seem cleaner if all the fallback handling was moved into a separate helper function. Or in fact by not using
generic_file_write_itereven for buffered I/O and at doing the pre-I/O checks and the finalgeneric_write_syncin common code in ext2 for direct and buffered I/O.
Make sense. However, since we are on the path to modify ext2 buffered-io code as well to move to iomap interface, I wouldn't bother too much as of now for this code as, all of this is going to go away anyways.
Quoted 10 lines
fs/ext2/inode.c static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, · line 820
Quoted 7 lines
fs/ext2/inode.c +7
-0+/* +* For writes that could fill holes inside i_size on a +* DIO_SKIP_HOLES filesystem we forbid block creations: only +* overwrites are permitted. +*/ +if ((flags &IOMAP_DIRECT ) && (first_block << blkbits <i_size_read (inode))) + create =0 ;No need for braes around the < operation, but I think you might need them around the shift.
left-shift has a higher precedence. But let me make it more clear in next rev.
Quoted 3 lines
Also an overly long line here.
Sure, will see to it.
Quoted 3 lines
fs/ext2/inode.c static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, · line 827
Quoted 1 line
fs/ext2/inode.c +1
-0+if ((flags &IOMAP_WRITE ) && (offset + length >i_size_read (inode)))No need for the second set of inner braces here either.
It's just avoids any confusion this way.
-ritesh
Re: [RFCv2 2/8] libfs: Add __generic_file_fsync_nolock implementation
In reply to Matthew WilcoxQuoted 6 lines · Matthew Wilcox
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Ritesh Harjani (IBM)
+/**
+ *__generic_file_fsync_nolock- generic fsync implementation for simple
+ * filesystems with no inode lock
Quoted 7 lines
Quoted 1 line
No reallz need for the __ prefix in the name.
It kind of makes sense though.
generic_file_fsyncdoes the flush__generic_file_fsyncdoesn't do the flush__generic_file_fsync_nolockdoesn't do the flush and doesn't lock/unlock
Indeed. Part of it is that the naming is a bit horrible. Maybe it should move to buffer.c and be called generic_buffer_fsync, or generic_block_fsync which still wouldn't be perfect but match the buffer.c naming scheme.
Quoted 11 lines
Quoted 5 lines
Quoted 2 lines
+extern int
__generic_file_fsync_nolock(struct file *, loff_t, loff_t, int);No need for the extern. And at least I personally prefer to spell out the parameter names to make the prototype much more readable.
Agreed, although I make an exception for the 'struct file *'. Naming that parameter adds no value, but a plain int is just obscene.
int __generic_file_fsync_nolock(struct file *,
loff_tstart,loff_tend,bool datasync);
While I agree that it's not needed for the file, leaving it out is a bit silly.
Quoted 1 line
(yes, the other variants don't use a bool for datasync, but they should)
.. including the ->fsync prototype to make it work ..
Re: [RFCv2 5/8] ext2: Move direct-io to use iomap
In reply to Ritesh Harjani (IBM)Quoted 5 lines · Ritesh Harjani
Quoted 3 lines
fs/ext2/inode.c static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, · line 827
Quoted 1 line
fs/ext2/inode.c +1
-0+if ((flags &IOMAP_WRITE ) && (offset + length >i_size_read (inode)))No need for the second set of inner braces here either.
It's just avoids any confusion this way.
Does it? Except for some really weird places it is very unusual in the kernel. To me it really does add confusion.
Re: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Ritesh Harjani (IBM)Quoted 2 lines · Ritesh Harjani
Sure. Let me also add a trace event for iomap DIO as well in the next revision. However I would like to keep ext2 dio trace point as is :)
Am I confused, or does this patch only add new trace points anyway?
Quoted 2 lines
-ritesh
---end quoted text---
Jan Kara
reviewerHi Ritesh!
Quoted 15 lines · Ritesh Harjani (IBM)
Please find the series which moves ext2 direct-io to use modern iomap interface.
Here are some more details -
1. Patch-1: Fixes a kernelbug_onproblem with ext2 dax code (found during code review and testing).
2. Patch-2: Adds a__generic_file_fsync_nolockimplementation as we had discussed.
3. Patch-3 & Patch-4: Moves ext4 nojournal and ext2 to use_nolockmethod.
4. Patch-5: This is the main patch which moves ext2 direct-io to use iomap. (more details can be found in the patch)
5. Patch-6: KillsIOMAP_DIO_NOSYNCflag as it is not in use by any filesystem.
6. Patch-7: addsIOCB_STRINGSmacro for use in trace events for better trace output of iocb flags.
7. Patch-8: Add ext2 trace point for DIO.
Quoted 4 lines
Testing:
=========
This passes ext2 "auto" group testing for fstests. There were no new failures with this patches.
I went through the patches and I have no further comments besides what has been already said. So feel free to update patches based on other feedback, I'll do the last round of review and can queue the patches to my tree.
Honza
Quoted 30 lines
Ritesh Harjani (IBM) (8):
ext2/dax: Fixext2_setsizewhen len is page aligned
libfs: Add__generic_file_fsync_nolockimplementation
ext4: Use__generic_file_fsync_nolockimplementation
ext2: Use__generic_file_fsync_nolockimplementation
ext2: Move direct-io to use iomap
iomap: RemoveIOMAP_DIO_NOSYNCunused dio flagfs.h: AddIOCB_STRINGSfor use in trace points
ext2: Add direct-io trace pointsfs/ext2/Makefile | 2 +- fs/ext2/ext2.h | 1 + fs/ext2/file.c | 127 +++++++++++++++++++++++++++++++++++++++++- fs/ext2/inode.c | 57 +++++++++++-------- fs/ext2/trace.c | 5 ++ fs/ext2/trace.h | 61 ++++++++++++++++++++ fs/ext4/fsync.c | 31 +++++------ fs/iomap/direct-io.c | 2 +- fs/libfs.c | 43 ++++++++++++++ include/linux/fs.h | 15 +++++ include/linux/iomap.h | 6 -- 11 files changed, 303 insertions(+), 47 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)
reviewerRe: [RFCv2 2/8] libfs: Add __generic_file_fsync_nolock implementation
In reply to Christoph HellwigQuoted 11 lines · Christoph Hellwig
Quoted 10 lines · Matthew Wilcox
Quoted 6 lines · Christoph Hellwig
Quoted 3 lines · Ritesh Harjani (IBM)
+/**
+ *__generic_file_fsync_nolock- generic fsync implementation for simple
+ * filesystems with no inode lockNo reallz need for the __ prefix in the name.
It kind of makes sense though.
Quoted 9 lines
Quoted 3 lines
generic_file_fsyncdoes the flush__generic_file_fsyncdoesn't do the flush__generic_file_fsync_nolockdoesn't do the flush and doesn't lock/unlockIndeed. Part of it is that the naming is a bit horrible. Maybe it should move to
buffer.cand be calledgeneric_buffer_fsync, orgeneric_block_fsyncwhich still wouldn't be perfect but match thebuffer.cnaming scheme.
Eventually it anyways needs some work to see if we can kill the lock variant all together. I didn't do that in this series which is focused on ext2 conversion of iomap.
So, if it's not that bad, I would like to keep both function definitions at one place so that it can be worked out later.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
+extern int
__generic_file_fsync_nolock(struct file *, loff_t, loff_t, int);No need for the extern. And at least I personally prefer to spell out the parameter names to make the prototype much more readable.
Quoted 9 lines
Quoted 5 lines
Agreed, although I make an exception for the 'struct file *'. Naming that parameter adds no value, but a plain int is just obscene.
int __generic_file_fsync_nolock(struct file *,
loff_tstart,loff_tend,bool datasync);While I agree that it's not needed for the file, leaving it out is a bit silly.
Sure. Will fix it.
Quoted 3 lines
Quoted 1 line
(yes, the other variants don't use a bool for datasync, but they should)
.. including the ->fsync prototype to make it work ..
Sure, this work should go as a seperate series.
-ritesh
Ritesh Harjani (IBM)
reviewerRe: [RFCv2 5/8] ext2: Move direct-io to use iomap
In reply to Christoph HellwigQuoted 9 lines · Christoph Hellwig
Quoted 5 lines · Ritesh Harjani
Quoted 3 lines
fs/ext2/inode.c static int ext2_iomap_begin(struct inode *inode, loff_t offset, loff_t length, · line 827
Quoted 1 line
fs/ext2/inode.c +1
-0+if ((flags &IOMAP_WRITE ) && (offset + length >i_size_read (inode)))No need for the second set of inner braces here either.
It's just avoids any confusion this way.
Does it? Except for some really weird places it is very unusual in the kernel. To me it really does add confusion.
Ok, In that case, will change it.
Thanks for the review!
-ritesh
Ritesh Harjani (IBM)
reviewerRe: [RFCv2 8/8] ext2: Add direct-io trace points
In reply to Christoph HellwigQuoted 5 lines · Christoph Hellwig
Quoted 2 lines · Ritesh Harjani
Sure. Let me also add a trace event for iomap DIO as well in the next revision. However I would like to keep ext2 dio trace point as is :)
Am I confused, or does this patch only add new trace points anyway?
I meant to keep ext2 trace point as well. One of the reason for that would be if someone wants to just enable all ext2 related tracing logs, it would be easier.
$ trace-cmd -e ext2 $ trace-cmd report
But since iomap dio also doesn't have a tracepoint, I am looking to add that in the next revision.
-ritesh
Ritesh Harjani (IBM)
reviewerQuoted 18 lines · Jan Kara
Hi Ritesh!
Quoted 15 lines · Ritesh Harjani (IBM)
Please find the series which moves ext2 direct-io to use modern iomap interface.
Here are some more details -
1. Patch-1: Fixes a kernelbug_onproblem with ext2 dax code (found during code review and testing).
2. Patch-2: Adds a__generic_file_fsync_nolockimplementation as we had discussed.
3. Patch-3 & Patch-4: Moves ext4 nojournal and ext2 to use_nolockmethod.
4. Patch-5: This is the main patch which moves ext2 direct-io to use iomap. (more details can be found in the patch)
5. Patch-6: KillsIOMAP_DIO_NOSYNCflag as it is not in use by any filesystem.
6. Patch-7: addsIOCB_STRINGSmacro for use in trace events for better trace output of iocb flags.
7. Patch-8: Add ext2 trace point for DIO.
Quoted 9 lines
Quoted 4 lines
Testing:
=========
This passes ext2 "auto" group testing for fstests. There were no new failures with this patches.I went through the patches and I have no further comments besides what has been already said. So feel free to update patches based on other feedback, I'll do the last round of review and can queue the patches to my tree.
Sure, thanks Jan for going over the patches.
I will send the next rev soon addressing review comments along with some other minor changes.
-ritesh
Christian Brauner
maintainerRe: [RFCv2 2/8] libfs: Add __generic_file_fsync_nolock implementation
In reply to Christoph HellwigQuoted 26 lines · Christoph Hellwig
Quoted 6 lines · Matthew Wilcox
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Ritesh Harjani (IBM)
+/**
+ *__generic_file_fsync_nolock- generic fsync implementation for simple
+ * filesystems with no inode lockQuoted 7 lines
Quoted 1 line
No reallz need for the __ prefix in the name.
It kind of makes sense though.
generic_file_fsyncdoes the flush__generic_file_fsyncdoesn't do the flush__generic_file_fsync_nolockdoesn't do the flush and doesn't lock/unlockIndeed. Part of it is that the naming is a bit horrible. Maybe it should move to
buffer.cand be calledgeneric_buffer_fsync, orgeneric_block_fsyncwhich still wouldn't be perfect but match thebuffer.cnaming scheme.Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
+extern int
__generic_file_fsync_nolock(struct file *, loff_t, loff_t, int);No need for the extern. And at least I personally prefer to spell out the parameter names to make the prototype much more readable.
Quoted 8 lines
Quoted 5 lines
Agreed, although I make an exception for the 'struct file *'. Naming that parameter adds no value, but a plain int is just obscene.
int __generic_file_fsync_nolock(struct file *,
loff_tstart,loff_tend,bool datasync);While I agree that it's not needed for the file, leaving it out is a bit silly.
I think we should just be consistent and try to enforce that the parameter name is added in new patches. It's often easier for grepping and there's really not a lot of value in leaving it out in general.