Re: [PATCH v6 04/10] fscrypt: Drop d_revalidate once the key is added

[Gabriel Krisman Bertazi <krisman@suse.de>]

Eric Biggers <ebiggers@kernel.org> writes:

> On Wed, Feb 14, 2024 at 04:16:31PM -0800, Eric Biggers wrote:
>> On Mon, Feb 12, 2024 at 09:13:15PM -0500, Gabriel Krisman Bertazi wrote:
>> > From fscrypt perspective, once the key is available, the dentry will
>> > remain valid until evicted for other reasons, since keyed dentries don't
>> > require revalidation and, if the key is removed, the dentry is
>> > forcefully evicted.  Therefore, we don't need to keep revalidating them
>> > repeatedly.
>> > 
>> > Obviously, we can only do this if fscrypt is the only thing requiring
>> > revalidation for a dentry.  For this reason, we only disable
>> > d_revalidate if the .d_revalidate hook is fscrypt_d_revalidate itself.
>> > 
>> > It is safe to do it here because when moving the dentry to the
>> > plain-text version, we are holding the d_lock.  We might race with a
>> > concurrent RCU lookup but this is harmless because, at worst, we will
>> > get an extra d_revalidate on the keyed dentry, which is will find the
>> > dentry is valid.
>> > 
>> > Finally, now that we do more than just clear the DCACHE_NOKEY_NAME in
>> > fscrypt_handle_d_move, skip it entirely for plaintext dentries, to avoid
>> > extra costs.
>> > 
>> > Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
>> 
>> I think this explanation misses an important point, which is that it's only
>> *directories* where a no-key dentry can become the regular dentry.  The VFS does
>> the move because it only allows one dentry to exist per directory.
>> 
>> For nondirectories, the dentries don't get reused and this patch is irrelevant.
>> 
>> (Of course, there's no point in making fscrypt_handle_d_move() check whether the
>> dentry is a directory, since checking DCACHE_NOKEY_NAME is sufficient.)
>> 
>> The diff itself looks good -- thanks.
>> 
>
> Also, do I understand correctly that this patch is a performance optimization,
> not preventing a performance regression?  The similar patch that precedes this
> one, "fscrypt: Drop d_revalidate for valid dentries during lookup", is about
> preventing a performance regression on dentries that aren't no-key.  This patch
> looks deceptively similar, but it only affects no-key directory dentries, which
> we were already doing the fscrypt_d_revalidate for, even after the move to the
> plaintext name.  It's probably still a worthwhile optimization to stop doing the
> fscrypt_d_revalidate when a directory dentry gets moved like that.  But I want
> to make sure I'm correctly understanding each patch.

Hi Eric,

Yes, your understanding is correct. The previous patch prevents the
regression, given that we will install d_revalidate "by default" on all
dentries when fscrypt is enabled. Once that was done, it seemed obvious
to add the optimization to also drop it when the key is added later,
which is what this patch is about.

I'll follow up with a v7 shortly. Just need to find some cycles to work
on it.

thanks,


-- 
Gabriel Krisman Bertazi