diff mbox series

[f2fs-dev,v6,03/10] fscrypt: Drop d_revalidate for valid dentries during lookup

Message ID 20240213021321.1804-4-krisman@suse.de (mailing list archive)
State Superseded
Headers show
Series Set casefold/fscrypt dentry operations through sb->s_d_op | expand

Commit Message

Gabriel Krisman Bertazi Feb. 13, 2024, 2:13 a.m. UTC
Unencrypted and encrypted-dentries where the key is available don't need
to be revalidated by fscrypt, since they don't go stale from under VFS
and the key cannot be removed for the encrypted case without evicting
the dentry.  Disable their d_revalidate hook on the first lookup, to
avoid repeated revalidation later. This is done in preparation to always
configuring d_op through sb->s_d_op.

The only part detail is that, since the filesystem might have other
features that require revalidation, we only apply this optimization if
the d_revalidate handler is fscrypt_d_revalidate itself.

Finally, we need to clean the dentry->flags even for unencrypted
dentries, so the ->d_lock might be acquired even for them.  In order to
avoid doing it for filesystems that don't care about fscrypt at all, we
peek ->d_flags without the lock at first, and only acquire it if we
actually need to write the flag.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>

---
changes since v5
 - d_set_always_valid -> d_revalidate (eric)
 - Avoid acquiring the lock for !fscrypt-capable filesystems (eric, Christian)
---
 include/linux/fscrypt.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Eric Biggers Feb. 14, 2024, 11:59 p.m. UTC | #1
On Mon, Feb 12, 2024 at 09:13:14PM -0500, Gabriel Krisman Bertazi wrote:
> Finally, we need to clean the dentry->flags even for unencrypted
> dentries, so the ->d_lock might be acquired even for them.  In order to

might => must?

> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 47567a6a4f9d..d1f17b90c30f 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -951,10 +951,29 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>  static inline void fscrypt_prepare_dentry(struct dentry *dentry,
>  					  bool is_nokey_name)
>  {
> +	/*
> +	 * This code tries to only take ->d_lock when necessary to write
> +	 * to ->d_flags.  We shouldn't be peeking on d_flags for
> +	 * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
> +	 * there is a race, the worst it can happen is that we fail to
> +	 * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
> +	 * d_revalidate.
> +	 */
>  	if (is_nokey_name) {
>  		spin_lock(&dentry->d_lock);
>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
>  		spin_unlock(&dentry->d_lock);
> +	} else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> +		   dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> +		/*
> +		 * Unencrypted dentries and encrypted dentries where the
> +		 * key is available are always valid from fscrypt
> +		 * perspective. Avoid the cost of calling
> +		 * fscrypt_d_revalidate unnecessarily.
> +		 */
> +		spin_lock(&dentry->d_lock);
> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> +		spin_unlock(&dentry->d_lock);
>  	}
>  }

Does this all get optimized out when !CONFIG_FS_ENCRYPTION?

As-is, I don't think the d_revalidate part will be optimized out.

You may need to create a !CONFIG_FS_ENCRYPTION stub explicitly.

- Eric
Gabriel Krisman Bertazi Feb. 20, 2024, 11:03 p.m. UTC | #2
Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Feb 12, 2024 at 09:13:14PM -0500, Gabriel Krisman Bertazi wrote:
>> Finally, we need to clean the dentry->flags even for unencrypted
>> dentries, so the ->d_lock might be acquired even for them.  In order to
>
> might => must?
>
>> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
>> index 47567a6a4f9d..d1f17b90c30f 100644
>> --- a/include/linux/fscrypt.h
>> +++ b/include/linux/fscrypt.h
>> @@ -951,10 +951,29 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
>>  static inline void fscrypt_prepare_dentry(struct dentry *dentry,
>>  					  bool is_nokey_name)
>>  {
>> +	/*
>> +	 * This code tries to only take ->d_lock when necessary to write
>> +	 * to ->d_flags.  We shouldn't be peeking on d_flags for
>> +	 * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
>> +	 * there is a race, the worst it can happen is that we fail to
>> +	 * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
>> +	 * d_revalidate.
>> +	 */
>>  	if (is_nokey_name) {
>>  		spin_lock(&dentry->d_lock);
>>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
>>  		spin_unlock(&dentry->d_lock);
>> +	} else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
>> +		   dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
>> +		/*
>> +		 * Unencrypted dentries and encrypted dentries where the
>> +		 * key is available are always valid from fscrypt
>> +		 * perspective. Avoid the cost of calling
>> +		 * fscrypt_d_revalidate unnecessarily.
>> +		 */
>> +		spin_lock(&dentry->d_lock);
>> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> +		spin_unlock(&dentry->d_lock);
>>  	}
>>  }
>
> Does this all get optimized out when !CONFIG_FS_ENCRYPTION?
>
> As-is, I don't think the d_revalidate part will be optimized out.
>

it seems to get optimized out:

This is ext4_lookup built with CONFIG_FS_ENCRYPTION=n

ffffffff814ca3e0 <ext4_lookup>:
ffffffff814ca3e0:       e8 5b b5 c3 ff          call   ffffffff81105940 <__fentry__>
ffffffff814ca3e5:       41 54                   push   %r12
ffffffff814ca3e7:       55                      push   %rbp
ffffffff814ca3e8:       53                      push   %rbx
ffffffff814ca3e9:       48 83 ec 58             sub    $0x58,%rsp
ffffffff814ca3ed:       8b 56 24                mov    0x24(%rsi),%edx
ffffffff814ca3f0:       65 48 8b 04 25 28 00    mov    %gs:0x28,%rax
ffffffff814ca3f7:       00 00
ffffffff814ca3f9:       48 89 44 24 50          mov    %rax,0x50(%rsp)
ffffffff814ca3fe:       31 c0                   xor    %eax,%eax
ffffffff814ca400:       48 c7 c0 dc ff ff ff    mov    $0xffffffffffffffdc,%rax
ffffffff814ca407:       81 fa ff 00 00 00       cmp    $0xff,%edx
ffffffff814ca40d:       76 21                   jbe    ffffffff814ca430 <ext4_lookup+0x50>
ffffffff814ca40f:       48 8b 4c 24 50          mov    0x50(%rsp),%rcx
ffffffff814ca414:       65 48 33 0c 25 28 00    xor    %gs:0x28,%rcx
ffffffff814ca41b:       00 00
ffffffff814ca41d:       0f 85 cd 01 00 00       jne    ffffffff814ca5f0 <ext4_lookup+0x210>  <- (__stack_chk_fail)
ffffffff814ca423:       48 83 c4 58             add    $0x58,%rsp
ffffffff814ca427:       5b                      pop    %rbx
ffffffff814ca428:       5d                      pop    %rbp
ffffffff814ca429:       41 5c                   pop    %r12
ffffffff814ca42b:       e9 70 21 8b 00          jmp    ffffffff81d7c5a0 <__x86_return_thunk>
ffffffff814ca430:       48 89 f3                mov    %rsi,%rbx
ffffffff814ca433:       89 54 24 20             mov    %edx,0x20(%rsp)
ffffffff814ca437:       48 8d 76 20             lea    0x20(%rsi),%rsi
ffffffff814ca43b:       48 8b 43 28             mov    0x28(%rbx),%rax
ffffffff814ca43f:       48 8d 54 24 10          lea    0x10(%rsp),%rdx
ffffffff814ca444:       48 89 fd                mov    %rdi,%rbp
ffffffff814ca447:       48 89 74 24 10          mov    %rsi,0x10(%rsp)
ffffffff814ca44c:       48 89 44 24 18          mov    %rax,0x18(%rsp)
ffffffff814ca451:       e8 ca f0 ff ff          call   ffffffff814c9520 <ext4_fname_setup_ci_filename>

[..]

I had also confirmed previously that fscrypt_lookup_prepare and
fscrypt_prepare_dentry gets correctly inlined into
ext4_fname_prepare_lookup.


> You may need to create a !CONFIG_FS_ENCRYPTION stub explicitly.

But, in spite of gcc doing the right thing now, fscrypt_prepare_dentry
might grow in the future. So, if you don't mind, I will still add the
stub explicitly, as you suggested.

thanks,
diff mbox series

Patch

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 47567a6a4f9d..d1f17b90c30f 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -951,10 +951,29 @@  static inline int fscrypt_prepare_rename(struct inode *old_dir,
 static inline void fscrypt_prepare_dentry(struct dentry *dentry,
 					  bool is_nokey_name)
 {
+	/*
+	 * This code tries to only take ->d_lock when necessary to write
+	 * to ->d_flags.  We shouldn't be peeking on d_flags for
+	 * DCACHE_OP_REVALIDATE unlocked, but in the unlikely case
+	 * there is a race, the worst it can happen is that we fail to
+	 * unset DCACHE_OP_REVALIDATE and pay the cost of an extra
+	 * d_revalidate.
+	 */
 	if (is_nokey_name) {
 		spin_lock(&dentry->d_lock);
 		dentry->d_flags |= DCACHE_NOKEY_NAME;
 		spin_unlock(&dentry->d_lock);
+	} else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
+		   dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
+		/*
+		 * Unencrypted dentries and encrypted dentries where the
+		 * key is available are always valid from fscrypt
+		 * perspective. Avoid the cost of calling
+		 * fscrypt_d_revalidate unnecessarily.
+		 */
+		spin_lock(&dentry->d_lock);
+		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
+		spin_unlock(&dentry->d_lock);
 	}
 }
 
@@ -992,6 +1011,9 @@  static inline int fscrypt_prepare_lookup(struct inode *dir,
 	fname->usr_fname = &dentry->d_name;
 	fname->disk_name.name = (unsigned char *)dentry->d_name.name;
 	fname->disk_name.len = dentry->d_name.len;
+
+	fscrypt_prepare_dentry(dentry, false);
+
 	return 0;
 }