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

From: Chris Mason <clm@meta.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Qu Wenruo <quwenruo.btrfs@gmx.com>, Chris Mason <clm@fb.com>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>,
	Johannes Thumshirn <jth@kernel.org>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 03/10] btrfs: offload all write I/O completions to a workqueue
Date: Fri, 24 Mar 2023 09:25:07 -0400	[thread overview]
Message-ID: <14e253bb-8530-af11-7395-9e4148249c54@meta.com> (raw)
In-Reply-To: <20230324010959.GB12152@lst.de>

On 3/23/23 9:09 PM, Christoph Hellwig wrote:
> On Thu, Mar 23, 2023 at 10:53:16AM -0400, Chris Mason wrote:
>> The original reason for limiting the number of workers was that work
>> like crc calculations was causing tons of context switches.  This isn't
>> a surprise, there were a ton of work items, and each one was processed
>> very quickly.
> 
> Yeah.  Although quite a bit of that went away since, by not doing
> the offload for sync I/O, not for metadata when there is a fast crc32
> implementation (we need to do the same for small data I/O and hardware
> accelerated sha256, btw), and by doing these in larger chunks, but ..
> 
>>
>> So the original btrfs thread pools had optimizations meant to limit
>> context switches by preferring to give work to a smaller number of workers.
> 
> .. this is something that the workqueue code already does under the hood
> anyway.

Yeah, between hardware changes and kernel changes, these motivations
don't really apply anymore.  Even if they did, we're probably better off
fixing that in the workqueue code directly now.

> 
>> For compression it's more clear cut.  I wanted the ability to saturate
>> all the CPUs with compression work, but also wanted a default that
>> didn't take over the whole machine.
> 
> And that's actually a very good use case.
> 
> It almost asks for a separate option just for compression, or at least
> for compression and checksumming only.
> 
> Is there consensus that for now we should limit thread_pool for 
> se workqueues that do compression and chekcsumming for now?  Then
> I'll send a series to wire it up for the read completion workqueue
> again that does the checksum verification, the compressed write
> workqueue, but drop it for all others but the write submission
> one?

As you mentioned above, we're currently doing synchronous crcs for
metadata when BTRFS_FS_CSUM_IMPL_FAST, and for synchronous writes.
We've benchmarked this a few times and I think with modern hardware a
better default is just always doing synchronous crcs for data too.

Qu or David have you looked synchronous data crcs recently?

Looking ahead, encryption is going to want similar knobs to compression.

My preference would be:

- crcs are always inline if your hardware is fast
- Compression, encryption, slow hardware crcs use the thread_pool_size knob
- We don't try to limit the other workers

The idea behind the knob is just "how much of your CPU should each btrfs
mount consume?"  Obviously we'll silently judge anyone that chooses less
than 100% but I guess it's good to give people options.

-chris