public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Luis Henriques <luis@igalia.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	 Amir Goldstein <amir73il@gmail.com>,
	 Bernd Schubert <bschubert@ddn.com>,
	 Bernd Schubert <bernd@bsbernd.com>,
	 "Darrick J. Wong" <djwong@kernel.org>,
	 Horst Birthelmer <hbirthelmer@ddn.com>,
	 Kevin Chen <kchen@ddn.com>,
	linux-fsdevel@vger.kernel.org,  linux-kernel@vger.kernel.org,
	 Matt Harvey <mharvey@jumptrading.com>,
	 kernel-dev@igalia.com
Subject: Re: [RFC PATCH v3 6/8] fuse: implementation of lookup_handle+statx compound operation
Date: Thu, 09 Apr 2026 11:46:06 +0100	[thread overview]
Message-ID: <87mrzcza1d.fsf@igalia.com> (raw)
In-Reply-To: <CAJnrk1bHqHQ5b2_HYskiJ125BdDFGHqG0zusbuz+1a2vCz-FfA@mail.gmail.com> (Joanne Koong's message of "Wed, 8 Apr 2026 08:05:38 -0700")

On Wed, Apr 08 2026, Joanne Koong wrote:

> On Wed, Apr 8, 2026 at 3:16 AM Luis Henriques <luis@igalia.com> wrote:
>>
>> On Tue, Apr 07 2026, Joanne Koong wrote:
>>
>> > On Tue, Apr 7, 2026 at 2:20 PM Luis Henriques <luis@igalia.com> wrote:
>> >>
>> >> Hi Joanne,
>> >>
>> >> On Tue, Apr 07 2026, Joanne Koong wrote:
>> >>
>> >> > On Wed, Feb 25, 2026 at 3:25 AM Luis Henriques <luis@igalia.com> wrote:
>> >> >>
>> >> >> The implementation of lookup_handle+statx compound operation extends the
>> >> >> lookup operation so that a file handle is be passed into the kernel.  It
>> >> >> also needs to include an extra inarg, so that the parent directory file
>> >> >> handle can be sent to user-space.  This extra inarg is added as an extension
>> >> >> header to the request.
>> >> >>
>> >> >> By having a separate statx including in a compound operation allows the
>> >> >> attr to be dropped from the lookup_handle request, simplifying the
>> >> >> traditional FUSE lookup operation.
>> >> >>
>> >> >> Signed-off-by: Luis Henriques <luis@igalia.com>
>> >> >> ---
>> >> >>  fs/fuse/dir.c             | 294 +++++++++++++++++++++++++++++++++++---
>> >> >>  fs/fuse/fuse_i.h          |  23 ++-
>> >> >>  fs/fuse/inode.c           |  48 +++++--
>> >> >>  fs/fuse/readdir.c         |   2 +-
>> >> >>  include/uapi/linux/fuse.h |  23 ++-
>> >> >>  5 files changed, 355 insertions(+), 35 deletions(-)
>> >> >>
>> >> >> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>> >> >> index 113583c4efb6..89e6176abe25 100644
>> >> >> --- a/include/uapi/linux/fuse.h
>> >> >> +++ b/include/uapi/linux/fuse.h
>> >> >>
>> >> >>  enum fuse_opcode {
>> >> >> @@ -671,6 +676,8 @@ enum fuse_opcode {
>> >> >>          */
>> >> >>         FUSE_COMPOUND           = 54,
>> >> >>
>> >> >> +       FUSE_LOOKUP_HANDLE      = 55,
>> >> >> +
>> >> >>         /* CUSE specific operations */
>> >> >>         CUSE_INIT               = 4096,
>> >> >>
>> >> >> @@ -707,6 +714,20 @@ struct fuse_entry_out {
>> >> >>         struct fuse_attr attr;
>> >> >>  };
>> >> >>
>> >> >> +struct fuse_entry2_out {
>> >> >> +       uint64_t        nodeid;
>> >> >> +       uint64_t        generation;
>> >> >> +       uint64_t        entry_valid;
>> >> >> +       uint32_t        entry_valid_nsec;
>> >> >> +       uint32_t        flags;
>> >> >> +       uint64_t        spare;
>> >> >> +};
>> >> >
>> >> > Hi Luis,
>> >> >
>> >> > Could you explain why we need a new struct fuse_entry2_out instead of
>> >> > reusing struct fuse_entry_out? From what i see, the only differences
>> >> > between them are that fuse_entry2_out drops attr_valid,
>> >> > attr_valid_nsec, and struct fuse_attr. Is this done so that it saves
>> >> > the ~100 bytes per lookup? Would it be cleaner from an abi perspective
>> >> > to just reuse fuse_entry_out and ignore the attr fields if they're not
>> >> > necessary? The reason I'm asking is because I'm looking at how you're
>> >> > doing the lookup request reply to see if the fuse passthrough stuff
>> >> > for metadata/directory operations can be combined with it. But I'm not
>> >> > fully understanding why fuse_entry2_out is needed here.
>> >> >
>> >> > I'm also a bit confused by why the compound with statx is needed here,
>> >> > could you explain this part? I see the call to fuse_statx_to_attr()
>> >> > after do_lookup_handle_statx(),  but fuse_statx_to_attr() converts the
>> >> > statx reply right back to a struct fuse_attr for inode setup, so if
>> >> > FUSE_LOOKUP_HANDLE returned struct fuse_entry_out instead of struct
>> >> > fuse_entry2_out, doesn't this solve the problem of needing to compound
>> >> > it with a statx or getattr request? I also noticed that the statx part
>> >> > uses FUSE_ROOT_ID as a workaround for the node id because the actual
>> >> > nodeid isn't known yet, this seems like another sign that the
>> >> > attributes stuff should just be part of the lookup response itself
>> >> > rather than a separate operation?
>> >>
>> >> First of all, thanks a lot for looking into this patchset.  Much
>> >> appreciated!
>> >>
>> >> The main reason for swapping the usage of attr by statx is that statx
>> >> includes some attributes that attr does not (e.g. btime).  And since I was
>> >> adding a new FUSE operation, it would be a good time for using statx
>> >> instead.  (Moreover, as new attributes may be added to statx in the
>> >> future, the benefits of using statx could eventually be even greater.)
>> >>
>> >> This was suggested by Miklos here[0], before converting the whole thing to
>> >> use compound commands.  So, I was going to use fuse_statx in the _out args
>> >> for lookup_handle.  However, because the interface was getting a bit
>> >> complex with extra args (and ext headers!), Miklos ended up suggesting[1]
>> >> to remove attr completely from the lookup_handle operation, and use
>> >> compounds instead to have the full functionality.
>> >
>> > Thank you for the context and links, Luis!
>> >
>> > Using fuse_statx over fuse_getattr makes sense to me for the new
>> > FUSE_LOOKUP_HANDLE op but the part I am confused about is why it needs
>> > to be compounded. If we are adding a new struct (struct
>> > fuse_entry2_out) to the abi, why is it not simpler to just make the
>> > new struct something like:
>> >
>> >   struct fuse_entry_handle_out {
>> >       uint64_t    nodeid;
>> >       uint64_t    generation;
>> >       uint64_t    entry_valid;
>> >       uint64_t    attr_valid;
>> >       uint32_t    entry_valid_nsec;
>> >       uint32_t    attr_valid_nsec;
>> >       struct fuse_statx stat;
>> >   };
>> >
>> > and avoid the compound stuff altogether? Is it because that would make
>> > the struct fuse_entry_handle_out too big to be stack-allocated and
>> > would thus have to be heap allocated? But if I'm recalling correctly,
>> > the compound requests path requires heap allocations as well. Although
>> > at that point if we're gonna have to do the heap allocation, then we
>> > might as well just also embed the struct fuse_file_handle inside
>> > struct fuse_entry_handle_out?
>>
>> I'm open to drop the usage of compounds for this, of course.  In fact,
>> during the v2 discussions I suggested here[0] the usage of a similar
>> struct fuse_entry_handle_out.  Using compound commands was a way to try to
>> simplify the interface.  But maybe at that point I was too quick at
>> jumping into the compound commands suggestion.  It may make sense to
>> reevaluate this decision if you think it simplifies things, specially for
>> passthrough.
>>
>> [0] https://lore.kernel.org/all/87zf6nov6c.fsf@wotan.olymp
>>
>> And I agree that the stack allocation is unlikely to be a good argument as
>> I can see that using compound commands can make these stack allocations
>> even worse, as we need to allocate more structs for more operations.
>>
>> Miklos, do you have a opinion on this?
>>
>> Cheers,
>
> Thanks for your flexibiltiy with being willing to re-discuss this. In
> my perspective of it, I like in theory the composability / modularity
> of having dentry resolution/creation be an independent entity from
> attributes but I feel like in practice, these two things are tightly
> intercoupled. This might just be a lack of my VFS in-depth knowledge
> of it, but I don't really see any cases where we would want to
> resolve/create a dentry but not need to know or care about its
> attributes. From what I see in the fuse code, every operation/request
> that returns a struct fuse_entry_out directly uses the attributes
> immediately after through fuse_iget(..., &outarg->attr, ...) or
> fuse_change_attributes(..., &outarg.attr, ...). For passthrough it
> could work just as well with compound requests for forwarding the
> backing id. But imo going through compound requests adds complexity
> and overhead for no benefit but maybe there is something I am missing
> / overlooking here?

I understand your points, and you're probably right -- using compounds for
this case may be overkill.  And I think that there are even more complex
problems to be fixed due to the fact that fuse_entry_handle_out doesn't
include a fuse_statx.  I've been looking into FUSE_{CREATE,TMPFILE}, but
READDIR is likely to be even worse.

Thank you Joanne for reopening this discussion.  Hopefully we'll get more
opinions on this soon, before taking any final decision on whether to
proceed with compound commands or going back to v2 of the patchset.

Cheers,
-- 
Luís

>
> Thanks,
> Joanne
>
>> --
>> Luís
>>
>> > Thanks,
>> > Joanne
>> >
>> >>
>> >> Obviously, I may have misunderstood (or mis-implemented) the suggestions
>> >> that were done.  And hopefully the provided links to the discussion that
>> >> originated this approach will help.
>> >>
>> >> Regarding the usage of FUSE_ROOT_ID as a workaround for the node id, I
>> >> believe this is a more generic problem which will occur in other compound
>> >> commands as well.  If we want to create a new file system object and
>> >> perform some operation with it within the same compound, a similar
>> >> workaround will be required (or some sort of flag in the compound command
>> >> to signal this dependency).
>> >>
>> >> I hope this helped to clarify a bit your questions.
>> >>
>> >> [0] https://lore.kernel.org/all/CAJfpegsoeUH42ZSg_MSEYukbgXOM_83YT8z_sksMj84xPPCMGQ@mail.gmail.com
>> >> [1] https://lore.kernel.org/all/CAJfpegst6oha7-M+8v9cYpk7MR-9k_PZofJ3uzG39DnVoVXMkA@mail.gmail.com
>> >>
>> >> Cheers,
>> >> --
>> >> Luís
>>


  reply	other threads:[~2026-04-09 10:46 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-25 11:24 [RFC PATCH v3 0/8] fuse: LOOKUP_HANDLE operation Luis Henriques
2026-02-25 11:24 ` [RFC PATCH v3 1/8] fuse: simplify fuse_lookup_name() interface Luis Henriques
2026-02-27 15:46   ` Miklos Szeredi
2026-02-28 14:42     ` Luis Henriques
2026-02-25 11:24 ` [RFC PATCH v3 2/8] fuse: export extend_arg() and factor out fuse_ext_size() Luis Henriques
2026-02-25 11:24 ` [RFC PATCH v3 3/8] fuse: store index of the variable length argument Luis Henriques
2026-02-27 15:41   ` Miklos Szeredi
2026-02-28 14:50     ` Luis Henriques
2026-02-25 11:24 ` [RFC PATCH v3 4/8] fuse: drop unnecessary argument from fuse_lookup_init() Luis Henriques
2026-02-27 15:57   ` Miklos Szeredi
2026-02-25 11:24 ` [RFC PATCH v3 5/8] fuse: extract helper functions from fuse_do_statx() Luis Henriques
2026-02-25 11:24 ` [RFC PATCH v3 6/8] fuse: implementation of lookup_handle+statx compound operation Luis Henriques
2026-02-25 18:06   ` Amir Goldstein
2026-02-26  9:54     ` Luis Henriques
2026-02-26 10:08       ` Amir Goldstein
2026-02-26 10:29         ` Miklos Szeredi
2026-02-26 15:06           ` Luis Henriques
2026-02-26 15:44             ` Miklos Szeredi
2026-02-26 16:17               ` Luis Henriques
2026-02-26 10:33         ` Luis Henriques
2026-04-07 17:43   ` Joanne Koong
2026-04-07 21:20     ` Luis Henriques
2026-04-07 23:06       ` Joanne Koong
2026-04-07 23:24         ` Joanne Koong
2026-04-07 23:38           ` Joanne Koong
2026-04-08 10:22           ` Luis Henriques
2026-04-08 15:15             ` Joanne Koong
2026-04-08 10:16         ` Luis Henriques
2026-04-08 15:05           ` Joanne Koong
2026-04-09 10:46             ` Luis Henriques [this message]
2026-04-09  2:27           ` Jingbo Xu
2026-04-09 11:10             ` Luis Henriques
2026-04-09 14:03             ` Amir Goldstein
2026-02-25 11:24 ` [RFC PATCH v3 7/8] fuse: export fuse_open_args_fill() helper function Luis Henriques
2026-02-25 11:24 ` [RFC PATCH v3 8/8] fuse: implementation of mkobj_handle+statx+open compound operation Luis Henriques
2026-02-25 15:08   ` Horst Birthelmer
2026-02-25 17:26     ` Luis Henriques
2026-02-25 15:14 ` [RFC PATCH v3 0/8] fuse: LOOKUP_HANDLE operation Horst Birthelmer
2026-02-25 17:06   ` Luis Henriques

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87mrzcza1d.fsf@igalia.com \
    --to=luis@igalia.com \
    --cc=amir73il@gmail.com \
    --cc=bernd@bsbernd.com \
    --cc=bschubert@ddn.com \
    --cc=djwong@kernel.org \
    --cc=hbirthelmer@ddn.com \
    --cc=joannelkoong@gmail.com \
    --cc=kchen@ddn.com \
    --cc=kernel-dev@igalia.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mharvey@jumptrading.com \
    --cc=miklos@szeredi.hu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox