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

[Horst Birthelmer <horst@birthelmer.de>]

Hi Luis,

On Wed, Feb 11, 2026 at 02:59:46PM +0000, Luis Henriques wrote:
> Hi Horst,
> 
> On Tue, Feb 10 2026, Horst Birthelmer wrote:
> 
> > From: Horst Birthelmer <hbirthelmer@ddn.com>
> >
> > For a FUSE_COMPOUND we add a header that contains information
> > about how many commands there are in the compound and about the
> > size of the expected result. This will make the interpretation
> > in libfuse easier, since we can preallocate the whole result.
> > Then we append the requests that belong to this compound.
> >
> > The API for the compound command has:
> >   fuse_compound_alloc()
> >   fuse_compound_add()
> >   fuse_compound_send()
> 
> First of all, thanks a lot for this.  It looks solid to me and in general,
> I'm happy with this interface.  (Note: I haven't yet tested this v5 rev --
> I'll rebase my own code on top of it soon so that I can give it a try).
> 
> Please find below a few comments I have, most of them minor.
> 
> As for the two other patches in this series, they look mostly alright to
> me, though I believe there's some extra clean-up work to be done in the
> error paths for the actual open+getattr implementation.  Is this
> open+getattr code something you'd like to get merged as well, or is it
> just an example on how to implement a compound operation?  If the latter,
> maybe it could be moved somewhere else (Documentation/filesystems/fuse/
> maybe?).

I actually thought that it would get merged in as well since it is a solution
to the problem described here:

https://lore.kernel.org/all/20240813212149.1909627-1-joannelkoong@gmail.com/

> 
> > Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
> > ---
> >  fs/fuse/Makefile          |   2 +-
> >  fs/fuse/compound.c        | 224 ++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/fuse/fuse_i.h          |  11 +++
> >  include/uapi/linux/fuse.h |  40 +++++++++
> >  4 files changed, 276 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
> > index 22ad9538dfc4b80c6d9b52235bdfead6a6567ae4..4c09038ef995d1b9133c2b6871b97b280a4693b0 100644
> > --- a/fs/fuse/Makefile
> > +++ b/fs/fuse/Makefile
> > @@ -11,7 +11,7 @@ obj-$(CONFIG_CUSE) += cuse.o
> >  obj-$(CONFIG_VIRTIO_FS) += virtiofs.o
> >  
> >  fuse-y := trace.o	# put trace.o first so we see ftrace errors sooner
> > -fuse-y += dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o
> > +fuse-y += dev.o dir.o file.o inode.o control.o xattr.o acl.o readdir.o ioctl.o compound.o
> >  fuse-y += iomode.o
> >  fuse-$(CONFIG_FUSE_DAX) += dax.o
> >  fuse-$(CONFIG_FUSE_PASSTHROUGH) += passthrough.o backing.o
> > diff --git a/fs/fuse/compound.c b/fs/fuse/compound.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..1a85209f4e99a0e5e54955bcd951eaf395176c12
> > --- /dev/null
> > +++ b/fs/fuse/compound.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * FUSE: Filesystem in Userspace
> > + * Copyright (C) 2025
> 
> Should this be 2026?  Or 2025-2026, not sure.

neither am I but I added the 2026 in the next version.

> 
> > + *
> > + * Compound operations for FUSE - batch multiple operations into a single
> > + * request to reduce round trips between kernel and userspace.
> > + */
> > +
> > +#include "fuse_i.h"
> > +
> > +/*
> > + * Compound request builder, state tracker, and args pointer storage
> > + */
> > +struct fuse_compound_req {
> > +	struct fuse_mount *fm;
> > +	struct fuse_compound_in compound_header;
> > +	struct fuse_compound_out result_header;
> > +	int op_errors[FUSE_MAX_COMPOUND_OPS];
> > +	struct fuse_args *op_args[FUSE_MAX_COMPOUND_OPS];
> > +};
> > +
> > +struct fuse_compound_req *fuse_compound_alloc(struct fuse_mount *fm, u32 flags)
> > +{
> > +	struct fuse_compound_req *compound;
> > +
> > +	compound = kzalloc(sizeof(*compound), GFP_KERNEL);
> > +	if (!compound)
> > +		return NULL;
> > +
> > +	compound->fm = fm;
> > +	compound->compound_header.flags = flags;
> > +
> > +	return compound;
> > +}
> > +
> > +int fuse_compound_add(struct fuse_compound_req *compound, struct fuse_args *args)
> > +{
> > +	if (!compound ||
> > +	    compound->compound_header.count >= FUSE_MAX_COMPOUND_OPS)
> > +		return -EINVAL;
> > +
> > +	if (args->in_pages)
> > +		return -EINVAL;
> > +
> > +	compound->op_args[compound->compound_header.count] = args;
> > +	compound->compound_header.count++;
> > +	return 0;
> > +}
> > +
> > +static void *fuse_copy_resp_data_per_req(const struct fuse_args *args,
> > +					  char *resp)
> > +{
> > +	const struct fuse_arg *arg;
> > +	int i;
> > +
> > +	for (i = 0; i < args->out_numargs; i++) {
> > +		arg = &args->out_args[i];
> > +		memcpy(arg->value, resp, arg->size);
> > +		resp += arg->size;
> > +	}
> > +
> > +	return resp;
> 
> fuse_copy_resp_data_per_req() should probably be a void function, as it's
> only caller doesn't seem to care about the return.

will be fixed in the next version

> 
> > 
> > +}
> > +
> > +int fuse_compound_get_error(struct fuse_compound_req *compound, int op_idx)
> > +{
> > +	return compound->op_errors[op_idx];
> > +}
> 
> Since this isn't being used in this file, this function could probably be
> moved into fuse_i.h as a static inline instead.
> 

good idea! Will fix in the next version.

> > +
> > +static void *fuse_compound_parse_one_op(struct fuse_compound_req *compound,
> > +					 int op_index, char *response,
> > +					 char *response_end)
> > +{
> > +	struct fuse_out_header *op_hdr = (struct fuse_out_header *)response;
> > +	struct fuse_args *args = compound->op_args[op_index];
> > +
> > +	if (op_hdr->len < sizeof(struct fuse_out_header))
> > +		return NULL;
> > +
> > +	if (response + op_hdr->len > response_end)
> > +		return NULL;
> > +
> > +	if (op_hdr->error)
> > +		compound->op_errors[op_index] = op_hdr->error;
> > +	else
> > +		fuse_copy_resp_data_per_req(args, response +
> > +					    sizeof(struct fuse_out_header));
> > +	/* in case of error, we still need to advance to the next op */
> > +	return response + op_hdr->len;
> > +}
> > +
> > +static int fuse_compound_parse_resp(struct fuse_compound_req *compound,
> > +				     char *response, size_t response_size)
> > +{
> > +	char *response_end = response + response_size;
> > +	int req_count;
> > +	int i;
> > +
> > +	req_count = min(compound->compound_header.count,
> > +			compound->result_header.count);
> > +
> > +	for (i = 0; i < req_count; i++) {
> > +		response = fuse_compound_parse_one_op(compound, i, response,
> > +						      response_end);
> 
> So, we allow the possibility of having some requests without out args.
> But there seems to be something wrong here, right?  What if the first
> request is e.g. a FUSE_UNLINK, which doesn't have an out arg?  In the
> function above we'll be copying the arg into the first request.  Or am I
> misunderstanding the logic?

Good catch ... This I have to think about and come up with a better solution.

> 
> > +		if (!response)
> > +			return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Build a single operation request in the buffer
> > + *
> > + * Returns the new buffer position after writing the operation.
> > + */
> > +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));
> 
> Nit: since the buffer has kmalloc'ed using '__GFP_ZERO', this memset()
> could probably be dropped.

Will change that, if it makes it easier to read.

> 
> > +	hdr->len = needed_size;
> > +	hdr->opcode = op_args->opcode;
> > +	hdr->nodeid = op_args->nodeid;
> > +	hdr->uid = from_kuid(fc->user_ns, current_fsuid());
> > +	hdr->gid = from_kgid(fc->user_ns, current_fsgid());
> > +	hdr->pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> > +	buffer_pos += sizeof(*hdr);
> > +
> > +	for (j = 0; j < op_args->in_numargs; j++) {
> > +		memcpy(buffer_pos, op_args->in_args[j].value,
> > +		       op_args->in_args[j].size);
> > +		buffer_pos += op_args->in_args[j].size;
> > +	}
> > +
> > +	return buffer_pos;
> > +}
> > +
> > +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->compound_header.count;
> > +	size_t total_expected_out_size = 0;
> > +	size_t actual_response_size;
> > +	size_t buffer_size = 0;
> > +	void *resp_payload_buffer;
> > +	char *buffer_pos;
> > +	void *buffer = NULL;
> > +	ssize_t ret;
> > +	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 = kmalloc(buffer_size, GFP_KERNEL | __GFP_ZERO);
> 
> Nit: I find kzalloc() easier to read (and write!).
> 
> > +	if (!buffer)
> > +		return -ENOMEM;
> > +
> > +	buffer_pos = buffer;
> > +	for (i = 0; i < req_count; i++)
> > +		buffer_pos = fuse_compound_build_one_op(fc,
> > +							compound->op_args[i],
> > +							buffer_pos);
> > +
> > +	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 = kmalloc(buffer_size, GFP_KERNEL | __GFP_ZERO);
> 
> Same as above.
> 
> > +	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 out;
> > +
> > +	actual_response_size = args.out_args[1].size;
> > +
> > +	ret = fuse_compound_parse_resp(compound, (char *)resp_payload_buffer,
> > +				       actual_response_size);
> > +out:
> > +	kfree(resp_payload_buffer);
> > +out_free_buffer:
> > +	kfree(buffer);
> > +	return ret;
> > +}
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 7f16049387d15e869db4be23a93605098588eda9..9ebcd96b6b309d75c86a9c716cbd88aaa55c57ef 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -1273,6 +1273,17 @@ static inline ssize_t fuse_simple_idmap_request(struct mnt_idmap *idmap,
> >  int fuse_simple_background(struct fuse_mount *fm, struct fuse_args *args,
> >  			   gfp_t gfp_flags);
> >  
> > +/**
> > + * Compound request API
> > + */
> > +struct fuse_compound_req;
> > +ssize_t fuse_compound_send(struct fuse_compound_req *compound);
> > +
> > +struct fuse_compound_req *fuse_compound_alloc(struct fuse_mount *fm, u32 flags);
> > +int fuse_compound_add(struct fuse_compound_req *compound,
> > +		      struct fuse_args *args);
> > +int fuse_compound_get_error(struct fuse_compound_req *compound, int op_idx);
> > +
> >  /**
> >   * Assign a unique id to a fuse request
> >   */
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index c13e1f9a2f12bd39f535188cb5466688eba42263..113583c4efb67268174dbe4f68e9ea1c21b45eb6 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -664,6 +664,13 @@ enum fuse_opcode {
> >  	FUSE_STATX		= 52,
> >  	FUSE_COPY_FILE_RANGE_64	= 53,
> >  
> > +	/* A compound request is handled like a single request,
> > +	 * but contains multiple requests as input.
> > +	 * This can be used to signal to the fuse server that
> > +	 * the requests can be combined atomically.
> > +	 */
> > +	FUSE_COMPOUND		= 54,
> > +
> >  	/* CUSE specific operations */
> >  	CUSE_INIT		= 4096,
> >  
> > @@ -1245,6 +1252,39 @@ struct fuse_supp_groups {
> >  	uint32_t	groups[];
> >  };
> >  
> > +#define FUSE_MAX_COMPOUND_OPS   16        /* Maximum operations per compound */
> > +
> > +#define FUSE_COMPOUND_SEPARABLE (1<<0)
> > +#define FUSE_COMPOUND_ATOMIC (1<<1)
> 
> Is there a difference between !ATOMIC and SEPARABLE?  I.e can we set both
> flags for the same request?  If there is a difference, I think it should
> be explicitly spelled out here.

I think this will change in the near future.

> 
> Cheers,
> -- 
> Luís
> 
> > +/*
> > + * Compound request header
> > + *
> > + * This header is followed by the fuse requests
> > + */
> > +struct fuse_compound_in {
> > +	uint32_t	count;			/* Number of operations */
> > +	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;
> > +	uint64_t	reserved;
> > +};
> > +
> > +/*
> > + * Compound response header
> > + *
> > + * This header is followed by complete fuse responses
> > + */
> > +struct fuse_compound_out {
> > +	uint32_t	count;     /* Number of results */
> > +	uint32_t	flags;     /* Result flags */
> > +	uint64_t	reserved;
> > +};
> > +
> >  /**
> >   * Size of the ring buffer header
> >   */
> >
> > -- 
> > 2.53.0
> >
>