[RFC v3 0/2] ext4: fast commit: fix lockdep issues

[RFC v3 0/2] ext4: fast commit: fix lockdep issues

[Li Chen]

Hi,

(This RFC v3 series is based on top of the patch posted at
https://lore.kernel.org/linux-ext4/20251223131342.287864-1-me@linux.beauty/T/#u)

This series fixes two lockdep issues in the ext4 fast commit paths.

1) ext4_fc_track_inode() can return without sleeping when
   EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
   i_data_sem should only fire when we actually go to sleep.

2) lockdep reports a possible deadlock due to lock order inversion
   between s_fc_lock and i_data_sem. The fast commit writer held s_fc_lock
   while writing the fast commit log. Writing the journal inode mapping
   can call ext4_map_blocks() and take i_data_sem, while metadata update
   paths can hold i_data_sem and call ext4_fc_track_inode() which takes
   s_fc_lock.

The fix drops s_fc_lock before the log writing step and uses
EXT4_STATE_FC_COMMITTING to keep inode and create dentry state stable
until cleanup.

Testing:
- QEMU VM, ext4 -O fast_commit on virtio-pmem + dax, verified both lockdep
  report reproduces on an older kernel and is gone with this series.

RFC v2 -> RFC v3:
 - rebase ontop of https://lore.kernel.org/linux-ext4/20251223131342.287864-1-me@linux.beauty/T/#u

RFC v1 -> RFC v2:
 - patch 1: move comments to correct place
 - patch 2: add it to patchset.
 - add missing RFC prefix

RFC v1: https://lore.kernel.org/linux-ext4/20251222032655.87056-1-me@linux.beauty/T/#u
RFC v2: https://lore.kernel.org/linux-ext4/20251222151906.24607-1-me@linux.beauty/T/#t

Li Chen (2):
  ext4: fast_commit: assert i_data_sem only before sleep
  ext4: fast commit: fix s_fc_lock vs i_data_sem inversion

 fs/ext4/fast_commit.c | 96 +++++++++++++++++++++++++++++++------------
 1 file changed, 69 insertions(+), 27 deletions(-)

-- 
2.52.0

[RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before sleep

[Li Chen]

ext4_fc_track_inode() can return without sleeping when
EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
ei->i_data_sem was done unconditionally before the wait loop, which can
WARN in call paths that hold i_data_sem even though we never block. Move
lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
right before schedule().

Signed-off-by: Li Chen <me@linux.beauty>
---
 fs/ext4/fast_commit.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index d0926967d086..b0c458082997 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -566,13 +566,6 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
 	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
 		return;
 
-	/*
-	 * If we come here, we may sleep while waiting for the inode to
-	 * commit. We shouldn't be holding i_data_sem when we go to sleep since
-	 * the commit path needs to grab the lock while committing the inode.
-	 */
-	lockdep_assert_not_held(&ei->i_data_sem);
-
 	while (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
 #if (BITS_PER_LONG < 64)
 		DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
@@ -586,8 +579,16 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
 				   EXT4_STATE_FC_COMMITTING);
 #endif
 		prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
-		if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING))
+		if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
+			/*
+			 * We might sleep while waiting for the inode to commit.
+			 * We shouldn't be holding i_data_sem when we go to sleep
+			 * since the commit path may grab it while committing this
+			 * inode.
+			 */
+			lockdep_assert_not_held(&ei->i_data_sem);
 			schedule();
+		}
 		finish_wait(wq, &wait.wq_entry);
 	}
 
-- 
2.52.0

[RFC v3 2/2] ext4: fast commit: fix s_fc_lock vs i_data_sem inversion

[Li Chen]

lockdep reports a possible deadlock due to lock order inversion:

     CPU0                    CPU1
     ----                    ----
lock(&sbi->s_fc_lock);
                             lock(&ei->i_data_sem);
                             lock(&sbi->s_fc_lock);
rlock(&ei->i_data_sem);

ext4_fc_perform_commit() held s_fc_lock while writing fast commit blocks.
This can write the journal inode, whose mapping can call ext4_map_blocks()
and take i_data_sem. At the same time, metadata update paths can hold
i_data_sem and call ext4_fc_track_inode(), which takes s_fc_lock.

Drop s_fc_lock before the log writing step. Keep inode and dentry state
stable by using EXT4_STATE_FC_COMMITTING for synchronization: ext4_fc_del()
waits for COMMITTING, and inodes referenced only from create dentry updates
are also marked COMMITTING and woken up on cleanup.

Signed-off-by: Li Chen <me@linux.beauty>
---
 fs/ext4/fast_commit.c | 79 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 60 insertions(+), 19 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index b0c458082997..aa209f1d3d36 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -244,23 +244,26 @@ void ext4_fc_del(struct inode *inode)
 		return;
 	}
 
-	/*
-	 * Since ext4_fc_del is called from ext4_evict_inode while having a
-	 * handle open, there is no need for us to wait here even if a fast
-	 * commit is going on. That is because, if this inode is being
-	 * committed, ext4_mark_inode_dirty would have waited for inode commit
-	 * operation to finish before we come here. So, by the time we come
-	 * here, inode's EXT4_STATE_FC_COMMITTING would have been cleared. So,
-	 * we shouldn't see EXT4_STATE_FC_COMMITTING to be set on this inode
-	 * here.
-	 *
-	 * We may come here without any handles open in the "no_delete" case of
-	 * ext4_evict_inode as well. However, if that happens, we first mark the
-	 * file system as fast commit ineligible anyway. So, even in that case,
-	 * it is okay to remove the inode from the fc list.
-	 */
-	WARN_ON(ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)
-		&& !ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE));
+	/* Don't race with fast commit processing of this inode. */
+	while (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
+#if (BITS_PER_LONG < 64)
+		DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
+				EXT4_STATE_FC_COMMITTING);
+		wq = bit_waitqueue(&ei->i_state_flags,
+				   EXT4_STATE_FC_COMMITTING);
+#else
+		DEFINE_WAIT_BIT(wait, &ei->i_flags,
+				EXT4_STATE_FC_COMMITTING);
+		wq = bit_waitqueue(&ei->i_flags, EXT4_STATE_FC_COMMITTING);
+#endif
+		prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
+		if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
+			mutex_unlock(&sbi->s_fc_lock);
+			schedule();
+			mutex_lock(&sbi->s_fc_lock);
+		}
+		finish_wait(wq, &wait.wq_entry);
+	}
 	while (ext4_test_inode_state(inode, EXT4_STATE_FC_FLUSHING_DATA)) {
 #if (BITS_PER_LONG < 64)
 		DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
@@ -1108,6 +1111,27 @@ static int ext4_fc_perform_commit(journal_t *journal)
 		ext4_set_inode_state(&iter->vfs_inode,
 				     EXT4_STATE_FC_COMMITTING);
 	}
+	/*
+	 * Also mark inodes referenced by create dentry updates. These inodes are
+	 * tracked via i_fc_dilist and might not be on s_fc_q[MAIN].
+	 */
+	{
+		struct ext4_fc_dentry_update *fc_dentry;
+		struct ext4_inode_info *ei;
+
+		list_for_each_entry(fc_dentry, &sbi->s_fc_dentry_q[FC_Q_MAIN],
+				    fcd_list) {
+			if (fc_dentry->fcd_op != EXT4_FC_TAG_CREAT)
+				continue;
+			if (list_empty(&fc_dentry->fcd_dilist))
+				continue;
+			ei = list_first_entry(&fc_dentry->fcd_dilist,
+					      struct ext4_inode_info,
+					      i_fc_dilist);
+			ext4_set_inode_state(&ei->vfs_inode,
+					     EXT4_STATE_FC_COMMITTING);
+		}
+	}
 	mutex_unlock(&sbi->s_fc_lock);
 	jbd2_journal_unlock_updates(journal);
 
@@ -1137,7 +1161,6 @@ static int ext4_fc_perform_commit(journal_t *journal)
 	}
 
 	/* Step 6.2: Now write all the dentry updates. */
-	mutex_lock(&sbi->s_fc_lock);
 	ret = ext4_fc_commit_dentry_updates(journal, &crc);
 	if (ret)
 		goto out;
@@ -1159,7 +1182,6 @@ static int ext4_fc_perform_commit(journal_t *journal)
 	ret = ext4_fc_write_tail(sb, crc);
 
 out:
-	mutex_unlock(&sbi->s_fc_lock);
 	memalloc_nofs_restore(nofs);
 	blk_finish_plug(&plug);
 	return ret;
@@ -1342,6 +1364,25 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)
 					     struct ext4_fc_dentry_update,
 					     fcd_list);
 		list_del_init(&fc_dentry->fcd_list);
+		if (fc_dentry->fcd_op == EXT4_FC_TAG_CREAT &&
+		    !list_empty(&fc_dentry->fcd_dilist)) {
+			ei = list_first_entry(&fc_dentry->fcd_dilist,
+					      struct ext4_inode_info,
+					      i_fc_dilist);
+			ext4_clear_inode_state(&ei->vfs_inode,
+					       EXT4_STATE_FC_COMMITTING);
+			/*
+			 * Make sure clearing of EXT4_STATE_FC_COMMITTING is
+			 * visible before we send the wakeup. Pairs with implicit
+			 * barrier in prepare_to_wait() in ext4_fc_track_inode().
+			 */
+			smp_mb();
+#if (BITS_PER_LONG < 64)
+			wake_up_bit(&ei->i_state_flags, EXT4_STATE_FC_COMMITTING);
+#else
+			wake_up_bit(&ei->i_flags, EXT4_STATE_FC_COMMITTING);
+#endif
+		}
 		list_del_init(&fc_dentry->fcd_dilist);
 
 		release_dentry_name_snapshot(&fc_dentry->fcd_name);
-- 
2.52.0

Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before sleep

[Zhang Yi]

Hi Li,

On 12/24/2025 11:29 AM, Li Chen wrote:
> ext4_fc_track_inode() can return without sleeping when
> EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
> ei->i_data_sem was done unconditionally before the wait loop, which can
> WARN in call paths that hold i_data_sem even though we never block. Move
> lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
> right before schedule().
> 
> Signed-off-by: Li Chen <me@linux.beauty>

Thank you for the fix patch! However, the solution does not seem to fix
the issue. IIUC, the root cause of this issue is the following race
condition (show only one case), and it may cause a real ABBA dead lock
issue.

ext4_map_blocks()
 hold i_data_sem // <- A
 ext4_mb_new_blocks()
  ext4_dirty_inode()
                                 ext4_fc_commit()
                                  ext4_fc_perform_commit()
                                   set EXT4_STATE_FC_COMMITTING  <-B
                                   ext4_fc_write_inode_data()
                                   ext4_map_blocks()
                                    hold i_data_sem  // <- A
   ext4_fc_track_inode()
    wait EXT4_STATE_FC_COMMITTING  <- B
                                  jbd2_fc_end_commit()
                                   ext4_fc_cleanup()
                                    clear EXT4_STATE_FC_COMMITTING()

Postponing the lockdep assertion to the point where sleeping is actually
necessary does not resolve this deadlock issue, it merely masks the
problem, right?

I currently don't quite understand why only ext4_fc_track_inode() needs
to wait for the inode being fast committed to be completed, instead of
adding it to the FC_Q_STAGING list like other tracking operations. So
now I don't have a good idea to fix this problem either.  Perhaps we
need to rethink the necessity of this waiting, or find a way to avoid
acquiring i_data_sem during fast commit.

Thanks,
Yi.

> ---
>  fs/ext4/fast_commit.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index d0926967d086..b0c458082997 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -566,13 +566,6 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
>  	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
>  		return;
>  
> -	/*
> -	 * If we come here, we may sleep while waiting for the inode to
> -	 * commit. We shouldn't be holding i_data_sem when we go to sleep since
> -	 * the commit path needs to grab the lock while committing the inode.
> -	 */
> -	lockdep_assert_not_held(&ei->i_data_sem);
> -
>  	while (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
>  #if (BITS_PER_LONG < 64)
>  		DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
> @@ -586,8 +579,16 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
>  				   EXT4_STATE_FC_COMMITTING);
>  #endif
>  		prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
> -		if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING))
> +		if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
> +			/*
> +			 * We might sleep while waiting for the inode to commit.
> +			 * We shouldn't be holding i_data_sem when we go to sleep
> +			 * since the commit path may grab it while committing this
> +			 * inode.
> +			 */
> +			lockdep_assert_not_held(&ei->i_data_sem);
>  			schedule();
> +		}
>  		finish_wait(wq, &wait.wq_entry);
>  	}
>  

Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before sleep

[Li Chen]

Hi Zhang Yi,

 ---- On Mon, 05 Jan 2026 20:18:42 +0800  Zhang Yi <yi.zhang@huaweicloud.com> wrote --- 
 > Hi Li,
 > 
 > On 12/24/2025 11:29 AM, Li Chen wrote:
 > > ext4_fc_track_inode() can return without sleeping when
 > > EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
 > > ei->i_data_sem was done unconditionally before the wait loop, which can
 > > WARN in call paths that hold i_data_sem even though we never block. Move
 > > lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
 > > right before schedule().
 > > 
 > > Signed-off-by: Li Chen <me@linux.beauty>
 > 
 > Thank you for the fix patch! However, the solution does not seem to fix
 > the issue. IIUC, the root cause of this issue is the following race
 > condition (show only one case), and it may cause a real ABBA dead lock
 > issue.
 > 
 > ext4_map_blocks()
 >  hold i_data_sem // <- A
 >  ext4_mb_new_blocks()
 >   ext4_dirty_inode()
 >                                  ext4_fc_commit()
 >                                   ext4_fc_perform_commit()
 >                                    set EXT4_STATE_FC_COMMITTING  <-B
 >                                    ext4_fc_write_inode_data()
 >                                    ext4_map_blocks()
 >                                     hold i_data_sem  // <- A
 >    ext4_fc_track_inode()
 >     wait EXT4_STATE_FC_COMMITTING  <- B
 >                                   jbd2_fc_end_commit()
 >                                    ext4_fc_cleanup()
 >                                     clear EXT4_STATE_FC_COMMITTING()
 > 
 > Postponing the lockdep assertion to the point where sleeping is actually
 > necessary does not resolve this deadlock issue, it merely masks the
 > problem, right?
 > 
 > I currently don't quite understand why only ext4_fc_track_inode() needs
 > to wait for the inode being fast committed to be completed, instead of
 > adding it to the FC_Q_STAGING list like other tracking operations. So
 > now I don't have a good idea to fix this problem either.  Perhaps we
 > need to rethink the necessity of this waiting, or find a way to avoid
 > acquiring i_data_sem during fast commit.

Thanks a lot for your kind review! I'll provide feedback tomorrow.

Regards,
Li​

Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before sleep

[Zhang Yi]

On 1/6/2026 8:18 PM, Li Chen wrote:
> Hi Zhang Yi,
> 
>  ---- On Mon, 05 Jan 2026 20:18:42 +0800  Zhang Yi <yi.zhang@huaweicloud.com> wrote --- 
>  > Hi Li,
>  > 
>  > On 12/24/2025 11:29 AM, Li Chen wrote:
>  > > ext4_fc_track_inode() can return without sleeping when
>  > > EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
>  > > ei->i_data_sem was done unconditionally before the wait loop, which can
>  > > WARN in call paths that hold i_data_sem even though we never block. Move
>  > > lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
>  > > right before schedule().
>  > > 
>  > > Signed-off-by: Li Chen <me@linux.beauty>
>  > 
>  > Thank you for the fix patch! However, the solution does not seem to fix
>  > the issue. IIUC, the root cause of this issue is the following race
>  > condition (show only one case), and it may cause a real ABBA dead lock
>  > issue.
>  > 
>  > ext4_map_blocks()
>  >  hold i_data_sem // <- A
>  >  ext4_mb_new_blocks()
>  >   ext4_dirty_inode()
>  >                                  ext4_fc_commit()
>  >                                   ext4_fc_perform_commit()
>  >                                    set EXT4_STATE_FC_COMMITTING  <-B
>  >                                    ext4_fc_write_inode_data()
>  >                                    ext4_map_blocks()
>  >                                     hold i_data_sem  // <- A
>  >    ext4_fc_track_inode()
>  >     wait EXT4_STATE_FC_COMMITTING  <- B
>  >                                   jbd2_fc_end_commit()
>  >                                    ext4_fc_cleanup()
>  >                                     clear EXT4_STATE_FC_COMMITTING()
>  > 
>  > Postponing the lockdep assertion to the point where sleeping is actually
>  > necessary does not resolve this deadlock issue, it merely masks the
>  > problem, right?
>  > 
>  > I currently don't quite understand why only ext4_fc_track_inode() needs
>  > to wait for the inode being fast committed to be completed, instead of
>  > adding it to the FC_Q_STAGING list like other tracking operations.

It seems that the inode metadata of the tracked inode was not recorded
during the __track_inode(), so the inode metadata committed at commit
time reflects real-time data. However, the current
ext4_fc_perform_commit() lacks concurrency control, allowing other
processes to simultaneously initiate new handles that modify the inode
metadata while the previous metadata is being fast committed. Therefore,
to prevent recording newly changed inode metadata during the old commit
phase, the ext4_fc_track_inode() function must wait for the ongoing
commit process to complete before modifying.

>  > So
>  > now I don't have a good idea to fix this problem either.  Perhaps we
>  > need to rethink the necessity of this waiting, or find a way to avoid
>  > acquiring i_data_sem during fast commit.

Ha, the solution seems to have already been listed in the TODOs in
fast_commit.c.

  Change ext4_fc_commit() to lookup logical to physical mapping using extent
  status tree. This would get rid of the need to call ext4_fc_track_inode()
  before acquiring i_data_sem. To do that we would need to ensure that
  modified extents from the extent status tree are not evicted from memory.

Alternatively, recording the mapped range of tracking might also be
feasible.

Thanks,
Yi.

> 
> Thanks a lot for your kind review! I'll provide feedback tomorrow.
> 
> Regards,
> Li​
> 

Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before sleep

[Li Chen]

Hi Zhang,

Thanks a lot for your comments!

 ---- On Mon, 05 Jan 2026 20:18:42 +0800  Zhang Yi <yi.zhang@huaweicloud.com> wrote --- 
 > Hi Li,
 > 
 > On 12/24/2025 11:29 AM, Li Chen wrote:
 > > ext4_fc_track_inode() can return without sleeping when
 > > EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
 > > ei->i_data_sem was done unconditionally before the wait loop, which can
 > > WARN in call paths that hold i_data_sem even though we never block. Move
 > > lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
 > > right before schedule().
 > > 
 > > Signed-off-by: Li Chen <me@linux.beauty>
 > 
 > Thank you for the fix patch! However, the solution does not seem to fix
 > the issue. IIUC, the root cause of this issue is the following race
 > condition (show only one case), and it may cause a real ABBA dead lock
 > issue.
 > 
 > ext4_map_blocks()
 >  hold i_data_sem // <- A
 >  ext4_mb_new_blocks()
 >   ext4_dirty_inode()
 >                                  ext4_fc_commit()
 >                                   ext4_fc_perform_commit()
 >                                    set EXT4_STATE_FC_COMMITTING  <-B
 >                                    ext4_fc_write_inode_data()
 >                                    ext4_map_blocks()
 >                                     hold i_data_sem  // <- A
 >    ext4_fc_track_inode()
 >     wait EXT4_STATE_FC_COMMITTING  <- B
 >                                   jbd2_fc_end_commit()
 >                                    ext4_fc_cleanup()
 >                                     clear EXT4_STATE_FC_COMMITTING()
 > 
 > Postponing the lockdep assertion to the point where sleeping is actually
 > necessary does not resolve this deadlock issue, it merely masks the
 > problem, right?
 
I agree. Moving lockdep_assert_not_held(&ei->i_data_sem) closer to the
schedule() site can reduce spurious warnings (since the wait-bit pattern
rechecks the bit after prepare_to_wait()), but it does not remove the
underlying deadlock risk if we ever end up sleeping there, althoughI still 
haven't been able to reproduce this ABBA issue.

 > I currently don't quite understand why only ext4_fc_track_inode() needs
 > to wait for the inode being fast committed to be completed, instead of
 > adding it to the FC_Q_STAGING list like other tracking operations. So
 > now I don't have a good idea to fix this problem either.  Perhaps we
 > need to rethink the necessity of this waiting, or find a way to avoid
 > acquiring i_data_sem during fast commit.
 > 
 > Thanks,
 > Yi.
 > 
 > > ---
 > >  fs/ext4/fast_commit.c | 17 +++++++++--------
 > >  1 file changed, 9 insertions(+), 8 deletions(-)
 > > 
 > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
 > > index d0926967d086..b0c458082997 100644
 > > --- a/fs/ext4/fast_commit.c
 > > +++ b/fs/ext4/fast_commit.c
 > > @@ -566,13 +566,6 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
 > >      if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
 > >          return;
 > >  
 > > -    /*
 > > -     * If we come here, we may sleep while waiting for the inode to
 > > -     * commit. We shouldn't be holding i_data_sem when we go to sleep since
 > > -     * the commit path needs to grab the lock while committing the inode.
 > > -     */
 > > -    lockdep_assert_not_held(&ei->i_data_sem);
 > > -
 > >      while (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
 > >  #if (BITS_PER_LONG < 64)
 > >          DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
 > > @@ -586,8 +579,16 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
 > >                     EXT4_STATE_FC_COMMITTING);
 > >  #endif
 > >          prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
 > > -        if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING))
 > > +        if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
 > > +            /*
 > > +             * We might sleep while waiting for the inode to commit.
 > > +             * We shouldn't be holding i_data_sem when we go to sleep
 > > +             * since the commit path may grab it while committing this
 > > +             * inode.
 > > +             */
 > > +            lockdep_assert_not_held(&ei->i_data_sem);
 > >              schedule();
 > > +        }
 > >          finish_wait(wq, &wait.wq_entry);
 > >      }
 > >  
 > 
 > 

Regards,
Li​

Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before sleep

[Li Chen]

Hi Zhang,

Thanks a lot for your detailed review!

 ---- On Wed, 07 Jan 2026 10:00:23 +0800  Zhang Yi <yi.zhang@huaweicloud.com> wrote --- 
 > On 1/6/2026 8:18 PM, Li Chen wrote:
 > > Hi Zhang Yi,
 > > 
 > >  ---- On Mon, 05 Jan 2026 20:18:42 +0800  Zhang Yi <yi.zhang@huaweicloud.com> wrote --- 
 > >  > Hi Li,
 > >  > 
 > >  > On 12/24/2025 11:29 AM, Li Chen wrote:
 > >  > > ext4_fc_track_inode() can return without sleeping when
 > >  > > EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
 > >  > > ei->i_data_sem was done unconditionally before the wait loop, which can
 > >  > > WARN in call paths that hold i_data_sem even though we never block. Move
 > >  > > lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
 > >  > > right before schedule().
 > >  > > 
 > >  > > Signed-off-by: Li Chen <me@linux.beauty>
 > >  > 
 > >  > Thank you for the fix patch! However, the solution does not seem to fix
 > >  > the issue. IIUC, the root cause of this issue is the following race
 > >  > condition (show only one case), and it may cause a real ABBA dead lock
 > >  > issue.
 > >  > 
 > >  > ext4_map_blocks()
 > >  >  hold i_data_sem // <- A
 > >  >  ext4_mb_new_blocks()
 > >  >   ext4_dirty_inode()
 > >  >                                  ext4_fc_commit()
 > >  >                                   ext4_fc_perform_commit()
 > >  >                                    set EXT4_STATE_FC_COMMITTING  <-B
 > >  >                                    ext4_fc_write_inode_data()
 > >  >                                    ext4_map_blocks()
 > >  >                                     hold i_data_sem  // <- A
 > >  >    ext4_fc_track_inode()
 > >  >     wait EXT4_STATE_FC_COMMITTING  <- B
 > >  >                                   jbd2_fc_end_commit()
 > >  >                                    ext4_fc_cleanup()
 > >  >                                     clear EXT4_STATE_FC_COMMITTING()
 > >  > 
 
I think the ABBA reasoning is plausible: if a caller violates the ordering
contract and enters ext4_fc_track_inode() while holding i_data_sem, and the
recheck still finds EXT4_STATE_FC_COMMITTING set (so we actually schedule()),
then we can get A -> wait(B). If the commit task, while holding the inode
in COMMITTING, still needs i_data_sem (e.g. via mapping/log writing), that
gives B -> wait(A), forming a cycle.

 > >  > Postponing the lockdep assertion to the point where sleeping is actually
 > >  > necessary does not resolve this deadlock issue, it merely masks the
 > >  > problem, right?
 > >  > 
 > >  > I currently don't quite understand why only ext4_fc_track_inode() needs
 > >  > to wait for the inode being fast committed to be completed, instead of
 > >  > adding it to the FC_Q_STAGING list like other tracking operations.
 > 
 > It seems that the inode metadata of the tracked inode was not recorded
 > during the __track_inode(), so the inode metadata committed at commit
 > time reflects real-time data. However, the current
 > ext4_fc_perform_commit() lacks concurrency control, allowing other
 > processes to simultaneously initiate new handles that modify the inode
 > metadata while the previous metadata is being fast committed. Therefore,
 > to prevent recording newly changed inode metadata during the old commit
 > phase, the ext4_fc_track_inode() function must wait for the ongoing
 > commit process to complete before modifying.
 > 
 > >  > So
 > >  > now I don't have a good idea to fix this problem either.  Perhaps we
 > >  > need to rethink the necessity of this waiting, or find a way to avoid
 > >  > acquiring i_data_sem during fast commit.
 > 
 > Ha, the solution seems to have already been listed in the TODOs in
 > fast_commit.c.
 > 
 >   Change ext4_fc_commit() to lookup logical to physical mapping using extent
 >   status tree. This would get rid of the need to call ext4_fc_track_inode()
 >   before acquiring i_data_sem. To do that we would need to ensure that
 >   modified extents from the extent status tree are not evicted from memory.
 > 
 > Alternatively, recording the mapped range of tracking might also be
 > feasible.

Thanks a lot for your insights!

For the next revesion, I plan to follow the "Alternatively" way firstly:
record the mapped ranges (and relvant inode metadata) at commit time in a
snapshot, when journal updates are locked/handles are drained, and then
consume only the snapshot during log writing. This avoids doing
logical-to-physical mapping (and thus avoids taking i_data_sem) in the log
writing phase, and removes the need for ext4_fc_track_inode() to wait for
EXT4_STATE_FC_COMMITTING.

I did not pick the extent status tree approach because it would require
additional work to guarantee the needed mappings are resident and not
evicted under memory pressure, which seems like a larger correctness
surface(Please correct me if I'm wrong). If you believe the extent stats tree
approach is better, please let me know, and I will do my best to implement it.

Thanks again for the guidance. I'll post an RFC v3 later.

Regards,
Li​

Re: [RFC v3 1/2] ext4: fast_commit: assert i_data_sem only before sleep

[Zhang Yi]

On 1/7/2026 10:30 PM, Li Chen wrote:
> Hi Zhang,
> 
> Thanks a lot for your detailed review!
> 
>  ---- On Wed, 07 Jan 2026 10:00:23 +0800  Zhang Yi <yi.zhang@huaweicloud.com> wrote --- 
>  > On 1/6/2026 8:18 PM, Li Chen wrote:
>  > > Hi Zhang Yi,
>  > > 
>  > >  ---- On Mon, 05 Jan 2026 20:18:42 +0800  Zhang Yi <yi.zhang@huaweicloud.com> wrote --- 
>  > >  > Hi Li,
>  > >  > 
>  > >  > On 12/24/2025 11:29 AM, Li Chen wrote:
>  > >  > > ext4_fc_track_inode() can return without sleeping when
>  > >  > > EXT4_STATE_FC_COMMITTING is already clear. The lockdep assertion for
>  > >  > > ei->i_data_sem was done unconditionally before the wait loop, which can
>  > >  > > WARN in call paths that hold i_data_sem even though we never block. Move
>  > >  > > lockdep_assert_not_held(&ei->i_data_sem) into the actual sleep path,
>  > >  > > right before schedule().
>  > >  > > 
>  > >  > > Signed-off-by: Li Chen <me@linux.beauty>
>  > >  > 
>  > >  > Thank you for the fix patch! However, the solution does not seem to fix
>  > >  > the issue. IIUC, the root cause of this issue is the following race
>  > >  > condition (show only one case), and it may cause a real ABBA dead lock
>  > >  > issue.
>  > >  > 
>  > >  > ext4_map_blocks()
>  > >  >  hold i_data_sem // <- A
>  > >  >  ext4_mb_new_blocks()
>  > >  >   ext4_dirty_inode()
>  > >  >                                  ext4_fc_commit()
>  > >  >                                   ext4_fc_perform_commit()
>  > >  >                                    set EXT4_STATE_FC_COMMITTING  <-B
>  > >  >                                    ext4_fc_write_inode_data()
>  > >  >                                    ext4_map_blocks()
>  > >  >                                     hold i_data_sem  // <- A
>  > >  >    ext4_fc_track_inode()
>  > >  >     wait EXT4_STATE_FC_COMMITTING  <- B
>  > >  >                                   jbd2_fc_end_commit()
>  > >  >                                    ext4_fc_cleanup()
>  > >  >                                     clear EXT4_STATE_FC_COMMITTING()
>  > >  > 
>  
> I think the ABBA reasoning is plausible: if a caller violates the ordering
> contract and enters ext4_fc_track_inode() while holding i_data_sem, and the
> recheck still finds EXT4_STATE_FC_COMMITTING set (so we actually schedule()),
> then we can get A -> wait(B). If the commit task, while holding the inode
> in COMMITTING, still needs i_data_sem (e.g. via mapping/log writing), that
> gives B -> wait(A), forming a cycle.
> 
>  > >  > Postponing the lockdep assertion to the point where sleeping is actually
>  > >  > necessary does not resolve this deadlock issue, it merely masks the
>  > >  > problem, right?
>  > >  > 
>  > >  > I currently don't quite understand why only ext4_fc_track_inode() needs
>  > >  > to wait for the inode being fast committed to be completed, instead of
>  > >  > adding it to the FC_Q_STAGING list like other tracking operations.
>  > 
>  > It seems that the inode metadata of the tracked inode was not recorded
>  > during the __track_inode(), so the inode metadata committed at commit
>  > time reflects real-time data. However, the current
>  > ext4_fc_perform_commit() lacks concurrency control, allowing other
>  > processes to simultaneously initiate new handles that modify the inode
>  > metadata while the previous metadata is being fast committed. Therefore,
>  > to prevent recording newly changed inode metadata during the old commit
>  > phase, the ext4_fc_track_inode() function must wait for the ongoing
>  > commit process to complete before modifying.
>  > 
>  > >  > So
>  > >  > now I don't have a good idea to fix this problem either.  Perhaps we
>  > >  > need to rethink the necessity of this waiting, or find a way to avoid
>  > >  > acquiring i_data_sem during fast commit.
>  > 
>  > Ha, the solution seems to have already been listed in the TODOs in
>  > fast_commit.c.
>  > 
>  >   Change ext4_fc_commit() to lookup logical to physical mapping using extent
>  >   status tree. This would get rid of the need to call ext4_fc_track_inode()
>  >   before acquiring i_data_sem. To do that we would need to ensure that
>  >   modified extents from the extent status tree are not evicted from memory.
>  > 
>  > Alternatively, recording the mapped range of tracking might also be
>  > feasible.
> 
> Thanks a lot for your insights!
> 
> For the next revesion, I plan to follow the "Alternatively" way firstly:
> record the mapped ranges (and relvant inode metadata) at commit time in a
> snapshot, when journal updates are locked/handles are drained, and then
> consume only the snapshot during log writing. This avoids doing
> logical-to-physical mapping (and thus avoids taking i_data_sem) in the log
> writing phase, and removes the need for ext4_fc_track_inode() to wait for
> EXT4_STATE_FC_COMMITTING.
> 
> I did not pick the extent status tree approach because it would require
> additional work to guarantee the needed mappings are resident and not
> evicted under memory pressure, which seems like a larger correctness
> surface(Please correct me if I'm wrong). If you believe the extent stats tree
> approach is better, please let me know, and I will do my best to implement it.
> 
> Thanks again for the guidance. I'll post an RFC v3 later.

Yes, in my opinion, I also prefer the solution of recording the mapped ranges,
as it should not incur too much overhead. Please give it a try.

Cheers,
Yi.

> 
> Regards,
> Li​
>