On 2024-02-16 09:11:43, Dave Chinner wrote:
> 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.
Thanks, haven't thought about that. I will change it.
--
- Andrey
