Re: [RFC PATCH] vfs: allow ->atomic_open() on positive

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, May 19, 2022 at 10:43:54PM +0200, Bernd Schubert wrote:
> 
> 
> On 5/19/22 22:09, Vivek Goyal wrote:
> > On Thu, May 19, 2022 at 04:43:58PM +0200, Miklos Szeredi wrote:
> > > Hi Al,
> > > 
> > > Do you see anything bad with allowing ->atomic_open() to take a positive dentry
> > > and possibly invalidate it after it does the atomic LOOKUP/CREATE+OPEN?
> > > 
> > > It looks wrong not to allow optimizing away the roundtrip associated with
> > > revalidation when we do allow optimizing away the roundtrip for the initial
> > > lookup in the same situation.
> > > 
> > > Thanks,
> > > Miklos
> > > 
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 509657fdf4f5..d35b5cbf7f64 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -3267,7 +3267,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> > >   		dput(dentry);
> > >   		dentry = NULL;
> > >   	}
> > > -	if (dentry->d_inode) {
> > > +	if (dentry->d_inode && !d_atomic_open(dentry)) {
> > >   		/* Cached positive dentry: will open in f_op->open */
> > >   		return dentry;
> > 
> > Hi Miklos,
> > 
> > I see that lookup_open() calls d_revalidate() first. So basically
> > idea is that fuse ->.d_revalidate will skip LOOKUP needed to make sure
> > dentry is still valid (Only if atomic lookup+open is implemented) and
> > return 1 claiming dentry is valid.
> > 
> > And later in ->atomic_open(), it will either open the file or
> > get an error and invalidate dentry. Hence will save one LOOKUP in
> > success case. Do I understand the intent right?
> 
> Yeah, I think Dharmendra and I had internally already debated over this. In
> order to reduce complexity for the patches we preferred to go without vfs
> modifications.

Fair enough. It is not trivial to be able to see all the paths and make
sure none of these paths is broken.

> 
> I assume the patch is a follow up to this comment
> https://lore.kernel.org/all/20220517100744.26849-1-dharamhans87@gmail.com/T/#m8bd440ddea4c135688c829f34e93371e861ba9fa
> 

Yes looks like. There are too many paths here and being able to wrap
one's head around all the paths is not trivial. Thankfully miklos
has summarized it here.

https://lore.kernel.org/all/20220517100744.26849-1-dharamhans87@gmail.com/T/#m90f64cd8c8fff70e2fba2b551ae01d0d47b3337e

Thanks
Vivek