| Message ID | 20220520183646.2002023-13-shr@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io-uring/xfs: support async buffered writes | expand |
On Fri 20-05-22 11:36:41, Stefan Roesch wrote: > This introduces the S_PENDING_TIME flag. If an async buffered write > needs to update the time, it cannot be processed in the fast path of > io-uring. When a time update is pending this flag is set for async > buffered writes. Other concurrent async buffered writes for the same > file do not need to wait while this time update is pending. > > This reduces the number of async buffered writes that need to get punted > to the io-workers in io-uring. > > Signed-off-by: Stefan Roesch <shr@fb.com> ... > @@ -2184,10 +2184,17 @@ int file_modified_async(struct file *file, int flags) > ret = file_needs_update_time(inode, file, &now); > if (ret <= 0) > return ret; > - if (flags & IOCB_NOWAIT) > + if (flags & IOCB_NOWAIT) { > + if (IS_PENDING_TIME(inode)) > + return 0; > + > + inode->i_flags |= S_PENDING_TIME; > return -EAGAIN; > + } > > - return __file_update_time(inode, file, &now, ret); > + ret = __file_update_time(inode, file, &now, ret); > + inode->i_flags &= ~S_PENDING_TIME; > + return ret; > } > EXPORT_SYMBOL(file_modified_async); You still didn't address my concern that i_flags is modified without the protection of i_rwsem here. That can lead to corruption of i_flags value and rather nasty (and hard to debug) consequences. You can use inode_set_flags() here to make things kinda safe. The whole inode->i_flags handling is a mess but not yours to resolve ;) Honza
On 5/23/22 3:51 AM, Jan Kara wrote: > On Fri 20-05-22 11:36:41, Stefan Roesch wrote: >> This introduces the S_PENDING_TIME flag. If an async buffered write >> needs to update the time, it cannot be processed in the fast path of >> io-uring. When a time update is pending this flag is set for async >> buffered writes. Other concurrent async buffered writes for the same >> file do not need to wait while this time update is pending. >> >> This reduces the number of async buffered writes that need to get punted >> to the io-workers in io-uring. >> >> Signed-off-by: Stefan Roesch <shr@fb.com> > > ... > >> @@ -2184,10 +2184,17 @@ int file_modified_async(struct file *file, int flags) >> ret = file_needs_update_time(inode, file, &now); >> if (ret <= 0) >> return ret; >> - if (flags & IOCB_NOWAIT) >> + if (flags & IOCB_NOWAIT) { >> + if (IS_PENDING_TIME(inode)) >> + return 0; >> + >> + inode->i_flags |= S_PENDING_TIME; >> return -EAGAIN; >> + } >> >> - return __file_update_time(inode, file, &now, ret); >> + ret = __file_update_time(inode, file, &now, ret); >> + inode->i_flags &= ~S_PENDING_TIME; >> + return ret; >> } >> EXPORT_SYMBOL(file_modified_async); > > You still didn't address my concern that i_flags is modified without the > protection of i_rwsem here. That can lead to corruption of i_flags value > and rather nasty (and hard to debug) consequences. You can use > inode_set_flags() here to make things kinda safe. The whole inode->i_flags > handling is a mess but not yours to resolve ;) > Replaced directly manipulating the inode flags with calls to the function inode_set_flags(). > Honza
diff --git a/fs/inode.c b/fs/inode.c index 3a5d0fa468ab..5c5021787780 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2184,10 +2184,17 @@ int file_modified_async(struct file *file, int flags) ret = file_needs_update_time(inode, file, &now); if (ret <= 0) return ret; - if (flags & IOCB_NOWAIT) + if (flags & IOCB_NOWAIT) { + if (IS_PENDING_TIME(inode)) + return 0; + + inode->i_flags |= S_PENDING_TIME; return -EAGAIN; + } - return __file_update_time(inode, file, &now, ret); + ret = __file_update_time(inode, file, &now, ret); + inode->i_flags &= ~S_PENDING_TIME; + return ret; } EXPORT_SYMBOL(file_modified_async); diff --git a/include/linux/fs.h b/include/linux/fs.h index 9760283af7dc..5f3aaf61fb4b 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2141,6 +2141,8 @@ struct super_operations { #define S_CASEFOLD (1 << 15) /* Casefolded file */ #define S_VERITY (1 << 16) /* Verity file (using fs/verity/) */ #define S_KERNEL_FILE (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */ +#define S_PENDING_TIME (1 << 18) /* File update time is pending */ + /* * Note that nosuid etc flags are inode-specific: setting some file-system @@ -2183,6 +2185,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags #define IS_ENCRYPTED(inode) ((inode)->i_flags & S_ENCRYPTED) #define IS_CASEFOLDED(inode) ((inode)->i_flags & S_CASEFOLD) #define IS_VERITY(inode) ((inode)->i_flags & S_VERITY) +#define IS_PENDING_TIME(inode) ((inode)->i_flags & S_PENDING_TIME) #define IS_WHITEOUT(inode) (S_ISCHR(inode->i_mode) && \ (inode)->i_rdev == WHITEOUT_DEV)
This introduces the S_PENDING_TIME flag. If an async buffered write needs to update the time, it cannot be processed in the fast path of io-uring. When a time update is pending this flag is set for async buffered writes. Other concurrent async buffered writes for the same file do not need to wait while this time update is pending. This reduces the number of async buffered writes that need to get punted to the io-workers in io-uring. Signed-off-by: Stefan Roesch <shr@fb.com> --- fs/inode.c | 11 +++++++++-- include/linux/fs.h | 3 +++ 2 files changed, 12 insertions(+), 2 deletions(-)