On Thu, 19 May 2022 at 22:09, Vivek Goyal <vgoyal@xxxxxxxxxx> 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.
Yes.
> 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?
It should not fail in the stale dentry case either, just merge the
revalidation into ->atomic_open().
Thanks,
Miklos
