Re: Re: [PATCH v7 4/4] fuse: add compound command for dentry revalidation

[Horst Birthelmer <horst@birthelmer.de>]

On Fri, Jun 05, 2026 at 10:06:55AM +0200, Amir Goldstein wrote:
> On Thu, Jun 4, 2026 at 11:51 AM Horst Birthelmer <horst@birthelmer.com> wrote:
> >
> > From: Horst Birthelmer <hbirthelmer@ddn.com>
> >
> > During dentry revalidation the compound LOOKUP+GETATTR
> > will save a round trip to user space.
> >
> > Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
> > ---
> >  fs/fuse/dir.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 111 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index b3406c33abd2..99800e8ca895 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -372,6 +372,101 @@ static void fuse_lookup_init(struct fuse_args *args, u64 nodeid,
> >         args->out_args[0].value = outarg;
> >  }
> >
> > +/*
> > + * Revalidate a dentry using a compound LOOKUP+GETATTR.  Saves a round
> > + * trip when both the entry and the attributes need refreshing.
> > + *
> > + * Returns 1 if valid, 0 if the dentry should be invalidated, or a
> > + * negative errno that the caller should propagate (only -ENOMEM /
> > + * -EINTR; other errors are mapped to invalidate).
> > + */
> > +static int fuse_dentry_revalidate_compound(struct inode *dir,
> > +                                          const struct qstr *name,
> > +                                          struct dentry *entry,
> > +                                          struct inode *inode,
> > +                                          struct fuse_mount *fm,
> > +                                          u64 attr_version)
> > +{
> > +       struct fuse_entry_out lookup_out = {};
> > +       struct fuse_attr_out getattr_out = {};
> > +       struct fuse_getattr_in getattr_in = {};
> > +       struct fuse_args lookup_args = {};
> > +       struct fuse_args getattr_args = {};
> > +       struct fuse_forget_link *forget;
> > +       int lookup_err = 0, getattr_err = 0;
> > +       struct fuse_compound_op ops[2] = {
> > +               { .arg = &lookup_args,  .error = &lookup_err,
> > +                 .dep_index = FUSE_COMPOUND_NO_DEP },
> > +               { .arg = &getattr_args, .error = &getattr_err,
> > +                 .dep_index = 0 /* nodeid comes from lookup */ },
> > +       };
> > +       struct fuse_inode *fi;
> > +       int ret;
> > +
> > +       forget = fuse_alloc_forget();
> > +       if (!forget)
> > +               return -ENOMEM;
> > +
> > +       fuse_lookup_init(&lookup_args, get_node_id(dir), name, &lookup_out);
> > +       /* nodeid is filled from the lookup result before getattr is sent */
> > +       fuse_getattr_args_fill(&getattr_args, 0, &getattr_in, &getattr_out);
> > +
> > +       ret = fuse_compound_send(fm, ops, 2);
> > +       if (ret == -ENOMEM || ret == -EINTR)
> > +               goto out;
> > +       /*
> > +        * The non-compound revalidate path propagates -ENOMEM / -EINTR
> > +        * from the lookup to VFS instead of treating them as "invalidate
> > +        * this dentry".  Keep that behaviour when the lookup ran inside
> > +        * a compound: surface the per-subop error to the caller.
> > +        */
> > +       if (lookup_err == -ENOMEM || lookup_err == -EINTR) {
> > +               ret = lookup_err;
> > +               goto out;
> > +       }
> > +       if (ret < 0 || lookup_err || !lookup_out.nodeid) {
> > +               ret = 0;
> > +               goto out;
> > +       }
> > +
> > +       fi = get_fuse_inode(inode);
> > +       if (lookup_out.nodeid != get_node_id(inode) ||
> > +           (bool)IS_AUTOMOUNT(inode) != (bool)(lookup_out.attr.flags & FUSE_ATTR_SUBMOUNT)) {
> > +               fuse_queue_forget(fm->fc, forget, lookup_out.nodeid, 1);
> > +               forget = NULL;
> > +               ret = 0;
> > +               goto out;
> > +       }
> > +
> > +       spin_lock(&fi->lock);
> > +       fi->nlookup++;
> > +       spin_unlock(&fi->lock);
> > +
> > +       if (fuse_invalid_attr(&lookup_out.attr) ||
> > +           fuse_stale_inode(inode, lookup_out.generation, &lookup_out.attr)) {
> > +               ret = 0;
> > +               goto out;
> > +       }
> > +
> > +       forget_all_cached_acls(inode);
> > +
> > +       if (!getattr_err && !fuse_invalid_attr(&getattr_out.attr))
> > +               fuse_change_attributes(inode, &getattr_out.attr, NULL,
> > +                                      ATTR_TIMEOUT(&getattr_out),
> > +                                      attr_version);
> > +       else
> > +               fuse_change_attributes(inode, &lookup_out.attr, NULL,
> > +                                      ATTR_TIMEOUT(&lookup_out),
> > +                                      attr_version);
> > +
> > +       fuse_change_entry_timeout(entry, &lookup_out);
> > +       ret = 1;
> > +
> > +out:
> > +       kfree(forget);
> > +       return ret;
> > +}
> > +
> 
> That's duplicating a lot of subtle code.
> I think this calls for some helpers.

OK ... this was a new one (compound I mean), and I valued the compactness of the patch more
than the possible code duplication. You're probably right ...

> 
> Thanks,
> Amir.