On Mon, Mar 04, 2024 at 05:08:05PM -0800, Eric Biggers wrote:
> 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@xxxxxxxxxx>
>
> 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.
(Agreed.)
> > 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.
Doesn't fscrypt already create its own workqueues?
> > 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.
Yeah.
> > +{
> > + 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?
I think it shouldn't use a term that already means "positioned read" to
userspace.
> > +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.
Aha, I was going to ask why XFS had its own #ifdef guards when this
already exists. :)
--D
> - Eric
>
