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.
> 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.
--
- Andrey
