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

[Bernd Schubert]

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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >
> > ---
> >   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