Re: [PATCH 1/9] vfs: add i_op->dentry_open()

[Miklos Szeredi]

On Wed, Mar 13, 2013 at 11:44 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 13 Mar 2013 15:16:25 +0100 Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
>> From: Miklos Szeredi <mszeredi@xxxxxxx>
>>
>> Add a new inode operation i_op->dentry_open().  This is for stacked filesystems
>> that want to return a struct file from a different filesystem.
>>
>> ...
>>
>> +/**
>> + * vfs_open - open the file at the given path
>> + * @path: path to open
>> + * @filp: newly allocated file with f_flag initialized
>> + * @cred: credentials to use
>> + */
>> +int vfs_open(const struct path *path, struct file *filp,
>> +          const struct cred *cred)
>> +{
>> +     struct inode *inode = path->dentry->d_inode;
>> +
>> +     if (inode->i_op->dentry_open)
>> +             return inode->i_op->dentry_open(path->dentry, filp, cred);
>> +     else {
>> +             filp->f_path = *path;
>> +             return do_dentry_open(filp, NULL, cred);
>> +     }
>> +}
>
> This looks like it will add some overhead to dentry_open().  That long
> walk path->dentry->d_inode->i_op->dentry_open, particularly.  Can we do
> anything?  Using filp->f_inode might save a tad.

filp->f_inode is initialized in do_dentry_open.  But we can move that
to callers.

Adding an IOP_DENTRY_OPEN flag should take care of the rest.

> It's unfortunate and a bit ugly that ->dentry_open() and
> do_dentry_open() don't have the same arguments.  One would expect and
> like them to do so, and this difference raises suspicions about design
> warts.

Hmm, the basic reason for the difference is that filesystems should
not have access to the vfsmount part of the path.

Otherwise it would be trivial to make them match up.

>
> If they _did_ have the same signature then we could perhaps populate
> ->dentry_open with do_dentry_open by default and avoid the `if'.

Except there's no way to add a default i_op, AFAIK.

>
> The test of inode->i_op->dentry_open could do with an unlikely(), but
> that won't save us :(
>
>> +EXPORT_SYMBOL(vfs_open);
>
> Did you consider EXPORT_SYMBOL_GPL()?

It is not clear to me what to base that decision on.  Either one is fine by me.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/