On Mon, Feb 12, 2024 at 05:58:10PM +0100, Andrey Albershteyn wrote:
> As noted by Dave there are two problems with using fs-verity's
> workqueue in XFS:
>
> 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.
>
> Signed-off-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
> ---
> fs/xfs/xfs_aops.c | 15 +++++++++++++--
> fs/xfs/xfs_linux.h | 1 +
> fs/xfs/xfs_mount.h | 1 +
> fs/xfs/xfs_super.c | 9 +++++++++
> 4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 7a6627404160..70e444c151b2 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -548,19 +548,30 @@ xfs_vm_bmap(
> return iomap_bmap(mapping, block, &xfs_read_iomap_ops);
> }
>
> +static inline struct workqueue_struct *
> +xfs_fsverity_wq(
> + struct address_space *mapping)
> +{
> + if (fsverity_active(mapping->host))
> + return XFS_I(mapping->host)->i_mount->m_postread_workqueue;
> + return NULL;
> +}
> +
> STATIC int
> xfs_vm_read_folio(
> struct file *unused,
> struct folio *folio)
> {
> - return iomap_read_folio(folio, &xfs_read_iomap_ops, NULL);
> + return iomap_read_folio(folio, &xfs_read_iomap_ops,
> + xfs_fsverity_wq(folio->mapping));
> }
>
> STATIC void
> xfs_vm_readahead(
> struct readahead_control *rac)
> {
> - iomap_readahead(rac, &xfs_read_iomap_ops, NULL);
> + iomap_readahead(rac, &xfs_read_iomap_ops,
> + xfs_fsverity_wq(rac->mapping));
> }
Ok, Now I see how this workqueue is specified, I just don't see
anything XFS specific about this, and it adds complexity to the
whole system by making XFS special.
Either the fsverity code provides a per-sb workqueue instance, or
we use the global fsverity workqueue. i.e. the filesystem itself
should not have to supply this, nor should it be plumbed into
generic iomap IO path.
We already do this with direct IO completion to use a
per-superblock workqueue for defering write completions
(sb->s_dio_done_wq), so I think that is what we should be doing
here, too. i.e. a generic per-sb post-read workqueue.
That way iomap_read_bio_alloc() becomes:
+#ifdef CONFIG_FS_VERITY
+ if (fsverity_active(inode)) {
+ bio = bio_alloc_bioset(bdev, nr_vecs, REQ_OP_READ, gfp,
+ &iomap_fsverity_bioset);
+ if (bio) {
+ bio->bi_private = inode->i_sb->i_postread_wq;
+ bio->bi_end_io = iomap_read_fsverity_end_io;
+ }
+ return bio;
+ }
And we no longer need to pass a work queue through the IO stack.
This workqueue can be initialised when we first initialise fsverity
support for the superblock at mount time, and it would be relatively
trivial to convert all the fsverity filesytsems to use this
mechanism, getting rid of the global workqueue altogether.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
