| Message ID | 20240119184742.31088-5-krisman@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Set casefold/fscrypt dentry operations through sb->s_d_op | expand |
On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote: > /* > * 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(). > */ > static inline void fscrypt_handle_d_move(struct dentry *dentry) > { > dentry->d_flags &= ~DCACHE_NOKEY_NAME; > + > + /* > + * Save the d_revalidate call cost during VFS operations. We > + * can do it because, when the key is available, the dentry > + * can't go stale and the key won't go away without eviction. > + */ > + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate) > + dentry->d_flags &= ~DCACHE_OP_REVALIDATE; > } Is there any way to optimize this further for the case where fscrypt is not being used? This is called unconditionally from d_move(). I've generally been trying to avoid things like this where the fscrypt support slows things down for everyone even when they're not using the feature. In any case, as always please don't let function comments get outdated. Since it now seems to just describe the DCACHE_NOKEY_NAME clearing and not the whole function, maybe it should be moved to above that line. - Eric
Eric Biggers <ebiggers@kernel.org> writes: > On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote: >> /* >> * 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(). >> */ >> static inline void fscrypt_handle_d_move(struct dentry *dentry) >> { >> dentry->d_flags &= ~DCACHE_NOKEY_NAME; >> + >> + /* >> + * Save the d_revalidate call cost during VFS operations. We >> + * can do it because, when the key is available, the dentry >> + * can't go stale and the key won't go away without eviction. >> + */ >> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate) >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE; >> } > > Is there any way to optimize this further for the case where fscrypt is not > being used? This is called unconditionally from d_move(). I've generally been > trying to avoid things like this where the fscrypt support slows things down for > everyone even when they're not using the feature. The problem would be figuring out whether the filesystem has fscrypt enabled. I think we can rely on sb->s_cop always being set: if (sb->s_cop) fscrypt_handle_d_move(dentry); What do you think?
On Thu, Jan 25, 2024 at 05:20:56PM -0300, Gabriel Krisman Bertazi wrote: > Eric Biggers <ebiggers@kernel.org> writes: > > > On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote: > >> /* > >> * 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(). > >> */ > >> static inline void fscrypt_handle_d_move(struct dentry *dentry) > >> { > >> dentry->d_flags &= ~DCACHE_NOKEY_NAME; > >> + > >> + /* > >> + * Save the d_revalidate call cost during VFS operations. We > >> + * can do it because, when the key is available, the dentry > >> + * can't go stale and the key won't go away without eviction. > >> + */ > >> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate) > >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE; > >> } > > > > Is there any way to optimize this further for the case where fscrypt is not > > being used? This is called unconditionally from d_move(). I've generally been > > trying to avoid things like this where the fscrypt support slows things down for > > everyone even when they're not using the feature. > > The problem would be figuring out whether the filesystem has fscrypt > enabled. I think we can rely on sb->s_cop always being set: > > if (sb->s_cop) > fscrypt_handle_d_move(dentry); > > What do you think? That's better, I just wonder if there's an even better way. Do you need to do this for dentries that don't have DCACHE_NOKEY_NAME set? If not, it would be more efficient to test for DCACHE_NOKEY_NAME before clearing the flags. - Eric
Eric Biggers <ebiggers@kernel.org> writes: > On Thu, Jan 25, 2024 at 05:20:56PM -0300, Gabriel Krisman Bertazi wrote: >> Eric Biggers <ebiggers@kernel.org> writes: >> >> > On Fri, Jan 19, 2024 at 03:47:36PM -0300, Gabriel Krisman Bertazi wrote: >> >> /* >> >> * 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(). >> >> */ >> >> static inline void fscrypt_handle_d_move(struct dentry *dentry) >> >> { >> >> dentry->d_flags &= ~DCACHE_NOKEY_NAME; >> >> + >> >> + /* >> >> + * Save the d_revalidate call cost during VFS operations. We >> >> + * can do it because, when the key is available, the dentry >> >> + * can't go stale and the key won't go away without eviction. >> >> + */ >> >> + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate) >> >> + dentry->d_flags &= ~DCACHE_OP_REVALIDATE; >> >> } >> > >> > Is there any way to optimize this further for the case where fscrypt is not >> > being used? This is called unconditionally from d_move(). I've generally been >> > trying to avoid things like this where the fscrypt support slows things down for >> > everyone even when they're not using the feature. >> >> The problem would be figuring out whether the filesystem has fscrypt >> enabled. I think we can rely on sb->s_cop always being set: >> >> if (sb->s_cop) >> fscrypt_handle_d_move(dentry); >> >> What do you think? > > That's better, I just wonder if there's an even better way. Do you need to do > this for dentries that don't have DCACHE_NOKEY_NAME set? If not, it would be > more efficient to test for DCACHE_NOKEY_NAME before clearing the flags. I like that. We don't need it for dentries without DCACHE_NOKEY_NAME, because those dentries have the d_revalidate disabled at lookup-time. I raced my v4 with your comment, but I'll spin a v5 folding in this suggestion shortly.
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index 3801c5c94fb6..0ba3928f3020 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) { @@ -230,6 +232,14 @@ static inline bool fscrypt_needs_contents_encryption(const struct inode *inode) static inline void fscrypt_handle_d_move(struct dentry *dentry) { dentry->d_flags &= ~DCACHE_NOKEY_NAME; + + /* + * Save the d_revalidate call cost during VFS operations. We + * can do it because, when the key is available, the dentry + * can't go stale and the key won't go away without eviction. + */ + if (dentry->d_op && dentry->d_op->d_revalidate == fscrypt_d_revalidate) + dentry->d_flags &= ~DCACHE_OP_REVALIDATE; } /** @@ -368,7 +378,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. Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de> --- 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 | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)