| 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 |
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
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
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 --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);
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(-)