diff mbox series

[f2fs-dev,v6,04/10] fscrypt: Drop d_revalidate once the key is added

Message ID 20240213021321.1804-5-krisman@suse.de (mailing list archive)
State Superseded
Headers show
Series Set casefold/fscrypt dentry operations through sb->s_d_op | expand

Commit Message

Gabriel Krisman Bertazi Feb. 13, 2024, 2:13 a.m. UTC
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>

---
Changes since v5:
  - Merge with another patch(eric)
  - revert conditional check (eric)
  - drop comment (eric)
Changes since v3:
  - Fix null-ptr-deref for filesystems that don't support fscrypt (ktr)
Changes since v2:
  - Do it when moving instead of when revalidating the dentry. (me)
Changes since v1:
  - Improve commit message (Eric)
  - Drop & in function comparison (Eric)
---
 include/linux/fscrypt.h | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

Comments

Eric Biggers Feb. 15, 2024, 12:16 a.m. UTC | #1
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
Eric Biggers Feb. 15, 2024, 12:31 a.m. UTC | #2
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
Gabriel Krisman Bertazi Feb. 20, 2024, 12:48 a.m. UTC | #3
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,
diff mbox series

Patch

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index d1f17b90c30f..4283997c1bfd 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -192,6 +192,8 @@  struct fscrypt_operations {
 					     unsigned int *num_devs);
 };
 
+int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags);
+
 static inline struct fscrypt_inode_info *
 fscrypt_get_inode_info(const struct inode *inode)
 {
@@ -221,15 +223,29 @@  static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
 }
 
 /*
- * When d_splice_alias() moves a directory's no-key alias to its plaintext alias
- * as a result of the encryption key being added, DCACHE_NOKEY_NAME must be
- * cleared.  Note that we don't have to support arbitrary moves of this flag
- * because fscrypt doesn't allow no-key names to be the source or target of a
- * rename().
+ * When d_splice_alias() moves a directory's no-key alias to its
+ * plaintext alias as a result of the encryption key being added,
+ * DCACHE_NOKEY_NAME must be cleared and there might be an opportunity
+ * to disable d_revalidate.  Note that we don't have to support the
+ * inverse operation because fscrypt doesn't allow no-key names to be
+ * the source or target of a rename().
  */
 static inline void fscrypt_handle_d_move(struct dentry *dentry)
 {
-	dentry->d_flags &= ~DCACHE_NOKEY_NAME;
+	/*
+	 * VFS calls fscrypt_handle_d_move even for non-fscrypt
+	 * filesystems.
+	 */
+	if (dentry->d_flags & DCACHE_NOKEY_NAME) {
+		dentry->d_flags &= ~DCACHE_NOKEY_NAME;
+
+		/*
+		 * Other filesystem features might be handling dentry
+		 * revalidation, in which case it cannot be disabled.
+		 */
+		if (dentry->d_op->d_revalidate == fscrypt_d_revalidate)
+			dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
+	}
 }
 
 /**
@@ -368,7 +384,6 @@  int fscrypt_fname_disk_to_usr(const struct inode *inode,
 bool fscrypt_match_name(const struct fscrypt_name *fname,
 			const u8 *de_name, u32 de_name_len);
 u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name);
-int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags);
 
 /* bio.c */
 bool fscrypt_decrypt_bio(struct bio *bio);