Re: Re: [PATCH v6 3/3] fuse: add an implementation of open+getattr

[Horst Birthelmer <horst@birthelmer.de>]

On Thu, Feb 26, 2026 at 11:12:00AM -0800, Joanne Koong wrote:
> On Thu, Feb 26, 2026 at 8:43 AM Horst Birthelmer <horst@birthelmer.com> wrote:
> >
> > From: Horst Birthelmer <hbirthelmer@ddn.com>
> >
> > The discussion about compound commands in fuse was
> > started over an argument to add a new operation that
> > will open a file and return its attributes in the same operation.
> >
> > Here is a demonstration of that use case with compound commands.
> >
> > Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
> > ---
> >  fs/fuse/file.c   | 111 +++++++++++++++++++++++++++++++++++++++++++++++--------
> >  fs/fuse/fuse_i.h |   4 +-
> >  fs/fuse/ioctl.c  |   2 +-
> >  3 files changed, 99 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index a408a9668abbb361e2c1e386ebab9dfcb0a7a573..daa95a640c311fc393241bdf727e00a2bc714f35 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -136,8 +136,71 @@ static void fuse_file_put(struct fuse_file *ff, bool sync)
> >         }
> >  }
> >
> > +static int fuse_compound_open_getattr(struct fuse_mount *fm, u64 nodeid,
> > +                                     struct inode *inode, int flags, int opcode,
> > +                                     struct fuse_file *ff,
> > +                                     struct fuse_attr_out *outattrp,
> > +                                     struct fuse_open_out *outopenp)
> > +{
> > +       struct fuse_conn *fc = fm->fc;
> > +       struct fuse_compound_req *compound;
> > +       struct fuse_args open_args = {};
> > +       struct fuse_args getattr_args = {};
> > +       struct fuse_open_in open_in = {};
> > +       struct fuse_getattr_in getattr_in = {};
> > +       int err;
> > +
> > +       compound = fuse_compound_alloc(fm, 2, FUSE_COMPOUND_SEPARABLE);
> > +       if (!compound)
> > +               return -ENOMEM;
> > +
> > +       open_in.flags = flags & ~(O_CREAT | O_EXCL | O_NOCTTY);
> > +       if (!fm->fc->atomic_o_trunc)
> > +               open_in.flags &= ~O_TRUNC;
> > +
> > +       if (fm->fc->handle_killpriv_v2 &&
> > +           (open_in.flags & O_TRUNC) && !capable(CAP_FSETID))
> > +               open_in.open_flags |= FUSE_OPEN_KILL_SUIDGID;
> 
> Do you think it makes sense to move this chunk of logic into
> fuse_open_args_fill() since this logic has to be done in
> fuse_send_open() as well?
>

Yes, I think that makes sense and would be beneficial to other requests in 
other compounds that will be constructed with that function.

> > +
> > +       fuse_open_args_fill(&open_args, nodeid, opcode, &open_in, outopenp);
> > +
> > +       err = fuse_compound_add(compound, &open_args, NULL);
> > +       if (err)
> > +               goto out;
> > +
> > +       fuse_getattr_args_fill(&getattr_args, nodeid, &getattr_in, outattrp);
> > +
> > +       err = fuse_compound_add(compound, &getattr_args, NULL);
> > +       if (err)
> > +               goto out;
> > +
> > +       err = fuse_compound_send(compound);
> > +       if (err)
> > +               goto out;
> > +
> > +       err = fuse_compound_get_error(compound, 0);
> > +       if (err)
> > +               goto out;
> > +
> > +       ff->fh = outopenp->fh;
> > +       ff->open_flags = outopenp->open_flags;
> 
> It looks like this logic is shared between here and the non-compound
> open path, maybe a bit better to just do this in fuse_file_open()
> instead? That way we also don't need to pass the struct fuse_file *ff
> as an arg either.
> 

Will do that.

> > +
> > +       err = fuse_compound_get_error(compound, 1);
> > +       if (err)
> > +               goto out;
> 
> For this open+getattr case, if getattr fails but the open succeeds,
> should this still succeed the open since they're separable requests? I
> think we had a conversation about it in v4, but imo this case should.
> 
You are right, we had the conversation and other people joined, so I
changed this code but to something else. Sorry about that.

I think your idea will work, since the behavior then is exactly what happens
at the moment with exactly the same drawback.

> > +
> > +       fuse_change_attributes(inode, &outattrp->attr, NULL,
> > +                              ATTR_TIMEOUT(outattrp),
> > +                              fuse_get_attr_version(fc));
> > +
> > +out:
> > +       fuse_compound_free(compound);
> > +       return err;
> > +}
> > +
> >  struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
> > -                                unsigned int open_flags, bool isdir)
> > +                               struct inode *inode,
> 
> As I understand it, now every open() is a opengetattr() (except for
> the ioctl path) but is this the desired behavior? for example if there
> was a previous FUSE_LOOKUP that was just done, doesn't this mean
> there's no getattr that's needed since the lookup refreshed the attrs?
> or if the server has reasonable entry_valid and attr_valid timeouts,
> multiple opens() of the same file would only need to send FUSE_OPEN
> and not the FUSE_GETATTR, no?

So your concern is, that we send too many requests?
If the fuse server implwments the compound that is not the case.

> 
> 
> > +                               unsigned int open_flags, bool isdir)
> >  {
> >         struct fuse_conn *fc = fm->fc;
> >         struct fuse_file *ff;
> > @@ -163,23 +226,40 @@ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
> >         if (open) {
> >                 /* Store outarg for fuse_finish_open() */
> >                 struct fuse_open_out *outargp = &ff->args->open_outarg;
> > -               int err;
> > +               int err = -ENOSYS;
> >
> > -               err = fuse_send_open(fm, nodeid, open_flags, opcode, outargp);
> > -               if (!err) {
> > -                       ff->fh = outargp->fh;
> > -                       ff->open_flags = outargp->open_flags;
> > -               } else if (err != -ENOSYS) {
> > -                       fuse_file_free(ff);
> > -                       return ERR_PTR(err);
> > -               } else {
> > -                       if (isdir) {
> > +               if (inode) {
> > +                       struct fuse_attr_out attr_outarg;
> > +
> > +                       err = fuse_compound_open_getattr(fm, nodeid, inode,
> > +                                                        open_flags, opcode, ff,
> > +                                                        &attr_outarg, outargp);
> 
> instead of passing in &attr_outarg, what about just having that moved
> to fuse_compound_open_getattr()?
> 

This is a victim of 'code move' already.
I had the code to handle the outarg here before and did not change the functions
signature, which now looks stupid.

> > +               }
> > +
> > +               if (err == -ENOSYS) {
> > +                       err = fuse_send_open(fm, nodeid, open_flags, opcode,
> > +                                            outargp);
> > +                       if (!err) {
> > +                               ff->fh = outargp->fh;
> > +                               ff->open_flags = outargp->open_flags;
> > +                       }
> > +               }
> > +
> > +               if (err) {
> > +                       if (err != -ENOSYS) {
> > +                               /* err is not ENOSYS */
> > +                               fuse_file_free(ff);
> > +                               return ERR_PTR(err);
> > +                       } else {
> >                                 /* No release needed */
> >                                 kfree(ff->args);
> >                                 ff->args = NULL;
> > -                               fc->no_opendir = 1;
> > -                       } else {
> > -                               fc->no_open = 1;
> > +
> > +                               /* we don't have open */
> > +                               if (isdir)
> > +                                       fc->no_opendir = 1;
> > +                               else
> > +                                       fc->no_open = 1;
> 
> kfree(ff->args) and ff->args = NULL should not be called for the
> !isdir case or it leads to the deadlock that was fixed in
> https://lore.kernel.org/linux-fsdevel/20251010220738.3674538-2-joannelkoong@gmail.com/
> 
> I think if you have the "ff->fh = outargp..." and "ff->open_flags =
> ..." logic shared between fuse_compound_open_getattr() and
> fuse_send_open() then the original errorr handling for this could just
> be left as-is.

Very good argument to share the error handling then ...

> 
> Thanks,
> Joanne
> 

Thanks for taking the time,
Horst