| Message ID | 20220516164718.2419891-9-shr@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io-uring/xfs: support async buffered writes | expand |
On Mon 16-05-22 09:47:10, Stefan Roesch wrote: > This introduces an optimization for the update time flag and async > buffered writes. While an update of the file modification time is > pending and is handled by the workers, concurrent writes do not need > to wait for this time update to complete. > > Signed-off-by: Stefan Roesch <shr@fb.com> > --- > fs/inode.c | 1 + > include/linux/fs.h | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/fs/inode.c b/fs/inode.c > index 1d0b02763e98..fd18b2c1b7c4 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2091,6 +2091,7 @@ static int do_file_update_time(struct inode *inode, struct file *file, > return 0; > > ret = inode_update_time(inode, now, sync_mode); > + inode->i_flags &= ~S_PENDING_TIME; So what protects this update of inode->i_flags? Usually we use inode->i_rwsem for that but not all file_update_time() callers hold it... Honza
On Tue, May 17, 2022 at 01:28:16PM +0200, Jan Kara wrote: > On Mon 16-05-22 09:47:10, Stefan Roesch wrote: > > This introduces an optimization for the update time flag and async > > buffered writes. While an update of the file modification time is > > pending and is handled by the workers, concurrent writes do not need > > to wait for this time update to complete. > > > > Signed-off-by: Stefan Roesch <shr@fb.com> > > --- > > fs/inode.c | 1 + > > include/linux/fs.h | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index 1d0b02763e98..fd18b2c1b7c4 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -2091,6 +2091,7 @@ static int do_file_update_time(struct inode *inode, struct file *file, > > return 0; > > > > ret = inode_update_time(inode, now, sync_mode); > > + inode->i_flags &= ~S_PENDING_TIME; > > So what protects this update of inode->i_flags? Usually we use > inode->i_rwsem for that but not all file_update_time() callers hold it... I think the confusion might come about because file_modified() mentions that the caller is expected to hold file's inode_lock()... Another reason why we should probably add more kernel doc with a "Context:" line explaining what locks are expected to be held to these helpers.
On 5/17/22 4:28 AM, Jan Kara wrote: > On Mon 16-05-22 09:47:10, Stefan Roesch wrote: >> This introduces an optimization for the update time flag and async >> buffered writes. While an update of the file modification time is >> pending and is handled by the workers, concurrent writes do not need >> to wait for this time update to complete. >> >> Signed-off-by: Stefan Roesch <shr@fb.com> >> --- >> fs/inode.c | 1 + >> include/linux/fs.h | 3 +++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/fs/inode.c b/fs/inode.c >> index 1d0b02763e98..fd18b2c1b7c4 100644 >> --- a/fs/inode.c >> +++ b/fs/inode.c >> @@ -2091,6 +2091,7 @@ static int do_file_update_time(struct inode *inode, struct file *file, >> return 0; >> >> ret = inode_update_time(inode, now, sync_mode); >> + inode->i_flags &= ~S_PENDING_TIME; > > So what protects this update of inode->i_flags? Usually we use > inode->i_rwsem for that but not all file_update_time() callers hold it... > I'll move setting the flags to the file_modified() function. > Honza >
diff --git a/fs/inode.c b/fs/inode.c index 1d0b02763e98..fd18b2c1b7c4 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2091,6 +2091,7 @@ static int do_file_update_time(struct inode *inode, struct file *file, return 0; ret = inode_update_time(inode, now, sync_mode); + inode->i_flags &= ~S_PENDING_TIME; __mnt_drop_write_file(file); return ret; diff --git a/include/linux/fs.h b/include/linux/fs.h index 3b479d02e210..3da641dfa6d9 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 an optimization for the update time flag and async buffered writes. While an update of the file modification time is pending and is handled by the workers, concurrent writes do not need to wait for this time update to complete. Signed-off-by: Stefan Roesch <shr@fb.com> --- fs/inode.c | 1 + include/linux/fs.h | 3 +++ 2 files changed, 4 insertions(+)