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

[Eric Biggers <ebiggers@kernel.org>]

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.

- Eric