On Mon, Apr 29, 2024 at 12:15:55PM +0200, Andrey Albershteyn wrote:
> On 2024-04-24 11:05:20, Darrick J. Wong wrote:
> > On Thu, Apr 04, 2024 at 11:14:07PM -0400, Eric Biggers wrote:
> > > On Fri, Mar 29, 2024 at 05:35:48PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > >
> > > > Now that we've made the verity workqueue per-superblock, we don't need
> > > > the systemwide workqueue. Get rid of the old implementation.
> > >
> > > This commit message needs to be rephrased because this commit isn't just
> > > removing unused code. It's also converting ext4 and f2fs over to the new
> > > workqueue type. (Maybe these two parts belong as separate patches?)
> >
> > Yes, will fix that.
> >
> > > Also, if there are any changes in the workqueue flags that are being used for
> > > ext4 and f2fs, that needs to be documented.
> >
> > Hmm. The current codebase does this:
> >
> > fsverity_read_workqueue = alloc_workqueue("fsverity_read_queue",
> > WQ_HIGHPRI,
> > num_online_cpus());
> >
> > Looking at commit f959325e6ac3 ("fsverity: Remove WQ_UNBOUND from
> > fsverity read workqueue"), I guess you want a bound workqueue so that
> > the CPU that handles the readahead ioend will also handle the verity
> > validation?
> >
> > Why do you set max_active to num_online_cpus()? Is that because the
> > verity hash is (probably?) being computed on the CPUs, and there's only
> > so many of those to go around, so there's little point in making more?
> > Or is it to handle systems with more than WQ_DFL_ACTIVE (~256) CPUs?
> > Maybe there's a different reason?
> >
> > If you add more CPUs to the system later, does this now constrain the
> > number of CPUs that can be participating in verity validation? Why not
> > let the system try to process as many read ioends as are ready to be
> > processed, rather than introducing a constraint here?
> >
> > As for WQ_HIGHPRI, I wish Dave or Andrey would chime in on why this
> > isn't appropriate for XFS. I think they have a reason for this, but the
> > most I can do is speculate that it's to avoid blocking other things in
> > the system.
>
> The log uses WQ_HIGHPRI for journal IO completion
> log->l_ioend_workqueue, as far I understand some data IO completion
> could require a transaction which make a reservation which
> could lead to data IO waiting for journal IO. But if data IO
> completion will be scheduled first this could be a possible
> deadlock... I don't see a particular example, but also I'm not sure
> why to make fs-verity high priority in XFS.
Ah, ok. I'll add a comment about that to my patch then.
> > In Andrey's V5 patch, XFS creates its own the workqueue like this:
> > https://lore.kernel.org/linux-xfs/20240304191046.157464-10-aalbersh@xxxxxxxxxx/
> >
> > struct workqueue_struct *wq = alloc_workqueue(
> > "pread/%s", (WQ_FREEZABLE | WQ_MEM_RECLAIM), 0, sb->s_id);
> >
> > I don't grok this either -- read ioend workqueues aren't usually
> > involved in memory reclaim at all, and I can't see why you'd want to
> > freeze the verity workqueue during suspend. Reads are allowed on frozen
> > filesystems, so I don't see why verity would be any different.
>
> Yeah maybe freezable can go away, initially I picked those flags as
> most of the other workqueues in xfs are in same configuration.
<nod>
--D
> --
> - Andrey
>
>
