Skip to contentSkip to conversation

ext4 fast commit API cleanup

PATCH Superseded Harshad Shirwadkar 2 participants 5 messages Dec 20, 2021 – Dec 22, 2021

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover ext4 fast commit API cleanup

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.

Harshad Shirwadkar (3):
ext4: drop transaction start stop APIs for fast commit
ext4: drop ineligible txn start stop APIs
ext4: simplify updating of fast commit stats

 fs/ext4/acl.c         |   2 -
 fs/ext4/ext4.h        |  12 +-
 fs/ext4/extents.c     |   9 +-
 fs/ext4/fast_commit.c | 250 ++++++++++++------------------------------
 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(+), 231 deletions(-)
Signature · 1 line
2.34.1.173.g76aa8bc2d0-goog
#1 lore raw

PATCH 1/3 ext4: drop transaction start stop APIs for fast commit

From: Harshad Shirwadkar <[email protected]>

This patch drops ext4_fc_start_update() and ext4_fc_stop_update() APIs for fast commits. 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.

---
 fs/ext4/acl.c         |  2 --
 fs/ext4/ext4.h        |  5 ---
 fs/ext4/extents.c     |  3 --
 fs/ext4/fast_commit.c | 76 ++++---------------------------------------
 fs/ext4/file.c        |  4 ---
 fs/ext4/inode.c       |  7 +---
 fs/ext4/ioctl.c       | 10 +-----
 fs/jbd2/journal.c     |  2 ++
 8 files changed, 10 insertions(+), 99 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/ext4.h +0 -5
index 404dd50856e5..89bf10d020c9 100644
@@ -1057,9 +1057,6 @@ struct ext4_inode_info {
 	/* End of lblk range that needs to be committed in this fast commit */
 	ext4_lblk_t i_fc_lblk_len;
 
-	/* Number of ongoing updates on this inode */
-	atomic_t  i_fc_updates;
-
 	/* Fast commit wait queue for this inode */
 	wait_queue_head_t i_fc_wait;
 
@@ -2928,8 +2925,6 @@ 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);
 bool ext4_fc_replay_check_excluded(struct super_block *sb, ext4_fsblk_t block);
 void ext4_fc_replay_cleanup(struct super_block *sb);
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/fast_commit.c +6 -70

107 lines not on this page · raw message

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.173.g76aa8bc2d0-goog
#2 lore raw

PATCH 2/3 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.

---
 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 89bf10d020c9..a6cb4ca99c41 100644
@@ -1722,9 +1722,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.
@@ -2923,8 +2923,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_del(struct inode *inode);
 bool ext4_fc_replay_check_excluded(struct super_block *sb, ext4_fsblk_t block);
 void ext4_fc_replay_cleanup(struct super_block *sb);
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.173.g76aa8bc2d0-goog
#3 lore raw

PATCH 3/3 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().

---
 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 a6cb4ca99c41..204302d6ecd1 100644
@@ -1744,7 +1744,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.173.g76aa8bc2d0-goog
Tested-by on the v1 series
#4 lore raw
In reply to Harshad Shirwadkar

* Harshad Shirwadkar <[email protected]>:

Quoted 24 lines

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.

Harshad Shirwadkar (3):
ext4: drop transaction start stop APIs for fast commit
ext4: drop ineligible txn start stop APIs
ext4: simplify updating of fast commit stats

Quoted 15 lines
 fs/ext4/acl.c         |   2 -
 fs/ext4/ext4.h        |  12 +-
 fs/ext4/extents.c     |   9 +-
 fs/ext4/fast_commit.c | 250 ++++++++++++------------------------------
 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(+), 231 deletions(-)
Signature · 1 line
2.34.1.173.g76aa8bc2d0-goog

Hi Harshad:

I applied this patch series to 5.16-rc6, and then ran 500 trials of generic/083 using xfstests-bld's adv test case (where I'd consistently encountered kernel hangs after 16 or 17 runs) without failures and a full -g auto run over all the default test cases without regressions.

Thanks for the patches!

Eric