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

Eric Biggers
2024-02-15T00:31:47Z

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