Received-SPF: pass (google.com: domain of linux-ext4+bounces-1234-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 14 Feb 2024 16:16:31 -0800
From: Eric Biggers <ebiggers@kernel.org>
To: Gabriel Krisman Bertazi <krisman@suse.de>
Cc: viro@zeniv.linux.org.uk, jaegeuk@kernel.org, tytso@mit.edu,
	amir73il@gmail.com, linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fsdevel@vger.kernel.org, brauner@kernel.org
Subject: Re: [PATCH v6 04/10] fscrypt: Drop d_revalidate once the key is added
Message-ID: <20240215001631.GI1638@sol.localdomain>
References: <20240213021321.1804-1-krisman@suse.de>
 <20240213021321.1804-5-krisman@suse.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240213021321.1804-5-krisman@suse.de>

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.

- Eric