ext4: remove journal barrier during fast commit

From: Harshad Shirwadkar <[email protected]>

In commit 2729cfdcfa1cc49bef5a90d046fa4a187fdfcc69 ("ext4: use

ext4_journal_start/stop for fast commit transactions"), journal barrier was introduced in fast commit path as an intermediate step for fast commit API migration. This patch removes the journal barrier to improve the fast commit performance. Instead of blocking the entire journal before starting the fast commit, this patch only blocks the inode that is being committed during a fast commit.

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

fs/ext4/ext4_jbd2.c +11 -0

index 3477a16d08ae..16321f89934c 100644
@@ -106,6 +106,61 @@ handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line,
 				   GFP_NOFS, type, line);
 }
 
+handle_t *__ext4_journal_start(struct inode *inode, unsigned int line,
+				  int type, int blocks, int rsv_blocks,
+				  int revoke_creds)
+{
+	handle_t *handle;
+	journal_t *journal;
+	int err;
+
+	trace_ext4_journal_start(inode->i_sb, blocks, rsv_blocks, revoke_creds,
+				 _RET_IP_);
+	err = ext4_journal_check_start(inode->i_sb);

fs/ext4/ext4_jbd2.c +8 -0

+	if (test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT)
+	    && !IS_ERR(handle)
+	    && !ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE)) {
+		if (handle->h_ref == 1) {
+			WARN_ON(handle->h_priv != NULL);
+			ext4_fc_start_update(handle, inode);
+			handle->h_priv = inode;
+			return handle;

fs/ext4/ext4_jbd2.c +16 -0

+
+	return handle;
+}
+
+/* Stop fast commit update on the inode in this handle, if any. */
+static void ext4_fc_journal_stop(handle_t *handle)
+{
+	if (!handle->h_priv || handle->h_ref > 1)
+		return;
+	/*
+	 * We have an inode and this is the top level __ext4_journal_stop call.
+	 */
+	ext4_fc_stop_update(handle);
+	handle->h_priv = NULL;
+}
+
 int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle)
 {
 	struct super_block *sb;

@@ -119,11 +174,13 @@ int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle)
 
 	err = handle->h_err;
 	if (!handle->h_transaction) {

fs/ext4/inode.c +0 -0

 	if (IS_I_VERSION(inode))
 		inode_inc_iversion(inode);

fs/ext4/super.c +0 -0

index d1c4b04e72ab..7cbe0084bb39 100644

Quoted 3 lines · Harshad Shirwadkar

From: Harshad Shirwadkar <[email protected]>

In commit 2729cfdcfa1cc49bef5a90d046fa4a187fdfcc69 ("ext4: use

Just first 12 digits from the commit sha is enough :)

Quoted 9 lines

ext4_journal_start/stop for fast commit transactions"), journal barrier was introduced in fast commit path as an intermediate step for fast commit API migration. This patch removes the journal barrier to improve the fast commit performance. Instead of blocking the entire journal before starting the fast commit, this patch only blocks the inode that is being committed during a fast commit.

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

...

Quoted 19 lines

fs/ext4/ext4_jbd2.c +11 -0

index 3477a16d08ae..16321f89934c 100644
@@ -106,6 +106,61 @@ handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line,
                                 GFP_NOFS, type, line);
 }
 
+handle_t *__ext4_journal_start(struct inode *inode, unsigned int line,
+                               int type, int blocks, int rsv_blocks,
+                               int revoke_creds)
+{
+     handle_t *handle;
+     journal_t *journal;
+     int err;
+
+     trace_ext4_journal_start(inode->i_sb, blocks, rsv_blocks, revoke_creds,
+                              _RET_IP_);
+     err = ext4_journal_check_start(inode->i_sb);

fs/ext4/ext4_jbd2.c handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line, · line 129

Quoted 10 lines

fs/ext4/ext4_jbd2.c +10 -0

+     if (err < 0)
+             return ERR_PTR(err);
+
+     journal = EXT4_SB(inode->i_sb)->s_journal;
+     if (!journal || (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY))
+             return ext4_get_nojournal();
+
+     handle = jbd2__journal_start(journal, blocks, rsv_blocks, revoke_creds,
+                                  GFP_NOFS, type, line);
+

Perhaps you could reuse __ext4_journal_start_sb() in the above?

Quoted 8 lines

fs/ext4/ext4_jbd2.c +8 -0

+     if (test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT)
+         && !IS_ERR(handle)
+         && !ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE)) {
+             if (handle->h_ref == 1) {
+                     WARN_ON(handle->h_priv != NULL);
+                     ext4_fc_start_update(handle, inode);
+                     handle->h_priv = inode;
+                     return handle;

fs/ext4/ext4_jbd2.c handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line, · line 147

Quoted 10 lines

fs/ext4/ext4_jbd2.c +10 -0

+             }
+             /*
+              * Check if this is a nested transaction that modifies multiple
+              * inodes. Such a transaction is fast commit ineligible.
+              */
+             if (handle->h_priv != inode)
+                     ext4_fc_mark_ineligible(inode->i_sb,
+                                             EXT4_FC_REASON_TOO_MANY_INODES,
+                                             handle);
+     }

Hum, here you seem to assume that if inode will be modified, we will call __ext4_journal_start() with that inode. But that is not true. It is perfectly valid to start a transaction with ext4_journal_start_sb() and then add inodes to it. ext4_journal_start() is just a convenience helper to save some boilerplate code you can but don't have to use when starting a transaction. In particular we can have handles modifying more inodes without calling ext4_journal_start() for all of them. We also have places (most notably inode allocation) that definitely modify inodes but start transaction with ext4_journal_start_sb(). A lot of auditing would be required to make this approach work and even more to make sure it does not break in the future.

If I'm reading the code right, what you need to achieve is that buffer backing raw inode or inode's logical->physical block mapping is not modified while the fastcommit including that inode is running because it would corrupt the information being committed. So would not it be enough to call ext4_fc_start_update() in ext4_map_blocks() once we know that we need to modify block mapping and similarly in ext4_reserve_inode_write() (which would need a bit of work to get used universally - fs/ext4/inline.c does not seem to use it)? In ext4_journal_stop() we can then call ext4_fc_stop_update() (we could either keep going with the one-inode-per-handle limitation you have or introduce a list of inodes attached to a handle). So essentially attaching inode to fastcommit would rather be similar to jbd2_journal_get_write_access() than a transaction start. I guess in principle that would work we just have to be careful not to introduce deadlocks with running fastcommit (so that fastcommit does not wait for some inode update to finish, owner of the handle with that inode update waits for some lock, and the lock is held by someone waiting for

fastcommit to finish). So to do that we would need to block all new handle starts, wait for all inode updates to finish (which essentially means wait for all handles that modify inodes involved in fastcommit), set EXT4_STATE_FC_COMMITTING for all involved inodes, unblock handle starts and then we can go on with the fastcommit and EXT4_STATE_FC_COMMITTING flags will protect us from inode modifications. This is better than journal_lock_updates() we have now but I'm not sure this is the improvement you were looking for ;).

Quoted 29 lines

fs/ext4/ext4_jbd2.c +17 -0

+
+     return handle;
+}
+
+/* Stop fast commit update on the inode in this handle, if any. */
+static void ext4_fc_journal_stop(handle_t *handle)
+{
+     if (!handle->h_priv || handle->h_ref > 1)
+             return;
+     /*
+      * We have an inode and this is the top level __ext4_journal_stop call.
+      */
+     ext4_fc_stop_update(handle);
+     handle->h_priv = NULL;
+}
+
 int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle)
 {
      struct super_block *sb;

@@ -119,11 +174,13 @@ int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle)
 
      err = handle->h_err;
      if (!handle->h_transaction) {
+             ext4_fc_journal_stop(handle);
              rc = jbd2_journal_stop(handle);
              return err ? err : rc;
      }
 

fs/ext4/ext4_jbd2.c int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle) · line 186

Quoted 5 lines

fs/ext4/ext4_jbd2.c +1 -0

      sb = handle->h_transaction->t_journal->j_private;
+     ext4_fc_journal_stop(handle);
      rc = jbd2_journal_stop(handle);
 
      if (!err)

Why don't you call ext4_fc_journal_stop() a bit earlier and thus avoid the two callsites?

fs/ext4/inode.c int ext4_mark_iloc_dirty(handle_t *handle, · line 5661

Quoted 10 lines

fs/ext4/inode.c +0 -1

index d2a29fc93742..5edac6f6f7d3 100644
@@ -5658,7 +5658,6 @@ int ext4_mark_iloc_dirty(handle_t *handle,
              put_bh(iloc->bh);
              return -EIO;
      }
-     ext4_fc_track_inode(handle, inode);

I'm confused why it is safe to remove this. I mean if a transaction is modifying multiple inodes you will not track them in fast commit?

Quoted 10 lines

fs/ext4/inode.c +0 -0

      if (IS_I_VERSION(inode))
              inode_inc_iversion(inode);

fs/ext4/super.c +0 -0

index d1c4b04e72ab..7cbe0084bb39 100644
@@ -1428,7 +1428,6 @@ static void destroy_inodecache(void)
 

fs/ext4/super.c static void destroy_inodecache(void) · line 1433

Quoted 6 lines

fs/ext4/super.c +0 -1

 void ext4_clear_inode(struct inode *inode)
 {
-     ext4_fc_del(inode);
      invalidate_inode_buffers(inode);
      clear_inode(inode);
      ext4_discard_preallocations(inode, 0);

Is this really safe? What prevents inode reclaim from reclaiming inode while it is still part of fastcommit?

Honza

--
Jan Kara <[email protected]>
SUSE Labs, CR