Re: significant drop fio IOPS performance on v5.10

From: "Theodore Ts'o" <tytso@mit.edu>
To: "Lu, Davina" <davinalu@amazon.com>
Cc: "linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
	"adilger.kernel@dilger.ca" <adilger.kernel@dilger.ca>,
	"regressions@lists.linux.dev" <regressions@lists.linux.dev>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"Mohamed Abuelfotoh, Hazem" <abuehaze@amazon.com>,
	hazem ahmed mohamed <hazem.ahmed.abuelfotoh@gmail.com>,
	"Ritesh Harjani (IBM)" <ritesh.list@gmail.com>,
	"Kiselev, Oleg" <okiselev@amazon.com>,
	"Liu, Frank" <franklmz@amazon.com>
Subject: Re: significant drop  fio IOPS performance on v5.10
Date: Wed, 28 Sep 2022 18:36:23 -0400	[thread overview]
Message-ID: <YzTMZ26AfioIbl27@mit.edu> (raw)
In-Reply-To: <5c819c9d6190452f9b10bb78a72cb47f@amazon.com>

On Wed, Sep 28, 2022 at 06:07:34AM +0000, Lu, Davina wrote:
> 
> Hello,
> 
> My analyzing is that the degradation is introduce by commit
> {244adf6426ee31a83f397b700d964cff12a247d3} and the issue is the
> contention on rsv_conversion_wq.  The simplest option is to increase
> the journal size, but that introduces more operational complexity.
> Another option is to add the following change in attachment "allow
> more ext4-rsv-conversion workqueue.patch"

Hi, thanks for the patch.  However, I don't believe as written it is
safe.  That's because your patch will allow multiple CPU's to run
ext4_flush_completed_IO(), and this function is not set up to be safe
to be run concurrently.  That is, I don't see the necessary locking if
we have two CPU's trying to convert unwritten extents on the same
inode.

It could be made safe, and certainly if the problem is that you are
worried about contention across multiple inodes being written to by
different FIO jobs, then certainly this could be a potential
contention issue.

However, what doesn't make sense is that increasing the journal size
also seems to fix the issue for you.  That implies the problem is one
of the journal being to small, and so you are running into an issue of
stalls caused by the need to do a synchronous checkpoint to clear
space in the journal.  That is a different problem than one of there
being a contention problem with rsv_conversion_wq.

So I want to make sure we understand what you are seeing before we
make such a change.  One potential concern is that will cause a large
number of additional kernel threads.  Now, if these extra kernel
threads are doing useful work, then that's not necessarily an issue.
But if not, then it's going to be burning a large amount of kernel
memory (especially for a system with a large number of CPU cores).

Regards,

					- Ted