On 22/01/11 08:19AM, harshad shirwadkar wrote: > 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, for taking time and helping me understand the reasoning behind. -ritesh > > Thanks, > Harshad > > On Tue, Jan 11, 2022 at 4:53 AM riteshh <riteshh@xxxxxxxxxxxxx> 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@xxxxxxxxx> > > > > > > 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 > > >
