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

[Joanne Koong]

On Thu, Feb 26, 2026 at 8:43 AM Horst Birthelmer <horst@xxxxxxxxxxxxxx> wrote:
>
> From: Horst Birthelmer <hbirthelmer@xxxxxxx>
>
> 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@xxxxxxx>
> ---
>  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?

> +
> +       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.

> +
> +       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.

> +
> +       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?


> +                               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()?

> +               }
> +
> +               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@xxxxxxxxx/

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.

Thanks,
Joanne

>                         }
>                 }
>         }
> @@ -195,11 +275,10 @@ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
>  int fuse_do_open(struct fuse_mount *fm, u64 nodeid, struct file *file,
>                  bool isdir)
>  {
> -       struct fuse_file *ff = fuse_file_open(fm, nodeid, file->f_flags, isdir);
> +       struct fuse_file *ff = fuse_file_open(fm, nodeid, file_inode(file), file->f_flags, isdir);
>
>         if (!IS_ERR(ff))
>                 file->private_data = ff;
> -
>         return PTR_ERR_OR_ZERO(ff);
>  }
>  EXPORT_SYMBOL_GPL(fuse_do_open);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index ff8222b66c4f7b04c0671a980237a43871affd0a..40409a4ab016a061eea20afee76c8a7fe9c15adb 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1588,7 +1588,9 @@ void fuse_file_io_release(struct fuse_file *ff, struct inode *inode);
>
>  /* file.c */
>  struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
> -                                unsigned int open_flags, bool isdir);
> +                                                               struct inode *inode,
> +                                                               unsigned int open_flags,
> +                                                               bool isdir);
>  void fuse_file_release(struct inode *inode, struct fuse_file *ff,
>                        unsigned int open_flags, fl_owner_t id, bool isdir);
>
> diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
> index fdc175e93f74743eb4d2e5a4bc688df1c62e64c4..07a02e47b2c3a68633d213675a8cc380a0cf31d8 100644
> --- a/fs/fuse/ioctl.c
> +++ b/fs/fuse/ioctl.c
> @@ -494,7 +494,7 @@ static struct fuse_file *fuse_priv_ioctl_prepare(struct inode *inode)
>         if (!S_ISREG(inode->i_mode) && !isdir)
>                 return ERR_PTR(-ENOTTY);
>
> -       return fuse_file_open(fm, get_node_id(inode), O_RDONLY, isdir);
> +       return fuse_file_open(fm, get_node_id(inode), NULL, O_RDONLY, isdir);
>  }
>
>  static void fuse_priv_ioctl_cleanup(struct inode *inode, struct fuse_file *ff)
>
> --
> 2.53.0
>