Re: [PATCH v2 3/3] pidfs: implement file handle support

[Amir Goldstein <amir73il@xxxxxxxxx>]

On Thu, Nov 14, 2024 at 11:51 PM Erin Shepherd <erin.shepherd@xxxxxx> wrote:
>
> On 14/11/2024 15:13, Christian Brauner wrote:
>
> > On Thu, Nov 14, 2024 at 02:13:06PM +0100, Erin Shepherd wrote:
> >> These two concerns combined with the special flag make me wonder if pidfs
> >> is so much of a special snowflake we should just special case it up front
> >> and skip all of the shared handle decode logic?
> > Care to try a patch and see what it looks like?
>
> The following is a completely untested sketch on top of the existing patch series.
> Some notes:
>
> - I made heavy use of the cleanup macros. I'm happy to convert things back to
>   goto out_xx style if preferred - writing things this way just made bashing out
>   the code without dropping resources on the floor easier

Your cleanup is very welcome, just please! not in the same patch as refactoring
and logic changes. Please do these 3 different things in different commits.
This patch is unreviewable as far as I am concerned.

> - If you don't implement fh_to_dentry then name_to_handle_at will just return an error
>   unless called with AT_HANDLE_FID. We need to decide what to do about that

What's to decide? I did not understand the problem.

> - The GET_PATH_FD_IS_NORMAL/etc constants don't match (what I see as) usual kernel style
>   but I'm not sure how to conventionally express something like that

I believe the conventional way to express a custom operation is an
optional method.

For example:

static int exportfs_get_name(struct vfsmount *mnt, struct dentry *dir,
                char *name, struct dentry *child)
{
        const struct export_operations *nop = dir->d_sb->s_export_op;
        struct path path = {.mnt = mnt, .dentry = dir};

        if (nop->get_name)
                return nop->get_name(dir, name, child);
        else
                return get_name(&path, name, child);
}

There are plenty of optional custom inode, file, sb, dentry
operations with default fallback. some examples:

        if (dir_inode->i_op->atomic_open) {
                dentry = atomic_open(nd, dentry, file, open_flag, mode);

        if (!splice && file_out->f_op->copy_file_range) {
                ret = file_out->f_op->copy_file_range(file_in, pos_in,
                                                      file_out, pos_out,
                                                      len, flags);
        } else if (!splice && file_in->f_op->remap_file_range && samesb) {
                ret = file_in->f_op->remap_file_range(file_in, pos_in,

So I think the right model for you to follow is a custom optional
s_export_op->open_by_handle() operation.

Thanks,
Amir.