Message-ID: <CAJnrk1YGyiWLjx3FF9U4z4ARAiW93mFjsEdCVxoBGGZP3hgXAg@mail.gmail.com>
Date: Wed, 14 Jan 2026 18:40:59 -0800
From: Joanne Koong <joannelkoong@...il.com>
To: Horst Birthelmer <horst@...thelmer.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, Bernd Schubert <bschubert@....com>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, Horst Birthelmer <hbirthelmer@....com>
Subject: Re: [PATCH v4 1/3] fuse: add compound command to combine multiple requests

On Fri, Jan 9, 2026 at 10:27 AM Horst Birthelmer <horst@...thelmer.com> wrote:
>
> From: Horst Birthelmer <hbirthelmer@....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()
>   fuse_compound_free()
>
> Signed-off-by: Horst Birthelmer <hbirthelmer@....com>
> ---
>  fs/fuse/Makefile          |   2 +-
>  fs/fuse/compound.c        | 270 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/fuse_i.h          |  12 +++
>  include/uapi/linux/fuse.h |  37 +++++++
>  4 files changed, 320 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
> index 22ad9538dfc4..4c09038ef995 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 000000000000..0f1e4c073d8b
> --- /dev/null
> +++ b/fs/fuse/compound.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FUSE: Filesystem in Userspace
> + * Copyright (C) 2025
> + *
> + * This file implements compound operations for FUSE, allowing multiple
> + * operations to be batched into a single request to reduce round trips
> + * between kernel and userspace.
> + */
> +
> +#include "fuse_i.h"
> +
> +/*
> + * Compound request builder and 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;
> +
> +       /* Per-operation error codes */
> +       int op_errors[FUSE_MAX_COMPOUND_OPS];
> +       /* Original fuse_args for response parsing */
> +       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 ERR_PTR(-ENOMEM);

imo this logic is cleaner with just returning NULL here and then on
the caller side doing

    compound = fuse_compound_alloc(fm, 0);
    if (!compound)
          return -ENOMEM;

instead of having to do

    if (IS_ERR(compound))
           return PTR_ERR(compound);

and dealing with all the err_ptr/ptr_err stuff

> +
> +       compound->fm = fm;
> +       compound->compound_header.flags = flags;
> +
> +       return compound;
> +}
> +
> +void fuse_compound_free(struct fuse_compound_req *compound)
> +{
> +       if (!compound)
> +               return;

The implementation of kfree (in mm/slub.c) already has a null check

> +
> +       kfree(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_response_data(struct fuse_args *args,
> +                                    char *response_data)
> +{
> +       size_t copied = 0;
> +       int i;
> +
> +       for (i = 0; i < args->out_numargs; i++) {
> +               struct fuse_arg current_arg = args->out_args[i];

struct fuse_arg *current_arg = &args->out_args[i]; instead? that would
avoid the extra stack copy

> +               size_t arg_size;
> +
> +               /*
> +                * Last argument with out_pages: copy to pages
> +                * External payload (in the last out arg) is not supported
> +                * at the moment
> +                */
> +               if (i == args->out_numargs - 1 && args->out_pages)
> +                       return response_data;

imo this should be detected and error-ed out at the
fuse_compound_send() layer or is there some reason we can't do that?

> +
> +               arg_size = current_arg.size;
> +
> +               if (current_arg.value && arg_size > 0) {

If this is part of the args->out_numargs tally, then I think it's
guaranteed here that arg->value is non-NULL and arg->size > 0 (iirc,
the only exception to this is if out_pages is set, then arg->value is
null, but that's already protected against above).

> +                       memcpy(current_arg.value,
> +                              (char *)response_data + copied, arg_size);

Instead of doing "response_data + copied" arithmetic and needing the
copied variable, what about just updating the ptr as we go? i think
that'd look cleaner

> +                       copied += arg_size;
> +               }
> +       }
> +
> +       return (char *)response_data + copied;

is this cast needed? afaict, response_data is already a char *?

> +}
> +
> +int fuse_compound_get_error(struct fuse_compound_req *compound, int op_idx)
> +{
> +       return compound->op_errors[op_idx];
> +}

Hmm, looking at how this gets used by fuse_compound_open_getattr(), it
seems like a more useful api is one that scans through op_errors for
all the op indexes in the compound and returns back the first error it
encounters; then the caller doesn't need to call
fuse_compound_get_error() for every op index in the compound.

> +
> +static void *fuse_compound_parse_one_op(struct fuse_compound_req *compound,
> +                                       int op_index, void *op_out_data,
> +                                       void *response_end)
> +{
> +       struct fuse_out_header *op_hdr = op_out_data;
> +       struct fuse_args *args = compound->op_args[op_index];

op_index is taken straight from "compound->result_header.count" which
can be any value set by the server. I think we need to either add
checking for op_index value here or add checking for that in
fuse_compound_send() before it calls fuse_compound_parse_resp()

> +
> +       if (op_hdr->len < sizeof(struct fuse_out_header))
> +               return NULL;
> +
> +       /* Check if the entire operation response fits in the buffer */
> +       if ((char *)op_out_data + op_hdr->len > (char *)response_end)

Is there a reason this doesn't just define response_end as a char * in
fuse_compound_parse_resp() and pass that as a char * to this function?

> +               return NULL;
> +
> +       if (op_hdr->error != 0)

nit: this could just be "if (op_hdr->error)"

> +               compound->op_errors[op_index] = op_hdr->error;

If this errors out, is the memcpy still needed? or should this just return NULL?

> +
> +       if (args && op_hdr->len > sizeof(struct fuse_out_header))

imo,  args should be checked right after the "struct fuse_args *args =
compound->op_args[op_index];" line in the beginning and if it's null,
t hen fuse_compound_parse_one_op() should return NULL.

> +               return fuse_copy_response_data(args, op_out_data +
> +                                              sizeof(struct fuse_out_header));
> +
> +       /* No response data, just advance past the header */
> +       return (char *)op_out_data + op_hdr->len;
> +}
> +
> +static int fuse_compound_parse_resp(struct fuse_compound_req *compound,
> +                                   u32 count, void *response,
> +                                   size_t response_size)
> +{
> +       void *op_out_data = response;

imo it's cleaner to just use response instead of defining a new
op_out_data variable. And if we change the response arg to char *
response instead of void * response, then that gets rid of a lot of
the char * casting.

> +       void *response_end = (char *)response + response_size;
> +       int i;
> +
> +       if (!response || response_size < sizeof(struct fuse_out_header))
> +               return -EIO;
> +
> +       for (i = 0; i < count && i < compound->result_header.count; i++) {

count is already compound->result_header.count or am I missing something here?

> +               op_out_data = fuse_compound_parse_one_op(compound, i,
> +                                                        op_out_data,
> +                                                        response_end);
> +               if (!op_out_data)
> +                       return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +ssize_t fuse_compound_send(struct fuse_compound_req *compound)
> +{
> +       struct fuse_args args = {
> +               .opcode = FUSE_COMPOUND,
> +               .nodeid = 0,

nit: this line can be removed

> +               .in_numargs = 2,
> +               .out_numargs = 2,
> +               .out_argvar = true,
> +       };
> +       size_t resp_buffer_size;
> +       size_t actual_response_size;
> +       size_t buffer_pos;
> +       size_t total_expected_out_size;
> +       void *buffer = NULL;
> +       void *resp_payload;
> +       ssize_t ret;
> +       int i;
> +
> +       if (!compound) {
> +               pr_info_ratelimited("FUSE: compound request is NULL in %s\n",
> +                                   __func__);
> +               return -EINVAL;
> +       }
> +
> +       if (compound->compound_header.count == 0) {
> +               pr_info_ratelimited("FUSE: compound request contains no operations\n");
> +               return -EINVAL;
> +       }

imo we can get rid of these two checks

> +
> +       buffer_pos = 0;
> +       total_expected_out_size = 0;

Could you move this initialization to the top where the variables get defined?

> +
> +       for (i = 0; i < compound->compound_header.count; i++) {

compound->compound_header.count gets used several times in this
function. i think it's worth declaring this as a separate (and less
verbose :)) variable

> +               struct fuse_args *op_args = compound->op_args[i];
> +               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;
> +
> +               buffer_pos += needed_size;
> +
> +               for (j = 0; j < op_args->out_numargs; j++)
> +                       total_expected_out_size += op_args->out_args[j].size;
> +       }
> +
> +       buffer = kvmalloc(buffer_pos, GFP_KERNEL);

nit: imo it's cleaner to rename resp_buffer_size to buffer_size and
use that variable for this instead of using buffer_pos

imo I don't think we need kvmalloc here or below for the resp buffer
given that compound requests don't support in_pages or out_pages.

> +       if (!buffer)
> +               return -ENOMEM;
> +
> +       buffer_pos = 0;
> +       for (i = 0; i < compound->compound_header.count; i++) {
> +               struct fuse_args *op_args = compound->op_args[i];
> +               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;

don't we already have the op_args->in_args[] needed size from the
computation abvoe? could we just reuse that?

> +
> +               hdr = (struct fuse_in_header *)(buffer + buffer_pos);
> +               memset(hdr, 0, sizeof(*hdr));
> +               hdr->len = needed_size;
> +               hdr->opcode = op_args->opcode;
> +               hdr->nodeid = op_args->nodeid;
> +               hdr->uid = from_kuid(compound->fm->fc->user_ns,
> +                                    current_fsuid());
> +               hdr->gid = from_kgid(compound->fm->fc->user_ns,
> +                                    current_fsgid());
> +               hdr->pid = pid_nr_ns(task_pid(current),
> +                                    compound->fm->fc->pid_ns);

at this point i think it's worth just defining fc at the top of the
function and using that here

> +               buffer_pos += sizeof(*hdr);
> +
> +               for (j = 0; j < op_args->in_numargs; j++) {
> +                       memcpy(buffer + buffer_pos, op_args->in_args[j].value,
> +                              op_args->in_args[j].size);
> +                       buffer_pos += op_args->in_args[j].size;
> +               }
> +       }

imo this would look nicer as a separate helper function

> +
> +       resp_buffer_size = total_expected_out_size +
> +                          (compound->compound_header.count *
> +                           sizeof(struct fuse_out_header));
> +
> +       resp_payload = kvmalloc(resp_buffer_size, GFP_KERNEL | __GFP_ZERO);

kvzalloc()?

> +       if (!resp_payload) {
> +               ret = -ENOMEM;
> +               goto out_free_buffer;
> +       }
> +
> +       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_pos;
> +       args.in_args[1].value = buffer;
> +
> +       args.out_args[0].size = sizeof(compound->result_header);
> +       args.out_args[0].value = &compound->result_header;
> +       args.out_args[1].size = resp_buffer_size;
> +       args.out_args[1].value = resp_payload;
> +
> +       ret = fuse_simple_request(compound->fm, &args);
> +       if (ret < 0)
> +               goto out;
> +
> +       actual_response_size = args.out_args[1].size;

since out_argvar was set for the FUSE_COMPOUND request args, ret is
already args.out_args[1].size (see  __fuse_simple_request())

> +
> +       if (actual_response_size < sizeof(struct fuse_compound_out)) {

can you explain why this checks against size of struct
fuse_compound_out? afaict from the logic in
fuse_compound_parse_resp(), actual_response_size doesn't include the
size of the compound_out struct?

Thanks,
Joanne


> +               pr_info_ratelimited("FUSE: compound response too small (%zu bytes, minimum %zu bytes)\n",
> +                                   actual_response_size,
> +                                   sizeof(struct fuse_compound_out));
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +

Powered by blists - more mailing lists