Re: Re: [PATCH v5 1/3] fuse: add compound command to combine multiple requests

[Horst Birthelmer <horst@birthelmer.de>]

Hi Miklos,

thanks for taking the time to look at this!

On Wed, Feb 11, 2026 at 05:13:21PM +0100, Miklos Szeredi wrote:
> On Tue, 10 Feb 2026 at 09:46, Horst Birthelmer <horst@birthelmer.com> wrote:
> 
> > +static char *fuse_compound_build_one_op(struct fuse_conn *fc,
> > +                                        struct fuse_args *op_args,
> > +                                        char *buffer_pos)
> > +{
> > +       struct fuse_in_header *hdr;
> > +       size_t needed_size = sizeof(struct fuse_in_header);
> > +       int j;
> > +
> > +       for (j = 0; j < op_args->in_numargs; j++)
> > +               needed_size += op_args->in_args[j].size;
> > +
> > +       hdr = (struct fuse_in_header *)buffer_pos;
> > +       memset(hdr, 0, sizeof(*hdr));
> > +       hdr->len = needed_size;
> > +       hdr->opcode = op_args->opcode;
> > +       hdr->nodeid = op_args->nodeid;
> 
> hdr->unique is notably missing.
> 
> I don't know.  Maybe just fill it with the index?

OK, will do. Since it was never used in libfuse, I didn't notice.

> 
> > +       hdr->uid = from_kuid(fc->user_ns, current_fsuid());
> > +       hdr->gid = from_kgid(fc->user_ns, current_fsgid());
> 
> uid/gid are not needed except for creation ops, and those need idmap
> to calculate the correct values.  I don't think we want to keep legacy
> behavior of always setting these.
> 
> > +       hdr->pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> 
> This will be the same as the value in the compound header, so it's
> redundant.  That might not be bad, but I feel that we're better off
> setting this to zero and letting the userspace server fetch the pid
> value from the compound header if that's needed.
> 
> > +#define FUSE_MAX_COMPOUND_OPS   16        /* Maximum operations per compound */
> 
> Don't see a good reason to declare this in the API.   More sensible
> would be to negotiate a max_request_size during INIT.
> 

Wouldn't that make for a very complicated implementation of larger compounds.
If a fuse server negotiates something like 2?

> > +
> > +#define FUSE_COMPOUND_SEPARABLE (1<<0)
> > +#define FUSE_COMPOUND_ATOMIC (1<<1)
> 
> What is the meaning of these flags?

FUSE_COMPOUND_SEPARABLE is a hint for the fuse server that the requests are all
complete and there is no need to use the result of one request to complete the input
of another request further down the line.

Think of LOOKUP+MKNOD+OPEN ... that would never be 'separable'.

At the moment I use this flag to signal libfuse that it can decode the compund 
and execute sequencially completely in the lib and just call the separate requests
of the fuse server.

FUSE_COMPOUND_ATOMIC was an idea to hint to the fuse server that the kernel treats
the compound as one atomic request. This can maybe save us some checks for some
compounds.

> 
> > +
> > +/*
> > + * Compound request header
> > + *
> > + * This header is followed by the fuse requests
> > + */
> > +struct fuse_compound_in {
> > +       uint32_t        count;                  /* Number of operations */
> 
> This is redundant, as the sum of the sub-request lengths is equal to
> the compound request length, hence calculating the number of ops is
> trivial.
> 
> > +       uint32_t        flags;                  /* Compound flags */
> > +
> > +       /* Total size of all results.
> > +        * This is needed for preallocating the whole result for all
> > +        * commands in this compound.
> > +        */
> > +       uint32_t        result_size;
> 
> I don't understand why this is needed.  Preallocation by the userspace
> server?  Why is this different from a simple request?
> 

The reason is the implementation in libfuse.
It makes it a lot easier to know what the sum of the result buffers in the kernel is
and preallocate that in libfuse for compounds that will be decoded and handled in the
library.

> > +       uint64_t        reserved;
> > +};
> > +
> > +/*
> > + * Compound response header
> > + *
> > + * This header is followed by complete fuse responses
> > + */
> > +struct fuse_compound_out {
> > +       uint32_t        count;     /* Number of results */
> 
> Again, redundant.
> 
> Thanks,
> Miklos
>