ext4: Fix deadlock on inode reallocation

Currently there is a race in ext4 when reallocating freed inode resulting in a deadlock:

Task1 Task2
ext4_evict_inode()
handle = ext4_journal_start();
...
if (IS_SYNC(inode))

    handle->h_sync = 1;

ext4_free_inode()
ext4_new_inode()
handle = ext4_journal_start() finds the bit in inode bitmap
already clear
insert_inode_locked()
waits for inode to be
removed from the hash.
ext4_journal_stop(handle)
jbd2_journal_stop(handle)
jbd2_log_wait_commit(journal, tid);
- deadlocks waiting for transaction handle Task2 holds

Fix the problem by removing inode from the hash already in ext4_clear_inode() by which time all IO for the inode is done so reuse is already fine but we are still before possibly blocking on transaction commit.

Reported-by "Lai, Yi" <[email protected]>

Link https://lore.kernel.org/all/abNvb2PcrKj1FBeC@ly-workstation open here

Fixes 88ec797c4680 ("fs: make insert_inode_locked() wait for inode destruction")

CC [email protected]

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/super.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Ted, this is a regression recently introduced by VFS changes in insert_inode_locked() but I think it's best fixed in ext4. If you agree, it would be nice to merge this so that it makes it to 7.0 release. Thanks!

fs/ext4/super.c +21 -1

index 43f680c750ae..b8122d24c083 100644
@@ -1527,6 +1527,27 @@ void ext4_clear_inode(struct inode *inode)
        invalidate_inode_buffers(inode);
        clear_inode(inode);
        ext4_discard_preallocations(inode);
+       /*
+        * We must remove the inode from the hash before ext4_free_inode()
+        * clears the bit in inode bitmap as otherwise another process reusing
+        * the inode will block in insert_inode_hash() waiting for inode
+        * eviction to complete while holding transaction handle open, but
+        * ext4_evict_inode() still running for that inode could block waiting
+        * for transaction commit if the inode is marked as IS_SYNC => deadlock.
+        *
+        * Removing the inode from the hash here is safe. There are two cases
+        * to consider:
+        * 1) The inode still has references to it (i_nlink > 0). In that case
+        * we are keeping the inode and once we remove the inode from the hash,
+        * iget() can create the new inode structure for the same inode number
+        * and we are fine with that as all IO on behalf of the inode is
+        * finished.
+        * 2) We are deleting the inode (i_nlink == 0). In that case inode
+        * number cannot be reused until ext4_free_inode() clears the bit in
+        * the inode bitmap, at which point all IO is done and reuse is fine
+        * again.
+        */
+       remove_inode_hash(inode);
        ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
        dquot_drop(inode);
        if (EXT4_I(inode)->jinode) {
--

2.51.0

can this get any traction?

master as-is contains a change which uncovered the ext4 problem. either the deadlock needs to be fixed in time for the release or the other change needs to get temporarily reverted.

interested parties can find proposed revert here:
https://lore.kernel.org/linux-fsdevel/[email protected]/

Quoted 76 lines · Jan Kara

Currently there is a race in ext4 when reallocating freed inode resulting in a deadlock:

Task1 Task2
ext4_evict_inode()
handle = ext4_journal_start();
...
if (IS_SYNC(inode))

    handle->h_sync = 1;

ext4_free_inode()
ext4_new_inode()
handle = ext4_journal_start() finds the bit in inode bitmap
already clear
insert_inode_locked()
waits for inode to be
removed from the hash.
ext4_journal_stop(handle)
jbd2_journal_stop(handle)
jbd2_log_wait_commit(journal, tid);
- deadlocks waiting for transaction handle Task2 holds

Fix the problem by removing inode from the hash already in ext4_clear_inode() by which time all IO for the inode is done so reuse is already fine but we are still before possibly blocking on transaction commit.

Reported-by "Lai, Yi" <[email protected]>

Link https://lore.kernel.org/all/abNvb2PcrKj1FBeC@ly-workstation open here

Fixes 88ec797c4680 ("fs: make insert_inode_locked() wait for inode destruction")

CC [email protected]

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/super.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Ted, this is a regression recently introduced by VFS changes in insert_inode_locked() but I think it's best fixed in ext4. If you agree, it would be nice to merge this so that it makes it to 7.0 release. Thanks!

fs/ext4/super.c +21 -1

index 43f680c750ae..b8122d24c083 100644
@@ -1527,6 +1527,27 @@ void ext4_clear_inode(struct inode *inode)
        invalidate_inode_buffers(inode);
        clear_inode(inode);
        ext4_discard_preallocations(inode);
+       /*
+        * We must remove the inode from the hash before ext4_free_inode()
+        * clears the bit in inode bitmap as otherwise another process reusing
+        * the inode will block in insert_inode_hash() waiting for inode
+        * eviction to complete while holding transaction handle open, but
+        * ext4_evict_inode() still running for that inode could block waiting
+        * for transaction commit if the inode is marked as IS_SYNC => deadlock.
+        *
+        * Removing the inode from the hash here is safe. There are two cases
+        * to consider:
+        * 1) The inode still has references to it (i_nlink > 0). In that case
+        * we are keeping the inode and once we remove the inode from the hash,
+        * iget() can create the new inode structure for the same inode number
+        * and we are fine with that as all IO on behalf of the inode is
+        * finished.
+        * 2) We are deleting the inode (i_nlink == 0). In that case inode
+        * number cannot be reused until ext4_free_inode() clears the bit in
+        * the inode bitmap, at which point all IO is done and reuse is fine
+        * again.
+        */
+       remove_inode_hash(inode);
        ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
        dquot_drop(inode);
        if (EXT4_I(inode)->jinode) {
--

2.51.0

Currently there is a race in ext4 when reallocating freed inode resulting in a deadlock:

Task1 Task2
ext4_evict_inode()
handle = ext4_journal_start();
...
if (IS_SYNC(inode))

    handle->h_sync = 1;

ext4_free_inode()
ext4_new_inode()
handle = ext4_journal_start() finds the bit in inode bitmap
already clear
insert_inode_locked()
waits for inode to be
removed from the hash.
ext4_journal_stop(handle)
jbd2_journal_stop(handle)
jbd2_log_wait_commit(journal, tid);
- deadlocks waiting for transaction handle Task2 holds

[...]