Skip to contentSkip to conversation

ext4 fast commit API cleanup

PATCH Applied Harshad Shirwadkar 3 participants 9 messages Dec 23, 2021 – Jan 11, 2022

Subsystems (2)
ext4 file system Journalling layer for block devices (JBD2)
lore
Share & tools
lore

Harshad Shirwadkar

author
#0 lore raw

v2 cover ext4 fast commit API cleanup

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.

Signed-off-by Harshad Shirwadkar <[email protected]>

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(-)
Signature · 1 line
2.34.1.307.g9b7440fafd-goog
author
#1 lore raw

PATCH v2 1/4 ext4: use ext4_journal_start/stop for fast commit transactions

From: Harshad Shirwadkar <[email protected]>

This patch drops all calls to ext4_fc_start_update() and ext4_fc_stop_update(). To ensure that there are no ongoing journal updates during fast commit, we also make jbd2_fc_begin_commit() lock journal for updates. This way we don't have to maintain two different transaction start stop APIs for fast commit and full commit. This patch doesn't remove the functions altogether since in future we want to have inode level locking for fast commits.

Signed-off-by Harshad Shirwadkar <[email protected]>
---
 fs/ext4/acl.c     |  2 --
 fs/ext4/extents.c |  3 ---
 fs/ext4/file.c    |  4 ----
 fs/ext4/inode.c   |  7 +------
 fs/ext4/ioctl.c   | 10 +---------
 fs/jbd2/journal.c |  2 ++
 6 files changed, 4 insertions(+), 24 deletions(-)
fs/ext4/acl.c +0 -2
index 0613dfcbfd4a..5a35768d6149 100644
@@ -246,7 +246,6 @@ ext4_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
-	ext4_fc_start_update(inode);
 
 	if ((type == ACL_TYPE_ACCESS) && acl) {
 		error = posix_acl_update_mode(mnt_userns, inode, &mode, &acl);

@@ -264,7 +263,6 @@ ext4_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 	}
 out_stop:
 	ext4_journal_stop(handle);
-	ext4_fc_stop_update(inode);
 	if (error == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry;
 	return error;
fs/ext4/extents.c +0 -3
index 0ecf819bf189..703feff8cb8c 100644
@@ -4697,8 +4697,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		     FALLOC_FL_INSERT_RANGE))
 		return -EOPNOTSUPP;
 
-	ext4_fc_start_update(inode);
-
 	if (mode & FALLOC_FL_PUNCH_HOLE) {
 		ret = ext4_punch_hole(inode, offset, len);
 		goto exit;

@@ -4762,7 +4760,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	inode_unlock(inode);
 	trace_ext4_fallocate_exit(inode, offset, max_blocks, ret);
 exit:
-	ext4_fc_stop_update(inode);
 	return ret;
 }
fs/ext4/file.c +0 -4
index 4c5f41052351..8cc11715518a 100644
@@ -259,7 +259,6 @@ static ssize_t ext4_buffered_write_iter(struct kiocb *iocb,
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		return -EOPNOTSUPP;
 
-	ext4_fc_start_update(inode);
 	inode_lock(inode);
 	ret = ext4_write_checks(iocb, from);
 	if (ret <= 0)

@@ -271,7 +270,6 @@ static ssize_t ext4_buffered_write_iter(struct kiocb *iocb,
 
 out:
 	inode_unlock(inode);
-	ext4_fc_stop_update(inode);
 	if (likely(ret > 0)) {
 		iocb->ki_pos += ret;
 		ret = generic_write_sync(iocb, ret);

@@ -552,9 +550,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
 			goto out;
 		}
 
-		ext4_fc_start_update(inode);
 		ret = ext4_orphan_add(handle, inode);
-		ext4_fc_stop_update(inode);
 		if (ret) {
 			ext4_journal_stop(handle);
 			goto out;
fs/ext4/inode.c +1 -6
index bfd3545f1e5d..82f555d26980 100644
@@ -5320,7 +5320,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		if (error)
 			return error;
 	}
-	ext4_fc_start_update(inode);
+
 	if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
 	    (ia_valid & ATTR_GID && !gid_eq(attr->ia_gid, inode->i_gid))) {
 		handle_t *handle;

@@ -5344,7 +5344,6 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 
 		if (error) {
 			ext4_journal_stop(handle);
-			ext4_fc_stop_update(inode);
 			return error;
 		}
 		/* Update corresponding info in inode so that everything is in

@@ -5356,7 +5355,6 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		error = ext4_mark_inode_dirty(handle, inode);
 		ext4_journal_stop(handle);
 		if (unlikely(error)) {
-			ext4_fc_stop_update(inode);
 			return error;
 		}
 	}

@@ -5370,12 +5368,10 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 			struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 
 			if (attr->ia_size > sbi->s_bitmap_maxbytes) {
-				ext4_fc_stop_update(inode);
 				return -EFBIG;
 			}
 		}
 		if (!S_ISREG(inode->i_mode)) {
-			ext4_fc_stop_update(inode);
 			return -EINVAL;
 		}
 

@@ -5499,7 +5495,6 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		ext4_std_error(inode->i_sb, error);
 	if (!error)
 		error = rc;
-	ext4_fc_stop_update(inode);
 	return error;
 }
fs/ext4/ioctl.c +1 -9
index 606dee9e08a3..e64a12e1218a 100644
@@ -743,7 +743,6 @@ int ext4_fileattr_set(struct user_namespace *mnt_userns,
 	u32 flags = fa->flags;
 	int err = -EOPNOTSUPP;
 
-	ext4_fc_start_update(inode);
 	if (flags & ~EXT4_FL_USER_VISIBLE)
 		goto out;
 

@@ -764,7 +763,6 @@ int ext4_fileattr_set(struct user_namespace *mnt_userns,
 		goto out;
 	err = ext4_ioctl_setproject(inode, fa->fsx_projid);
 out:
-	ext4_fc_stop_update(inode);
 	return err;
 }
 

@@ -1273,13 +1271,7 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
-	long ret;
-
-	ext4_fc_start_update(file_inode(filp));
-	ret = __ext4_ioctl(filp, cmd, arg);
-	ext4_fc_stop_update(file_inode(filp));
-
-	return ret;
+	return __ext4_ioctl(filp, cmd, arg);
 }
 
 #ifdef CONFIG_COMPAT
fs/jbd2/journal.c +2 -0
index 35302bc192eb..0b86a4365b66 100644
@@ -757,6 +757,7 @@ int jbd2_fc_begin_commit(journal_t *journal, tid_t tid)
 	}
 	journal->j_flags |= JBD2_FAST_COMMIT_ONGOING;
 	write_unlock(&journal->j_state_lock);
+	jbd2_journal_lock_updates(journal);
 
 	return 0;
 }

@@ -768,6 +769,7 @@ EXPORT_SYMBOL(jbd2_fc_begin_commit);
  */
 static int __jbd2_fc_end_commit(journal_t *journal, tid_t tid, bool fallback)
 {
+	jbd2_journal_unlock_updates(journal);
 	if (journal->j_fc_cleanup_callback)
 		journal->j_fc_cleanup_callback(journal, 0);
 	write_lock(&journal->j_state_lock);
Signature · 1 line
2.34.1.307.g9b7440fafd-goog
author
#2 lore raw

PATCH v2 2/4 ext4: drop ineligible txn start stop APIs

In reply to Harshad Shirwadkar

From: Harshad Shirwadkar <[email protected]>

This patch drops ext4_fc_start_ineligible() and ext4_fc_stop_ineligible() APIs. Fast commit ineligible transactions should simply call ext4_fc_mark_ineligible() after starting the trasaction.

Signed-off-by Harshad Shirwadkar <[email protected]>
---
 fs/ext4/ext4.h        |  6 ++--
 fs/ext4/extents.c     |  6 ++--
 fs/ext4/fast_commit.c | 79 ++++++++-----------------------------------
 fs/ext4/ioctl.c       |  3 +-
 fs/ext4/super.c       |  1 -
 5 files changed, 20 insertions(+), 75 deletions(-)
fs/ext4/ext4.h +2 -4
index 404dd50856e5..d71485d53050 100644
@@ -1725,9 +1725,9 @@ struct ext4_sb_info {
 	 */
 	struct work_struct s_error_work;
 
-	/* Ext4 fast commit stuff */
+	/* Ext4 fast commit sub transaction ID */
 	atomic_t s_fc_subtid;
-	atomic_t s_fc_ineligible_updates;
+
 	/*
 	 * After commit starts, the main queue gets locked, and the further
 	 * updates get added in the staging queue.

@@ -2926,8 +2926,6 @@ void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
 void ext4_fc_track_create(handle_t *handle, struct dentry *dentry);
 void ext4_fc_track_inode(handle_t *handle, struct inode *inode);
 void ext4_fc_mark_ineligible(struct super_block *sb, int reason);
-void ext4_fc_start_ineligible(struct super_block *sb, int reason);
-void ext4_fc_stop_ineligible(struct super_block *sb);
 void ext4_fc_start_update(struct inode *inode);
 void ext4_fc_stop_update(struct inode *inode);
 void ext4_fc_del(struct inode *inode);
fs/ext4/extents.c +2 -4
index 703feff8cb8c..38111ea18ae1 100644
@@ -5341,7 +5341,7 @@ static int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 		ret = PTR_ERR(handle);
 		goto out_mmap;
 	}
-	ext4_fc_start_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE);
+	ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE);
 
 	down_write(&EXT4_I(inode)->i_data_sem);
 	ext4_discard_preallocations(inode, 0);

@@ -5380,7 +5380,6 @@ static int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 
 out_stop:
 	ext4_journal_stop(handle);
-	ext4_fc_stop_ineligible(sb);
 out_mmap:
 	filemap_invalidate_unlock(mapping);
 out_mutex:

@@ -5482,7 +5481,7 @@ static int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
 		ret = PTR_ERR(handle);
 		goto out_mmap;
 	}
-	ext4_fc_start_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE);
+	ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE);
 
 	/* Expand file to avoid data loss if there is error while shifting */
 	inode->i_size += len;

@@ -5557,7 +5556,6 @@ static int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
 
 out_stop:
 	ext4_journal_stop(handle);
-	ext4_fc_stop_ineligible(sb);
 out_mmap:
 	filemap_invalidate_unlock(mapping);
 out_mutex:
fs/ext4/fast_commit.c +15 -64

116 lines not on this page · raw message

fs/ext4/ioctl.c +1 -2
index e64a12e1218a..1366afb59fba 100644
@@ -169,7 +169,7 @@ static long swap_inode_boot_loader(struct super_block *sb,
 		err = -EINVAL;
 		goto err_out;
 	}
-	ext4_fc_start_ineligible(sb, EXT4_FC_REASON_SWAP_BOOT);
+	ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_SWAP_BOOT);
 
 	/* Protect extent tree against block allocations via delalloc */
 	ext4_double_down_write_data_sem(inode, inode_bl);

@@ -252,7 +252,6 @@ static long swap_inode_boot_loader(struct super_block *sb,
 
 err_out1:
 	ext4_journal_stop(handle);
-	ext4_fc_stop_ineligible(sb);
 	ext4_double_up_write_data_sem(inode, inode_bl);
 
 err_out:
fs/ext4/super.c +0 -1
index 6998c07c209a..0e547c6ec73f 100644
@@ -5069,7 +5069,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb,
 
 	/* Initialize fast commit stuff */
 	atomic_set(&sbi->s_fc_subtid, 0);
-	atomic_set(&sbi->s_fc_ineligible_updates, 0);
 	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_MAIN]);
 	INIT_LIST_HEAD(&sbi->s_fc_q[FC_Q_STAGING]);
 	INIT_LIST_HEAD(&sbi->s_fc_dentry_q[FC_Q_MAIN]);
Signature · 1 line
2.34.1.307.g9b7440fafd-goog
author
#3 lore raw

PATCH v2 3/4 ext4: simplify updating of fast commit stats

In reply to Harshad Shirwadkar

From: Harshad Shirwadkar <[email protected]>

Move fast commit stats updating logic to a separate function from ext4_fc_commit(). This significantly improves readability of ext4_fc_commit().

Signed-off-by Harshad Shirwadkar <[email protected]>
---
 fs/ext4/ext4.h        |  1 -
 fs/ext4/fast_commit.c | 99 +++++++++++++++++++++++--------------------
 fs/ext4/fast_commit.h | 27 ++++++------
 3 files changed, 68 insertions(+), 59 deletions(-)
fs/ext4/ext4.h +0 -1
index d71485d53050..82fa51d6f145 100644
@@ -1747,7 +1747,6 @@ struct ext4_sb_info {
 	spinlock_t s_fc_lock;
 	struct buffer_head *s_fc_bh;
 	struct ext4_fc_stats s_fc_stats;
-	u64 s_fc_avg_commit_time;
 #ifdef CONFIG_EXT4_DEBUG
 	int s_fc_debug_max_replay;
 #endif
fs/ext4/fast_commit.c +54 -45

143 lines not on this page · raw message

fs/ext4/fast_commit.h +14 -13
index 937c381b4c85..083ad1cb705a 100644
@@ -71,21 +71,19 @@ struct ext4_fc_tail {
 };
 
 /*
- * Fast commit reason codes
+ * Fast commit status codes
+ */
+enum {
+	EXT4_FC_STATUS_OK = 0,
+	EXT4_FC_STATUS_INELIGIBLE,
+	EXT4_FC_STATUS_SKIPPED,
+	EXT4_FC_STATUS_FAILED,
+};
+
+/*
+ * Fast commit ineligiblity reasons:
  */
 enum {
-	/*
-	 * Commit status codes:
-	 */
-	EXT4_FC_REASON_OK = 0,
-	EXT4_FC_REASON_INELIGIBLE,
-	EXT4_FC_REASON_ALREADY_COMMITTED,
-	EXT4_FC_REASON_FC_START_FAILED,
-	EXT4_FC_REASON_FC_FAILED,
-
-	/*
-	 * Fast commit ineligiblity reasons:
-	 */
 	EXT4_FC_REASON_XATTR = 0,
 	EXT4_FC_REASON_CROSS_RENAME,
 	EXT4_FC_REASON_JOURNAL_FLAG_CHANGE,

@@ -117,7 +115,10 @@ struct ext4_fc_stats {
 	unsigned int fc_ineligible_reason_count[EXT4_FC_REASON_MAX];
 	unsigned long fc_num_commits;
 	unsigned long fc_ineligible_commits;
+	unsigned long fc_failed_commits;
+	unsigned long fc_skipped_commits;
 	unsigned long fc_numblks;
+	u64 s_fc_avg_commit_time;
 };
 
 #define EXT4_FC_REPLAY_REALLOC_INCREMENT	4
Signature · 1 line
2.34.1.307.g9b7440fafd-goog
author
#4 lore raw

PATCH v2 4/4 ext4: update fast commit TODOs

In reply to Harshad Shirwadkar

From: Harshad Shirwadkar <[email protected]>

This series takes care of a couple of TODOs and adds new ones. Update the TODOs section to reflect current state and future work that needs to happen.

Signed-off-by Harshad Shirwadkar <[email protected]>
---
 fs/ext4/fast_commit.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
fs/ext4/fast_commit.c +6 -8
index a37384054c9e..dd002facf6c9 100644
@@ -156,15 +156,13 @@
  *    fast commit recovery even if that area is invalidated by later full
  *    commits.
  *
- * 1) Make fast commit atomic updates more fine grained. Today, a fast commit
- *    eligible update must be protected within ext4_fc_start_update() and
- *    ext4_fc_stop_update(). These routines are called at much higher
- *    routines. This can be made more fine grained by combining with
- *    ext4_journal_start().
+ * 1) Fast commit's commit path locks the entire file system during fast
+ *    commit. This has significant performance penalty. Instead of that, we
+ *    should use ext4_fc_start/stop_update functions to start inode level
+ *    updates from ext4_journal_start/stop. Once we do that we can drop file
+ *    system locking during commit path.
  *
- * 2) Same above for ext4_fc_start_ineligible() and ext4_fc_stop_ineligible()
- *
- * 3) Handle more ineligible cases.
+ * 2) Handle more ineligible cases.
  */
 
 #include <trace/events/ext4.h>
Signature · 1 line
2.34.1.307.g9b7440fafd-goog
maintainer Applied
#5 lore raw
In reply to Harshad Shirwadkar
Quoted 12 lines · Harshad Shirwadkar

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.

Quoted 1 line

[...]

Applied, thanks!

[1/4] ext4: use ext4_journal_start/stop for fast commit transactions

      commit: 2729cfdcfa1cc49bef5a90d046fa4a187fdfcc69
[2/4] ext4: drop ineligible txn start stop APIs
      commit: 7bbbe241ec7ce0def9f71464c878fdbd2b0dcf37
[3/4] ext4: simplify updating of fast commit stats
      commit: 0915e464cb274648e1ef1663e1356e53ff400983
[4/4] ext4: update fast commit TODOs
      commit: d1199b94474ac4513b8491a4b751a8a466e1886b

Best regards,

Signature · 1 line
Theodore Ts'o <[email protected]>
#6 lore raw
In reply to Harshad Shirwadkar
Quoted 18 lines · Harshad Shirwadkar

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:
---------------

Quoted 9 lines

- 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

Quoted 24 lines
Signed-off-by Harshad Shirwadkar <[email protected]>

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

author
#7 lore raw

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

Quoted 78 lines · riteshh
Quoted 18 lines · Harshad Shirwadkar

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:
---------------

Quoted 9 lines

- 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

Quoted 24 lines
Signed-off-by Harshad Shirwadkar <[email protected]>

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

#8 lore raw
Quoted 16 lines · harshad shirwadkar

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.

Quoted 3 lines

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

Quoted 83 lines

Thanks,
Harshad

Quoted 78 lines · riteshh
Quoted 18 lines · Harshad Shirwadkar

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:
---------------

Quoted 9 lines

- 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

Quoted 24 lines
Signed-off-by Harshad Shirwadkar <[email protected]>

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