Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Sep 2022 22:07:37 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     "Mohamed Abuelfotoh, Hazem" <abuehaze@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>
Subject: Re: Ext4: Buffered random writes performance regression with
 dioread_nolock enabled
Message-ID: <Yypx6VQRbl3bFP2v@mit.edu>
References: <28460B7B-F66E-4BDC-9F6E-B7E77A3FEE83@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <28460B7B-F66E-4BDC-9F6E-B7E77A3FEE83@amazon.com>
Precedence: bulk

On Mon, Sep 19, 2022 at 03:06:46PM +0000, Mohamed Abuelfotoh, Hazem wrote:
> Hey Team,
> 
> 
>   *   I am sending this e-mail to report a performance regression that’s caused by commit 244adf6426(ext4: make dioread_nolock the default) , I am listing the performance regression symptoms below & our analysis for the reported regression.

Performance regressions are always tricky; dioread_nolock improves on
some workloads, and can cause regressions for others.  In this
particular case, the choice to make it the default was to also fix a
direct I/O vs. writeback race which can result in stale data being
revealed (which is a security issue).

That being said...

1)  as you've noted, this commit has been around since 5.6.

2)  as you noted,

    Increasing the journal size from ext4 128 MiB to 1GiB will also
    fix the problem .

Since 2016, the commit bbd2f78cf63a ("libext2fs: allow the default
journal size to go as large as a gigabyte") has been in e2fsprogs
v1.43.2 and newer (the current version of e2fsprogs v1.46.5; v1.43.2
was released in September 2016, six years ago).  Quoting the commit
description:

    Recent research has shown that for a metadata-heavy workload, a 128 MB
    is journal be a bottleneck on HDD's, and that the optimal journal size
    is proportional to number of unique metadata blocks that can be
    modified (and written into the journal) in a 30 second window.  One
    gigabyte should be sufficient for most workloads, which will be used
    for file systems larger than 128 gigabytes.

So this should not be a problem in practice, and if there are users
who are using antedeluvian versions of e2fsprogs, or who have old file
systems which were created many years ago, it's quite easy to adjust
the journal size.  For example, to adjust the journal to be 2GiB (2048
MiB), just run the commands:

   tune2fs -O ^has_journal /dev/sdXX
   tune2fs -O has_journal -J size=2048 /tmp/sdXX

Hence, I disagree that we should revert commit 244adf6426.  It may be
that for your workload and your file system configuration, using the
mount option nodioread_nolock (or dioread_lock), may make sense.  But
there were also workloads for which using dioread_nolock improved
benchmark numbers, so the question of which is the better default is
not at all obvious.

That being said, I do have plans for a new writeback scheme which will
replace dioread_nolock *and* dioread_lock, and which will hopefully be
faster than either approach.

						- Ted

P.S.  I'm puzzled by your comment, "we have to note that this should
be only beneficial with extent-based files" --- while this is true,
why does this matter?  Unless you're dealing with an ancient file
system that was originally created as ext2 or ext3 and then converted
to ext4, *all* ext4 files should be extent-based...