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

[Horst Birthelmer]

On Fri, Feb 27, 2026 at 10:45:36AM +0100, Miklos Szeredi wrote:
> On Thu, 26 Feb 2026 at 17:43, Horst Birthelmer <horst@xxxxxxxxxxxxxx> wrote:
> 
> > +int fuse_compound_add(struct fuse_compound_req *compound,
> > +                       struct fuse_args *args,
> > +                       int (*converter)(struct fuse_compound_req *compound,
> > +                       unsigned int index))
> > +{
> > +       if (!compound || compound->count >= compound->max_count)
> > +               return -EINVAL;
> > +
> > +       if (args->in_pages)
> > +               return -EINVAL;
> 
> WARN_ON()
> 
> > +
> > +       compound->op_args[compound->count] = args;
> 
> This could be done *much* simpler with lists.  Just add a 'struct
> list_head list' member to struct fuse_args and pass a 'struct
> list_head *compound' to fuse_compound_add().  No need for
> fuse_compound_alloc/free().
> 
> Alternatively pass a 'void **' to fuse_compound_add(), where the input
> args could be copied directly. This has the advantage of not having to
> keep the args around, so they could be local to the fill function.
> After the request is done the responses would similarly be decoded
> into the outargs.
> 
> Both approaches have advantages and disadvantages, I don't see a clear winner.

Will have another go at this.

> > +       compound->op_converters[compound->count] = converter;
> 
> What are these converters?

This was my way of dealing with the interdependencies.
The automatic sequencialization will call this for every request.
So we can copy and manipulate the args for the next request.
No need for any other flags then. We can provide one or more
of this callback functions and be done.

> 
> > +ssize_t fuse_compound_send(struct fuse_compound_req *compound)
> > +{
> > +       struct fuse_conn *fc = compound->fm->fc;
> > +       struct fuse_args args = {
> > +               .opcode = FUSE_COMPOUND,
> > +               .in_numargs = 2,
> > +               .out_numargs = 2,
> > +               .out_argvar = true,
> > +       };
> > +       unsigned int req_count = compound->count;
> > +       size_t total_expected_out_size = 0;
> > +       size_t buffer_size = 0;
> > +       void *resp_payload_buffer;
> > +       char *buffer_pos;
> > +       void *buffer = NULL;
> > +       ssize_t ret;
> > +       unsigned int i, j;
> > +
> > +       for (i = 0; i < req_count; i++) {
> > +               struct fuse_args *op_args = compound->op_args[i];
> > +               size_t needed_size = sizeof(struct fuse_in_header);
> > +
> > +               for (j = 0; j < op_args->in_numargs; j++)
> > +                       needed_size += op_args->in_args[j].size;
> > +
> > +               buffer_size += needed_size;
> > +
> > +               for (j = 0; j < op_args->out_numargs; j++)
> > +                       total_expected_out_size += op_args->out_args[j].size;
> > +       }
> > +
> > +       buffer = kzalloc(buffer_size, GFP_KERNEL);
> > +       if (!buffer)
> > +               return -ENOMEM;
> > +
> > +       buffer_pos = buffer;
> > +       for (i = 0; i < req_count; i++) {
> > +               if (compound->op_converters[i]) {
> > +                       ret = compound->op_converters[i](compound, i);
> > +                       if (ret < 0)
> > +                               goto out_free_buffer;
> > +               }
> > +
> > +               buffer_pos = fuse_compound_build_one_op(fc,
> > +                                                       compound->op_args[i],
> > +                                                       buffer_pos, i);
> > +       }
> > +
> > +       compound->compound_header.result_size = total_expected_out_size;
> > +
> > +       args.in_args[0].size = sizeof(compound->compound_header);
> > +       args.in_args[0].value = &compound->compound_header;
> > +       args.in_args[1].size = buffer_size;
> > +       args.in_args[1].value = buffer;
> > +
> > +       buffer_size = total_expected_out_size +
> > +                     req_count * sizeof(struct fuse_out_header);
> > +
> > +       resp_payload_buffer = kzalloc(buffer_size, GFP_KERNEL);
> > +       if (!resp_payload_buffer) {
> > +               ret = -ENOMEM;
> > +               goto out_free_buffer;
> > +       }
> > +
> > +       args.out_args[0].size = sizeof(compound->result_header);
> > +       args.out_args[0].value = &compound->result_header;
> > +       args.out_args[1].size = buffer_size;
> > +       args.out_args[1].value = resp_payload_buffer;
> > +
> > +       ret = fuse_simple_request(compound->fm, &args);
> > +       if (ret < 0)
> > +               goto fallback_separate;
> > +
> > +       ret = fuse_handle_compound_results(compound, &args);
> > +       if (ret == 0)
> > +               goto out;
> > +
> > +fallback_separate:
> > +       /* Kernel tries to fallback to separate requests */
> > +       if (!(compound->compound_header.flags & FUSE_COMPOUND_ATOMIC))
> > +               ret = fuse_compound_fallback_separate(compound);
> > +
> > +out:
> > +       kfree(resp_payload_buffer);
> > +out_free_buffer:
> > +       kfree(buffer);
> > +       return ret;
> > +}
> 
> If we go with the list of fuse_args, then all the above logic could go
> into the lower layer (dev.c) which already handles fuse_args ->
> request -> fuse_args conversion.  What's needed is mostly just a loop
> that repeats this for all the sub requests.
> 
> 
> > +struct fuse_compound_req {
> > +       struct fuse_mount *fm;
> > +       struct fuse_compound_in compound_header;
> > +       struct fuse_compound_out result_header;
> > +
> > +       struct fuse_args **op_args;
> > +
> > +       /*
> > +        * Every op can add a converter function to construct the ops args from
> > +        * the already received responses.
> > +        */
> > +       int (**op_converters)(struct fuse_compound_req *compound,
> > +                             unsigned int index);
> > +       int *op_errors;
> 
> Can go into fuse_args.
> 
> > +
> > +       unsigned int max_count;
> > +       unsigned int count;
> > +};
> > +/*
> > + * This is a hint to the fuse server that all requests are complete and it can
> > + * use automatic decoding and sequential processing from libfuse.
> > + */
> > +#define FUSE_COMPOUND_SEPARABLE (1 << 0)
> 
> We really need per sub-request flags, not per-compound flags.
> 
> I.e:
> 
> FUSE_SUB_IS_ENTRY - this sub request will return a new entry on
> success (nodeid, filehandle)
> FUSE_SUB_DEP_ENTRY - this sub request depends on the result of a previous lookup
> 

we don't need this if we use my converters from above.

> > +/*
> > + * This will be used by the kernel to continue on
> > + * even after one of the requests fail.
> > + */
> > +#define FUSE_COMPOUND_CONTINUE (1 << 1)
> 
> Again, I think it makes no sense to have compound-global flags, since
> it might be possible that there are several sub-requests and there are
> different dependencies between for each of them.
> 
> Also if there are no examples of a certain flag in this patchset, then
> it's better to just leave it out and add it together with the actual
> user.

actually there is in compound.c

> 
> > +/*
> > + * This flags the compound as atomic, which
> > + * means that the operation has to be interpreted
> > + * atomically and be directly supported by the fuse server
> > + * itself.
> > + */
> > +#define FUSE_COMPOUND_ATOMIC (1 << 2)
> 
> Why would this be needed?  The VFS provides the locking that ensures
> atomicity, even if the implementation is not atomic.  At least for
> local filesystems that is always the case.
> 

we (by we I mean the fuse server I work on) could use the information that
a certain combination of requests should be atomic.

> > +
> > +/*
> > + * Compound request header
> > + *
> > + * This header is followed by the fuse requests
> > + */
> > +struct fuse_compound_in {
> > +       uint32_t        flags;                  /* Compound flags */
> 
> Not needed.
> 
> > +
> > +       /* Total size of all results expected from the fuse server.
> > +        * This is needed for preallocating the whole result for all
> > +        * commands in the fuse server.
> > +        */
> > +       uint32_t        result_size;
> 
> Please drop this.  I think libfuse can allocate separate buffers for
> each sub-request's out arg and hand a vector of these to the transport
> layer.
> 
> > +       uint64_t        reserved;
> 
> So it turns out the compound header is empty.   Not a problem, just
> make it contain 'uint64_t reserved[2]' for future use.
> 

OK, will do.

> > +};
> > +
> > +/*
> > + * Compound response header
> > + *
> > + * This header is followed by complete fuse responses
> > + */
> > +struct fuse_compound_out {
> > +       uint32_t        flags;     /* Result flags */
> 
> What is this for?
> 

This was used for signalling stuff from the fuse server, like e.g. 
did we actually create something etc.

On second glance ... in the spirit of your minimalization, probably
not needed any more.

> > +       uint32_t        padding;
> > +       uint64_t        reserved;
> > +};
> 
> Thanks,
> Miklos

Overall I like your idea to make compounds really minimal.
There is only the part with the interdependencies that I struggle with, since
almost all examples I tried did not have a very simple methodology.
(LOOKUP+MKNOD+OPEN or Luis CREATE_HANDLE+OPEN)

Could you maybe provide some examples of usecases, that I should try to drill the
new logic?
It feels like you have other compounds in mind than I do.

I have used compounds to send groups of semantically linked requests to the fuse server
signalling to it if the kernel expects it to be one atomic operation or a preferred
'group' of requests (like open+getattr, nothing happens if those are not processed atomic
in a distributed file system)

Thanks for taking the time!
Horst