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

2024-02-20 Thread Gabriel Krisman Bertazi
Eric Biggers  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(>d_lock);
>>  dentry->d_flags |= DCACHE_NOKEY_NAME;
>>  spin_unlock(>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(>d_lock);
>> +dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
>> +spin_unlock(>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

814ca3e0 :
814ca3e0:   e8 5b b5 c3 ff  call   81105940 
<__fentry__>
814ca3e5:   41 54   push   %r12
814ca3e7:   55  push   %rbp
814ca3e8:   53  push   %rbx
814ca3e9:   48 83 ec 58 sub$0x58,%rsp
814ca3ed:   8b 56 24mov0x24(%rsi),%edx
814ca3f0:   65 48 8b 04 25 28 00mov%gs:0x28,%rax
814ca3f7:   00 00
814ca3f9:   48 89 44 24 50  mov%rax,0x50(%rsp)
814ca3fe:   31 c0   xor%eax,%eax
814ca400:   48 c7 c0 dc ff ff ffmov$0xffdc,%rax
814ca407:   81 fa ff 00 00 00   cmp$0xff,%edx
814ca40d:   76 21   jbe814ca430 

814ca40f:   48 8b 4c 24 50  mov0x50(%rsp),%rcx
814ca414:   65 48 33 0c 25 28 00xor%gs:0x28,%rcx
814ca41b:   00 00
814ca41d:   0f 85 cd 01 00 00   jne814ca5f0 
  <- (__stack_chk_fail)
814ca423:   48 83 c4 58 add$0x58,%rsp
814ca427:   5b  pop%rbx
814ca428:   5d  pop%rbp
814ca429:   41 5c   pop%r12
814ca42b:   e9 70 21 8b 00  jmp81d7c5a0 
<__x86_return_thunk>
814ca430:   48 89 f3mov%rsi,%rbx
814ca433:   89 54 24 20 mov%edx,0x20(%rsp)
814ca437:   48 8d 76 20 lea0x20(%rsi),%rsi
814ca43b:   48 8b 43 28 mov0x28(%rbx),%rax
814ca43f:   48 8d 54 24 10  lea0x10(%rsp),%rdx
814ca444:   48 89 fdmov%rdi,%rbp
814ca447:   48 89 74 24 10  mov%rsi,0x10(%rsp)
814ca44c:   48 89 44 24 18  mov%rax,0x18(%rsp)
814ca451:   e8 ca f0 ff ff  call   814c9520 


[..]

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,

-- 
Gabriel Krisman Bertazi


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


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

2024-02-14 Thread Eric Biggers
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(>d_lock);
>   dentry->d_flags |= DCACHE_NOKEY_NAME;
>   spin_unlock(>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(>d_lock);
> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> + spin_unlock(>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


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel