diff mbox series

[RFC,v2,08/16] fs: add pending file update time flag.

Message ID 20220516164718.2419891-9-shr@fb.com (mailing list archive)
State New
Headers show
Series io-uring/xfs: support async buffered writes | expand

Commit Message

Stefan Roesch May 16, 2022, 4:47 p.m. UTC
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(+)

Comments

Jan Kara May 17, 2022, 11:28 a.m. UTC | #1
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
Christian Brauner May 17, 2022, 1:48 p.m. UTC | #2
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.
Stefan Roesch May 18, 2022, 11:23 p.m. UTC | #3
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 mbox series

Patch

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)