Message-ID: <87ikf2z4w7.fsf@wotan.olymp>
Date: Sat, 22 Nov 2025 11:24:40 +0000
From: Luis Henriques <luis@...lia.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: Miklos Szeredi <miklos@...redi.hu>,  "Darrick J. Wong"
 <djwong@...nel.org>,  Bernd Schubert <bschubert@....com>,  Kevin Chen
 <kchen@....com>,  Horst Birthelmer <hbirthelmer@....com>,  linux-fsdevel
 <linux-fsdevel@...r.kernel.org>,  linux-kernel
 <linux-kernel@...r.kernel.org>,  Matt Harvey <mharvey@...ptrading.com>,
  kernel-dev@...lia.com
Subject: Re: [RFC PATCH v1 4/3] fuse: implementation of export_operations
 with FUSE_LOOKUP_HANDLE

On Fri, Nov 21 2025, Amir Goldstein wrote:

> [changing the subject to comment on this part separately]
>
> On Thu, Nov 20, 2025 at 11:55 AM Luis Henriques <luis@...lia.com> wrote:
>>
>>
>> The export_operations were also modified to use this new file handle instead
>> if the lookup_handle operation is implemented for the file system.
>>
>> Signed-off-by: Luis Henriques <luis@...lia.com>
> ...
>>
>> +enum {
>> +       HANDLE_TYPE_NODEID      = 0,
>> +       HANDLE_TYPE_HANDLE      = 1,
>> +};
>> +
>>  struct fuse_inode_handle {
>> -       u64 nodeid;
>> -       u32 generation;
>> +       u32 type;
>
> I don't understand the reason for type as it is always categorically
> determined by fc->lookup_handle in this code.
>
>> +       union {
>
> Perhaps not a union, see below...
>
>> +               struct {
>> +                       u64 nodeid;
>> +                       u32 generation;
>> +               };
>> +               struct fuse_file_handle fh;
>
> Feels like this should be struct fuse_file_handle *fh;

Right, I believe this is leftovers from an earlier version I had where the
fh->handle was an array with the max size FUSE_MAX_HANDLE_SZ.  And some of
the mistakes below in the encode/decode are likely to be related :-/

>> +       };
>>  };
>>
>>  static struct dentry *fuse_get_dentry(struct super_block *sb,
>> @@ -1092,7 +1147,7 @@ static struct dentry *fuse_get_dentry(struct super_block *sb,
>>                         goto out_err;
>>                 }
>>
>> -               err = fuse_lookup_name(sb, handle->nodeid, &name, outarg,
>> +               err = fuse_lookup_name(sb, handle->nodeid, NULL, &name, outarg,
>>                                        &inode);
>
> This is a special case of LOOKUP where the parent is unknown.
> Current fuse code does lookup for name "." in "parent" nodeid and servers
> should know how to treat it specially.
>
> I think that for LOOKUP_HANDLE, we can do one of two things.
> Either we always encode the nodeid in NFS exported file handles
> in addition to the server file handle,
> or we skip the ilookup5() optimization and alway send LOOKUP_HANDLE
> to the fuse server with nodeid 0 when the NFS server calls fuse_fh_to_dentry()
> so the server knows to lookup only by file handle.
>
> The latter option sounds more robust, OTOH the ilookup5() "optimization"
> could be quite useful, so not sure it is worth giving up on.

OK, my guess is that the best option is to keep the ilookup5(), and have
to always encode the nodeid.  But you have a lot of information below for
me to digest, so I'll need to spend some more time figuring things out :-)

>>                 kfree(outarg);
>>                 if (err && err != -ENOENT)
>> @@ -1121,13 +1176,42 @@ static struct dentry *fuse_get_dentry(struct super_block *sb,
>>         return ERR_PTR(err);
>>  }
>>
>> +static int fuse_encode_lookup_fh(struct inode *inode, u32 *fh, int *max_len,
>> +                                struct inode *parent)
>> +{
>> +       struct fuse_inode *fi = get_fuse_inode(inode);
>> +       int total_len, len;
>> +
>> +       total_len = len = sizeof(struct fuse_file_handle);
>> +       if (parent)
>> +               total_len *= 2;
>> +
>> +       if (*max_len < total_len)
>> +               return FILEID_INVALID;
>> +
>> +       memcpy(fh, &fi->fh, len);
>> +       if (parent) {
>> +               fi = get_fuse_inode(parent);
>> +               memcpy((fh + len), &fi->fh, len);
>> +       }
>> +
>> +       *max_len = total_len;
>> +
>> +       /* XXX define new fid_type */
>> +       return parent ? FILEID_INO64_GEN_PARENT : FILEID_INO64_GEN;
>> +}
>> +
>
> This is very odd.
> I don't understand what you were trying to do here.
> As far as I can see, you are not treating fuse_inode_handle as a variable
> length struct that it is in the export operations.

As I mentioned above, I believe this is because the code wasn't updated to
the version where the handle is dynamically allocated instead of a static
array.  Sorry, I should have noticed that.

>>  static int fuse_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>>                            struct inode *parent)
>>  {
>> +       struct fuse_conn *fc = get_fuse_conn(inode);
>>         int len = parent ? 6 : 3;
>>         u64 nodeid;
>>         u32 generation;
>>
>> +       if (fc->lookup_handle)
>> +               return fuse_encode_lookup_fh(inode, fh, max_len, parent);
>> +
>>         if (*max_len < len) {
>>                 *max_len = len;
>>                 return  FILEID_INVALID;
>> @@ -1156,30 +1240,51 @@ static int fuse_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>>  static struct dentry *fuse_fh_to_dentry(struct super_block *sb,
>>                 struct fid *fid, int fh_len, int fh_type)
>>  {
>> +       struct fuse_conn *fc = get_fuse_conn_super(sb);
>>         struct fuse_inode_handle handle;
>>
>>         if ((fh_type != FILEID_INO64_GEN &&
>>              fh_type != FILEID_INO64_GEN_PARENT) || fh_len < 3)
>>                 return NULL;
>>
>> -       handle.nodeid = (u64) fid->raw[0] << 32;
>> -       handle.nodeid |= (u64) fid->raw[1];
>> -       handle.generation = fid->raw[2];
>> +       if (fc->lookup_handle) {
>> +               if (fh_len < sizeof(struct fuse_file_handle))
>> +                       return NULL;
>> +               handle.type = HANDLE_TYPE_HANDLE;
>> +               memcpy(&handle.fh, &fid->raw[0],
>> +                      sizeof(struct fuse_file_handle));
>> +       } else {
>> +               handle.nodeid = (u64) fid->raw[0] << 32;
>> +               handle.nodeid |= (u64) fid->raw[1];
>> +               handle.generation = fid->raw[2];
>> +       }
>>         return fuse_get_dentry(sb, &handle);
>>  }
>>
>>  static struct dentry *fuse_fh_to_parent(struct super_block *sb,
>>                 struct fid *fid, int fh_len, int fh_type)
>>  {
>> -       struct fuse_inode_handle parent;
>> +       struct fuse_conn *fc = get_fuse_conn_super(sb);
>> +       struct fuse_inode_handle handle;
>>
>>         if (fh_type != FILEID_INO64_GEN_PARENT || fh_len < 6)
>>                 return NULL;
>>
>> -       parent.nodeid = (u64) fid->raw[3] << 32;
>> -       parent.nodeid |= (u64) fid->raw[4];
>> -       parent.generation = fid->raw[5];
>> -       return fuse_get_dentry(sb, &parent);
>> +       if (fc->lookup_handle) {
>> +               struct fuse_file_handle *fh = (struct fuse_file_handle *)fid->raw;
>> +
>> +               if (fh_len < sizeof(struct fuse_file_handle) * 2)
>> +                       return NULL;
>> +               handle.type = HANDLE_TYPE_HANDLE;
>> +               memcpy(&handle.fh, &fh[1],
>> +                      sizeof(struct fuse_file_handle));
>> +       } else {
>> +               handle.type = HANDLE_TYPE_NODEID;
>> +               handle.nodeid = (u64) fid->raw[3] << 32;
>> +               handle.nodeid |= (u64) fid->raw[4];
>> +               handle.generation = fid->raw[5];
>> +       }
>> +       return fuse_get_dentry(sb, &handle);
>>  }
>>
>
> You may want to look at ovl_encode_fh() as an example of how overlayfs
> encapsulates whatever file handle it got from the real filesystem and packs it
> as type OVL_FILEID_V1 to hand out to the NFS server.
>
> If we go that route, then the FILEID_FUSE type may include the legacy
> nodeid+gen and the server's variable length file handle following that.
>
> To support "connectable" file handles (see AT_HANDLE_CONNECTABLE
> and fstests test generic/777) would need to implement also handle type
> FILEID_FUSE_WITH_PARENT, which is a concatenation of two
> variable sized FILEID_FUSE handles.
>
> But note that the fact that the server supports LOOKUP_HANDLE does not
> mean that all NFS exported handles MUST be FILEID_FUSE handles.
>
> I think we consider allowing the server to reply to LOOKUP_HANDLE without
> a file handle argument (only nodeid+gen) for specific inodes (e.g. the
> root inode).
>
> If we allow that, then those inodes could still be encoded as FILEID_INO64_GEN
> when exported to NFS and when fuse_fh_to_dentry() is requested to decide
> a file handle of type FILEID_INO64_GEN, it may call LOOKUP_HANDLE
> without nodeid and without a file handle and let the fuse server decide if this
> lookup is acceptable (e.g. with FUSE_ROOT_ID) or stale.
>
> One thing that would be useful with the above is that you will not
> have to implement
> encode/decode of FILEID_FUSE for the first version of LOOKUP_HANDLE.
>
> First of all you could require FUSE_NO_EXPORT_SUPPORT for first version,
> but even without it, a fuse server that supports LOOKUP_HANDLE can fail
> LOOKUP_HANDLE requests without a file handle (or FUSE_ROOT_ID) and
> even that will behave better than NFS export of fuse today (*).
>
> Hope I was not piling too much and that I was not piling garbage.

Well, this is definitely a long list of things to consider :-)
And I *really* appreciate the time you took to write them down in an
email.  I may consider using the FUSE_NO_EXPORT_SUPPORT for now until I'm
happy with LOOKUP_HANDLE.  But I will need some time to go through all of
the comments above and eventually come back with questions.  I believe I
understand your points, and I'll start by looking at ovl_encode_fh() to
see how it handles this.

And once again, thanks a lot for you feedback, Amir!

Cheers,
-- 
Luís

> Thanks,
> Amir.
>
> (*) In current fuse, fuse_fh_to_dentry() after fuse was unmounted and mounted
> may return ESTALE if nodeid is not already in inode cache and it may also
> decode the wrong object if after fuse restart nodeid (with same gen)
> was assigned
> to a completely different object (yes that happens).

Powered by blists - more mailing lists