Re: [PATCH v5 08/24] fsverity: add per-sb workqueue for post read processing

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Mar 04, 2024 at 08:10:31PM +0100, Andrey Albershteyn wrote:
> For XFS, fsverity's global workqueue is not really suitable due to:
> 
> 1. High priority workqueues are used within XFS to ensure that data
>    IO completion cannot stall processing of journal IO completions.
>    Hence using a WQ_HIGHPRI workqueue directly in the user data IO
>    path is a potential filesystem livelock/deadlock vector.
> 
> 2. The fsverity workqueue is global - it creates a cross-filesystem
>    contention point.
> 
> This patch adds per-filesystem, per-cpu workqueue for fsverity
> work. This allows iomap to add verification work in the read path on
> BIO completion.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>

Should ext4 and f2fs switch over to this by converting
fsverity_enqueue_verify_work() to use it?  I'd prefer not to have to maintain
two separate workqueue strategies as part of the fs/verity/ infrastructure.

> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1fbc72c5f112..5863519ffd51 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1223,6 +1223,8 @@ struct super_block {
>  #endif
>  #ifdef CONFIG_FS_VERITY
>  	const struct fsverity_operations *s_vop;
> +	/* Completion queue for post read verification */
> +	struct workqueue_struct *s_read_done_wq;
>  #endif

Maybe s_verity_wq?  Or do you anticipate this being used for other purposes too,
such as fscrypt?  Note that it's behind CONFIG_FS_VERITY and is allocated by an
fsverity_* function, so at least at the moment it doesn't feel very generic.

> diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> index 0973b521ac5a..45b7c613148a 100644
> --- a/include/linux/fsverity.h
> +++ b/include/linux/fsverity.h
> @@ -241,6 +241,22 @@ void fsverity_enqueue_verify_work(struct work_struct *work);
>  void fsverity_invalidate_block(struct inode *inode,
>  		struct fsverity_blockbuf *block);
>  
> +static inline int fsverity_set_ops(struct super_block *sb,
> +				   const struct fsverity_operations *ops)

This doesn't just set the ops, but also allocates a workqueue too.  A better
name for this function might be fsverity_init_sb.

Also this shouldn't really be an inline function.

> +{
> +	sb->s_vop = ops;
> +
> +	/* Create per-sb workqueue for post read bio verification */
> +	struct workqueue_struct *wq = alloc_workqueue(
> +		"pread/%s", (WQ_FREEZABLE | WQ_MEM_RECLAIM), 0, sb->s_id);

"pread" is short for "post read", I guess?  Should it really be this generic?

> +static inline int fsverity_set_ops(struct super_block *sb,
> +				   const struct fsverity_operations *ops)
> +{
> +	return -EOPNOTSUPP;
> +}

I think it would be better to just not have a !CONFIG_FS_VERITY stub for this.

You *could* make it work like fscrypt_set_ops(), which the ubifs folks added,
where it can be called unconditionally if the filesystem has a declaration for
the operations (but not necessarily a definition).  In that case it would need
to return 0, rather than an error.  But I think I prefer just omitting the stub
and having filesystems guard the call to this by CONFIG_FS_VERITY, as you've
already done in XFS.

- Eric