Re: [PATCH v4 3/3] FUSE: Avoid lookup in d_revalidate()

[Bernd Schubert <bschubert@ddn.com>]

On 5/4/22 22:39, Vivek Goyal wrote:
> On Mon, May 02, 2022 at 03:55:21PM +0530, Dharmendra Singh wrote:
>> From: Dharmendra Singh <dsingh@ddn.com>
>>
>> With atomic open + lookup implemented, it is possible
>> to avoid lookups in FUSE d_revalidate() for objects
>> other than directories.
>>
>> If FUSE is mounted with default permissions then this
>> optimization is not possible as we need to fetch fresh
>> inode attributes for permission check. This lookup
>> skipped in d_revalidate() can be performed  as part of
>> open call into libfuse which is made from fuse_file_open().
>> And when we return from USER SPACE with file opened and
>> fresh attributes, we can revalidate the inode.
>>
>> Signed-off-by: Dharmendra Singh <dsingh@ddn.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> ---
>>   fs/fuse/dir.c    | 89 ++++++++++++++++++++++++++++++++++++++++++------
>>   fs/fuse/file.c   | 30 ++++++++++++++--
>>   fs/fuse/fuse_i.h | 10 +++++-
>>   fs/fuse/ioctl.c  |  2 +-
>>   4 files changed, 115 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index 6879d3a86796..1594fecc920f 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -196,6 +196,7 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
>>    * the lookup once more.  If the lookup results in the same inode,
>>    * then refresh the attributes, timeouts and mark the dentry valid.
>>    */
>> +
>>   static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
>>   {
>>   	struct inode *inode;
>> @@ -224,6 +225,17 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
>>   
>>   		fm = get_fuse_mount(inode);
>>   
>> +		/* If atomic open is supported by FUSE then use this opportunity
>> +		 * (only for non-dir) to avoid this lookup and combine
>> +		 * lookup + open into single call.
>> +		 */
>> +		if (!fm->fc->default_permissions && fm->fc->do_atomic_open &&
>> +		    !(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) &&
>> +		    (flags & LOOKUP_OPEN) && !S_ISDIR(inode->i_mode)) {
>> +			ret = 1;
> 
> So basically we think that VFS is going to do OPEN and calling
> ->revalidate() before that. So we are returning "1" to vfs saying
> dentry is valid (despite the fact that we have no idea at this
> point of time).
> 
> And later when open comes we are opening and revalidating inode etc.
> 
> Seriously, IMHO, all this seems very fragile and hard to understand
> and maintain code. Things can go easily wrong if even little bit
> of assumptions change in VFS.
> 
> This sounds more like VFS should know about it and if VFS knows
> that filesystem supports facility where it can open + revalidate
> at the same time, it should probably call that. Something like
> ->open_revalidate() etc. That would be much more maintainable code but
> this feels like very fragile to me, IMHO.
> 

I'm not opposed to make things more clear, but AFAIK these lookup-intent 
flags are the way how it works for quite some time. Also see 
nfs_lookup_verify_inode(), which makes use of that the same way. I 
entirely agree, though, that using a dedicated method would make things 
much easier to understand. It is just a bit more complicated to get in 
patches that change the vfs...

Adding in a vfs ->open_revalidate might have the advantage that we could 
also support 'default_permissions' - ->open_revalidate  needs to 
additionally check the retrieved file permissions and and needs to call 
into generic_permissions for that. Right now that is not easily 
feasible, without adding some code dup to convert flags in MAY_* flags - 
a vfs change would be needed here to pass the right flags.

The other part that is missing in the current patches is something like 
->revalidate_getattr -  stat() of positive dentry first sends a 
revalidate and then another getattr and right now there is no good way 
to combine these.


Thanks,
Bernd