From: Andrey Albershteyn <aalbersh@redhat.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: ebiggers@kernel.org, linux-fsdevel@vger.kernel.org,
fsverity@lists.linux.dev, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 08/29] fsverity: add per-sb workqueue for post read processing
Date: Wed, 20 Mar 2024 17:22:23 +0100 [thread overview]
Message-ID: <tuidl6b6miayxekeqfpdgegvxyisqjryf5zvgnz5dfbqab5def@cnks2upytvto> (raw)
In-Reply-To: <20240320145504.GY1927156@frogsfrogsfrogs>
On 2024-03-20 07:55:04, Darrick J. Wong wrote:
> On Wed, Mar 20, 2024 at 11:37:28AM +0100, Andrey Albershteyn wrote:
> > On 2024-03-19 16:30:10, Darrick J. Wong wrote:
> > > On Wed, Mar 13, 2024 at 10:54:39AM -0700, Darrick J. Wong wrote:
> > > > From: Andrey Albershteyn <aalbersh@redhat.com>
> > > >
> > > > 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>
> > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > > 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.
> >
> > There's good explanation by Dave why WQ_HIGHPRI is not a good fit
> > for XFS (potential livelock/deadlock):
> >
> > https://lore.kernel.org/linux-xfs/20221214054357.GI3600936@dread.disaster.area/
> >
> > Based on his feedback I changed it to per-filesystem.
>
> Ah, ok. Why is the workqueue tagged with MEM_RECLAIM though? Does
> letting it run actually help out with reclaim? I guess it does by
> allowing pages involved in readahead to get to unlocked state where they
> can be ripped out. :)
Not sure how much it actually helps with reclaims, leaving it out
would probably have the same effect in most cases. But I suppose at
least one reserved execution context is good thing to not block BIO
finalization.
--
- Andrey
next prev parent reply other threads:[~2024-03-20 16:22 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-13 17:52 [PATCHSET v5.2] fs-verity support for XFS Darrick J. Wong
2024-03-13 17:52 ` [PATCH 01/29] fsverity: remove hash page spin lock Darrick J. Wong
2024-03-13 17:53 ` [PATCH 02/29] xfs: add parent pointer support to attribute code Darrick J. Wong
2024-03-13 17:53 ` [PATCH 03/29] xfs: define parent pointer ondisk extended attribute format Darrick J. Wong
2024-03-13 17:53 ` [PATCH 04/29] xfs: add parent pointer validator functions Darrick J. Wong
2024-03-13 17:53 ` [PATCH 05/29] fs: add FS_XFLAG_VERITY for verity files Darrick J. Wong
2024-03-13 17:54 ` [PATCH 06/29] fsverity: pass tree_blocksize to end_enable_verity() Darrick J. Wong
2024-03-13 17:54 ` [PATCH 07/29] fsverity: support block-based Merkle tree caching Darrick J. Wong
2024-03-13 17:54 ` [PATCH 08/29] fsverity: add per-sb workqueue for post read processing Darrick J. Wong
2024-03-19 23:30 ` Darrick J. Wong
2024-03-20 10:37 ` Andrey Albershteyn
2024-03-20 14:55 ` Darrick J. Wong
2024-03-20 16:22 ` Andrey Albershteyn [this message]
2024-03-13 17:54 ` [PATCH 09/29] fsverity: add tracepoints Darrick J. Wong
2024-03-13 17:55 ` [PATCH 10/29] fsverity: fix "support block-based Merkle tree caching" Darrick J. Wong
2024-03-13 17:55 ` [PATCH 11/29] fsverity: send the level of the merkle tree block to ->read_merkle_tree_block Darrick J. Wong
2024-03-13 17:55 ` [PATCH 12/29] fsverity: pass the new tree size and block size to ->begin_enable_verity Darrick J. Wong
2024-03-13 17:55 ` [PATCH 13/29] iomap: integrate fs-verity verification into iomap's read path Darrick J. Wong
2024-03-13 17:56 ` [PATCH 14/29] xfs: add attribute type for fs-verity Darrick J. Wong
2024-03-13 17:56 ` [PATCH 15/29] xfs: add fs-verity ro-compat flag Darrick J. Wong
2024-03-13 17:56 ` [PATCH 16/29] xfs: add inode on-disk VERITY flag Darrick J. Wong
2024-03-13 17:57 ` [PATCH 17/29] xfs: initialize fs-verity on file open and cleanup on inode destruction Darrick J. Wong
2024-03-13 17:57 ` [PATCH 18/29] xfs: don't allow to enable DAX on fs-verity sealed inode Darrick J. Wong
2024-03-13 17:57 ` [PATCH 19/29] xfs: disable direct read path for fs-verity files Darrick J. Wong
2024-03-13 17:57 ` [PATCH 20/29] xfs: widen flags argument to the xfs_iflags_* helpers Darrick J. Wong
2024-03-13 17:58 ` [PATCH 21/29] xfs: add fs-verity support Darrick J. Wong
2024-03-14 17:06 ` Darrick J. Wong
2024-03-14 17:16 ` Andrey Albershteyn
2024-03-15 2:59 ` Darrick J. Wong
2024-03-13 17:58 ` [PATCH 22/29] xfs: create a per-mount shrinker for verity inodes merkle tree blocks Darrick J. Wong
2024-03-13 17:58 ` [PATCH 23/29] xfs: create an icache tag for files with cached " Darrick J. Wong
2024-03-13 17:58 ` [PATCH 24/29] xfs: shrink verity blob cache Darrick J. Wong
2024-03-13 17:59 ` [PATCH 25/29] xfs: clean up stale fsverity metadata before starting Darrick J. Wong
2024-03-13 17:59 ` [PATCH 26/29] xfs: better reporting and error handling in xfs_drop_merkle_tree Darrick J. Wong
2024-03-13 17:59 ` [PATCH 27/29] xfs: make scrub aware of verity dinode flag Darrick J. Wong
2024-03-13 17:59 ` [PATCH 28/29] xfs: add fs-verity ioctls Darrick J. Wong
2024-03-13 18:00 ` [PATCH 29/29] xfs: enable ro-compat fs-verity flag Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=tuidl6b6miayxekeqfpdgegvxyisqjryf5zvgnz5dfbqab5def@cnks2upytvto \
--to=aalbersh@redhat.com \
--cc=djwong@kernel.org \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).