Re: [PATCH 03/10] btrfs: offload all write I/O completions to a workqueue

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/3/21 20:55, Christoph Hellwig wrote:
> On Tue, Mar 21, 2023 at 07:37:46AM +0800, Qu Wenruo wrote:
>>> I think it is stalled.  That's why the workqueue heavily discourages
>>> limiting max_active unless you have a good reason to, and most callers
>>> follow that advise.
>>
>> To me, this looks more like hiding a bug in the workqueue user.
>> Shouldn't we expose such bugs instead of hiding them?
> 
> If you limit max_active to a certain value, you clearly tell the
> workqueue code not not use more workers that that.  That is what the
> argument is for.

And if a work load can only be deadlock free using the default 
max_active, but not any value smaller, then I'd say the work load itself 
is buggy.

> 
> I don't see where there is a bug, and that someone is hiding it.
> 
>> Furthermore although I'm a big fan of plain workqueue, the old btrfs
>> workqueues allows us to apply max_active to the plain workqueues, but now
>> we don't have this ability any more.
> 
> You can pass max_active to plain Linux workqueues and there is a bunch
> of places that do that, although the vast majority passes either 0 to
> use the default, or 1 to limit themselves to a single active worker.
> 
> The core also allows to change it, but nothing but the btrfs_workqueue
> code ever does.  We can wire up btrfs to change the conccurency if we
> have a good reason to do.  And I'd like to figure out if there is one,
> and if yes for which workqueue, but for that we'd need to have an
> argument for why which workqueue would like to use a larger or smaller
> than default value.  The old argument of higher resource usage with
> a bigger number of workers does not apply any more with the concurrency
> managed workqueue implementation.

The usecase is still there.
To limit the amount of CPU time spent by btrfs workloads, from csum 
verification to compression.

And if limiting max_active for plain workqueue could help us to expose 
possible deadlocks, it would be extra benefits.

Thanks,
Qu

> 
>>>> Personally speaking, I'd like to keep the btrfs bio endio function calls in
>>>> the old soft/hard irq context, and let the higher layer to queue the work.
>>>
>>> Can you explain why?
>>
>> Just to keep the context consistent.
> 
> Which is what this series does.  Before all read I/O completion handlers
> were called in workqueue context, while write ones weren't.  With the
> series write completion handlers are called from workqueue context
> as well, and with that all significant btrfs code except for tiny bits
> of bi_end_io handlers are called from process context.
> 
>> Image a situation, where we put some sleep functions into a endio function
>> without any doubt, and fully rely on the fact the endio function is
>> executed under workqueue context.
> 
> Being able to do that is the point of this series.