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

[Bernd Schubert <bernd@bsbernd.com>]

On 2/11/26 17:13, 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?
> 
>> +       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.
> 
>> +
>> +#define FUSE_COMPOUND_SEPARABLE (1<<0)
>> +#define FUSE_COMPOUND_ATOMIC (1<<1)
> 
> What is the meaning of these flags?
> 
>> +
>> +/*
>> + * 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?

With simple request and a single request per buffer, one can re-use the
existing buffer for the reply in fuse-server

- write: Do the write operation, then store the result into the io-buffer
- read: Copy the relatively small header, store the result into the
io-buffer

- Meta-operations: Same as read


Compound:
- iterate over each compound, store the result into a temporary buffer
- After completion, copy the temporary buffer back into the compound

Which then results in the choice of
- Make the temporary buffer as large as the IO buffer
- Reallocate for each compound, or something like 2x 1st buffer
- Allocate the exact size of what kernel nows the reply is supposed to
be on success

In this specific case I had especially asked to store the result size
into the compound to simplify code in libfuse.

Question, this is a just a uint32_t, does it hurt to leave it? Maybe
with the additional comment like "Optimization, not strictly needed, but
might simplify fuse-server".


Thanks,
Bernd