MIME-Version: 1.0
References: <20211223202140.2061101-1-harshads@google.com> <20220111125257.ffdn4gmtvfdkunr2@riteshh-domain>
In-Reply-To: <20220111125257.ffdn4gmtvfdkunr2@riteshh-domain>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Tue, 11 Jan 2022 08:19:50 -0800
Message-ID: <CAD+ocbxw8oYg6RVXViwwwh1zuu4mrOQdJ6p0rT_FuXF=sMGaJA@mail.gmail.com>
Subject: Re: [PATCH v2 0/4] ext4 fast commit API cleanup
To:     riteshh <riteshh@linux.ibm.com>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>,
        "Theodore Y. Ts'o" <tytso@mit.edu>,
        Harshad Shirwadkar <harshads@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hey Ritesh,

Yes, your understanding is correct, this patch series does have a side
effect that the entire file system gets locked before starting a fast
commit. However, this regression is meant to be temporary (mainly to
prevent merging of unnecessary correctness patches). I am working on
another series which fixes this by only locking the inode that is
being committed. That patch should be out shortly.

The reason I hurried this patch series in without the inode locking
patches first was that we had some consistency and file system hanging
issues which needed to be fixed in the right way before the code
becomes more cluttered with temporary correctness fixes which would
eventually get dropped out. The hope is that with these patches, such
fixes wouldn't need to be merged in.

But yeah, I am fully aware of the performance degradation that this
series introduces and you will soon see another patch series that
fixes that issue.

Thanks,
Harshad

On Tue, Jan 11, 2022 at 4:53 AM riteshh <riteshh@linux.ibm.com> wrote:
>
> On 21/12/23 12:21PM, Harshad Shirwadkar wrote:
> > This patch series fixes up fast commit APIs. There are NO on-disk
> > format changes introduced in this series. The main contribution of the
> > series is that it drops fast commit specific transaction APIs and
> > makes fast commits work with journal transaction APIs of JBD2
> > journalling system. With these changes, a fast commit eligible
> > transaction is simply enclosed in calls to "jbd2_journal_start()" and
> > "jbd2_journal_stop()". If the update that is being performed is fast
> > commit ineligible, one must simply call ext4_fc_mark_ineligible()
> > after starting a transaction using "jbd2_journal_start()". The last
> > patch in the series simplifies fast commit stats recording by moving
> > it to a different function.
> >
> > I verified that the patch series introduces no regressions in "quick"
> > and "log" groups when "fast_commit" feature is enabled.
> >
> > Changes from V1:
> > ---------------
> >
> > - In the V1 of the patch series, there's performance regression. With
> >   this patch series, we lock the entire file system from starting any
> >   new handles during (which ensures consistency at the cost of
> >   performance). What we ideally want to do is to lock individual
> >   inodes from starting new updates during a commit. To do so, the V2
> >   of this patch series retains the infrastructure of inode level
> >   transactions (ext4_fc_start/stop_update()). In future (not in this
> >   series), we would build on this infrastructure to lock individual
> >   inodes and drop the file system level locking during the commit path.
>
> Hello Harshad,
>
> Sorry about being so late in the game :(
>
> So from what I understood from your above commit msg is that even the current
> v2 patch series suffers from the same performance regression which is:-
> we lock the filesystem from any starting transaction updates
> (by taking j_barrier or say by calling jbd2_journal_lock_updates()) while
> fast_commit's commit operation is in progress (which happens during a file fsync()).
> This means when fast_commit's commit operation is in progress, then we can't even
> start a new transaction for recording any metadata updates to any inodes of my FS.
>
> Is above understanding correct w.r.t this v2 patch series?
> If yes, then why do we need to lock the full filesystem from starting any
> journal txns? Why can't we let any process starts a new transaction while
> the previous fast_commit's commit operation is in progress?
>
> JBD2 does allow us to do that right? i.e. while a jbd2 commit is in progress,
> a new running transaction can be allocated and all the new metadata updates will
> now be tracked in the new running txn, right?
>
> -ritesh
>
>
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> >
> > Harshad Shirwadkar (4):
> >   ext4: use ext4_journal_start/stop for fast commit transactions
> >   ext4: drop ineligible txn start stop APIs
> >   ext4: simplify updating of fast commit stats
> >   ext4: update fast commit TODOs
> >
> >  fs/ext4/acl.c         |   2 -
> >  fs/ext4/ext4.h        |   7 +-
> >  fs/ext4/extents.c     |   9 +-
> >  fs/ext4/fast_commit.c | 188 ++++++++++++++++--------------------------
> >  fs/ext4/fast_commit.h |  27 +++---
> >  fs/ext4/file.c        |   4 -
> >  fs/ext4/inode.c       |   7 +-
> >  fs/ext4/ioctl.c       |  13 +--
> >  fs/ext4/super.c       |   1 -
> >  fs/jbd2/journal.c     |   2 +
> >  10 files changed, 96 insertions(+), 164 deletions(-)
> >
> > --
> > 2.34.1.307.g9b7440fafd-goog
> >