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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 13, 2024 at 10:54:39AM -0700, Darrick J. Wong wrote:
> From: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
>  fs/super.c               |    7 +++++++
>  include/linux/fs.h       |    2 ++
>  include/linux/fsverity.h |   22 ++++++++++++++++++++++
>  3 files changed, 31 insertions(+)
> 
> 
> diff --git a/fs/super.c b/fs/super.c
> index d35e85295489..338d86864200 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -642,6 +642,13 @@ void generic_shutdown_super(struct super_block *sb)
>  			sb->s_dio_done_wq = NULL;
>  		}
>  
> +#ifdef CONFIG_FS_VERITY
> +		if (sb->s_read_done_wq) {
> +			destroy_workqueue(sb->s_read_done_wq);
> +			sb->s_read_done_wq = NULL;
> +		}
> +#endif
> +
>  		if (sop->put_super)
>  			sop->put_super(sb);
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ed5966a70495..9db24a825d94 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1221,6 +1221,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
>  #if IS_ENABLED(CONFIG_UNICODE)
>  	struct unicode_map *s_encoding;
> 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)
> +{
> +	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);

Looking at this more closely, why is it that the fsverity_read_queue
is unbound and tagged WQ_HIGHPRI, whereas this one is instead FREEZEABLE
and MEM_RECLAIM and bound?

If it's really feasible to use /one/ workqueue for all the read
post-processing then this ought to be a fs/super.c helper ala
sb_init_dio_done_wq.  That said, from Eric's comments on the v5 thread
about fsverity and fscrypt locking horns over workqueue stalls I'm not
convinced that's true.

--D

> +	if (!wq)
> +		return -ENOMEM;
> +
> +	sb->s_read_done_wq = wq;
> +
> +	return 0;
> +}
> +
>  #else /* !CONFIG_FS_VERITY */
>  
>  static inline struct fsverity_info *fsverity_get_info(const struct inode *inode)
> @@ -318,6 +334,12 @@ static inline void fsverity_enqueue_verify_work(struct work_struct *work)
>  	WARN_ON_ONCE(1);
>  }
>  
> +static inline int fsverity_set_ops(struct super_block *sb,
> +				   const struct fsverity_operations *ops)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  #endif	/* !CONFIG_FS_VERITY */
>  
>  static inline bool fsverity_verify_folio(struct folio *folio)
> 
> 




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux