Re: [RFC PATCH v4 12/17] fs: Optimization for concurrent file time updates.

[Stefan Roesch <shr@fb.com>]

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