[+linux-fsdevel]
Thanks! The general concept looks good. A few nits below:
On Tue, May 07, 2024 at 11:36:53AM +0200, Mateusz Guzik wrote:
> fscrypto: try to avoid refing parent dentry in fscrypt_file_open
fscrypt, not fscrypto
> Merely checking if the directory is encrypted happens for every open
> when using ext4, at the moment refing and unrefing the parent, costing 2
> atomics and serializing opens of different files.
>
> The most common case of encryption not being used can be checked for
> with RCU instead.
>
> Sample result from open1_processes -t 20 ("Separate file open/close") from
Overly long line above
> will-it-scale on Sapphire Rapids (ops/s):
> before: 12539898
> after: 25575494 (+103%)
>
> Arguably a vfs helper would be nice here.
>
> Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>
> ---
> fs/crypto/hooks.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
> index 104771c3d3f6..16328ec14266 100644
> --- a/fs/crypto/hooks.c
> +++ b/fs/crypto/hooks.c
> @@ -30,21 +30,32 @@
> int fscrypt_file_open(struct inode *inode, struct file *filp)
> {
> int err;
> - struct dentry *dir;
> + struct dentry *dentry, *dentry_parent;
> + struct inode *inode_parent;
>
> err = fscrypt_require_key(inode);
> if (err)
> return err;
>
> - dir = dget_parent(file_dentry(filp));
> - if (IS_ENCRYPTED(d_inode(dir)) &&
> - !fscrypt_has_permitted_context(d_inode(dir), inode)) {
> + dentry = file_dentry(filp);
> + rcu_read_lock();
> + dentry_parent = READ_ONCE(dentry->d_parent);
> + inode_parent = d_inode_rcu(dentry_parent);
> + if (inode_parent != NULL && !IS_ENCRYPTED(inode_parent)) {
> + rcu_read_unlock();
> + return 0;
> + }
> + rcu_read_unlock();
It would be helpful for there to be a comment here that explains the
optimization. How about something like the following?
/*
* Getting a reference to the parent dentry is needed for the actual
* encryption policy comparison, but it's expensive on multi-core
* systems. Since this function runs on unencrypted files too, start
* with a lightweight RCU-mode check for the parent directory being
* unencrypted (in which case it's fine for the child to be either
* unencrypted, or encrypted with any policy). Only continue on to the
* full policy check if the parent directory is actually encrypted.
*/
dentry = file_dentry(filp);
rcu_read_lock();
...
> +
> + dentry_parent = dget_parent(dentry);
> + if (IS_ENCRYPTED(d_inode(dentry_parent)) &&
> + !fscrypt_has_permitted_context(d_inode(dentry_parent), inode)) {
> fscrypt_warn(inode,
> "Inconsistent encryption context (parent directory: %lu)",
> - d_inode(dir)->i_ino);
> + d_inode(dentry_parent)->i_ino);
> err = -EPERM;
> }
> - dput(dir);
> + dput(dentry_parent);
> return err;
The IS_ENCRYPTED() check above can be removed, because it becomes redundant due
to this patch. I think it was intended to optimize the case of unencrypted
files, like your patch does. But clearly it wasn't too effective, as it was
after the dget_parent().
- Eric
|