[PATCH v2 1/2] ocfs2: exit recovery thread on mount error path

[PATCH v2 1/2] ocfs2: exit recovery thread on mount error path

[Joseph Qi]

When a mount fails after the cluster connection has been established,
e.g. in ocfs2_mount_volume(), ocfs2_fill_super() unwinds via
out_debugfs/out_super and frees the osb without disabling recovery.

A node failure event can concurrently launch the recovery thread, which
blocks in __ocfs2_wait_on_mount() waiting for the volume state to
become VOLUME_MOUNTED or VOLUME_DISABLED.  As the mount error path
neither sets VOLUME_DISABLED nor wakes osb_mount_event, the thread can
never make progress: the kthread leaks and stays blocked on the wait
queue embedded in the freed osb, which may then be accessed as freed
memory.

Fix it by setting VOLUME_DISABLED and waking osb_mount_event on this
path so the thread bails out, and replace the plain
kfree(osb->recovery_map) with ocfs2_recovery_exit(), which waits for a
running recovery thread to exit before the recovery map is freed.

Fixes: f1e75d128b46 ("ocfs2: rewrite error handling of ocfs2_fill_super")
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/super.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index c62e389d4dd6..f785c39d1fb8 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1169,8 +1169,17 @@ static int ocfs2_fill_super(struct super_block *sb, struct fs_context *fc)
 out_debugfs:
 	debugfs_remove_recursive(osb->osb_debug_root);
 out_super:
+	/*
+	 * A recovery thread launched by a node failure event may still be
+	 * waiting for the volume to be mounted.  Set VOLUME_DISABLED and
+	 * wake it up, then wait for it to exit before osb is freed,
+	 * otherwise the kthread would leak and stay blocked on the wait
+	 * queue embedded in the freed osb.
+	 */
+	atomic_set(&osb->vol_state, VOLUME_DISABLED);
+	wake_up(&osb->osb_mount_event);
 	ocfs2_release_system_inodes(osb);
-	kfree(osb->recovery_map);
+	ocfs2_recovery_exit(osb);
 	ocfs2_delete_osb(osb);
 	kfree(osb);
 out:
-- 
2.39.3

[PATCH v2 2/2] ocfs2: defer suballocator block group reclaim to workqueue

[Joseph Qi]

When the last bit in a suballocator block group is freed,
_ocfs2_free_suballoc_bits() reclaims the group back to the global
bitmap.  The reclaim takes inode_lock() on the global bitmap inode
while running inside the freeing transaction, adding a lock
dependency of

  j_trans_barrier -> global bitmap inode i_rwsem

This forms a circular dependency with paths such as
ocfs2_shutdown_local_alloc(), which take the global bitmap inode
lock before starting a transaction:

  Task1 (dealloc):
    ocfs2_run_deallocs
      ocfs2_free_cached_blocks
        ocfs2_start_trans
          down_read(j_trans_barrier)
        _ocfs2_free_suballoc_bits
          _ocfs2_reclaim_suballoc_to_main
            inode_lock(main_bm_inode)  <- wait on Task2

  Task2 (dismount):
    ocfs2_shutdown_local_alloc
      inode_lock(main_bm_inode)
      ocfs2_start_trans
        down_read(j_trans_barrier)  <- wait on Task3

  Task3 (ocfs2cmt):
    ocfs2_commit_cache
      down_write(j_trans_barrier)  <- wait on Task1's handle
      jbd2_journal_flush

Task1 waits for Task2's inode_lock(), Task2 waits for the
j_trans_barrier down_write() held by ocfs2cmt, and ocfs2cmt waits
for Task1's running transaction to commit - a real deadlock,
observed with aio-stress direct IO writes racing dismount.

Fix it by deferring the reclaim to the per-superblock ocfs2_wq
workqueue, so the freeing transaction no longer takes the global
bitmap inode lock.  The worker re-checks under the suballocator
locks that the block group is still fully freed (it may have been
allocated from again in the meantime), takes the global bitmap
inode locks before starting its own transaction, and performs the
same suballocator cleanup and space return.  The inode lock order
(suballocator inode -> global bitmap inode) is consistent with the
existing "inode lock before transaction" order, breaking the cycle.

Reclaim work can still be queued late in dismount, e.g. when the
truncate log is flushed or orphan dir recovery frees inode bits, so
both ocfs2_dismount_volume() and the mount error path flush
ocfs2_wq right before the system inodes are released, while the
journal is still alive, to make sure no reclaim work is left
running.  The worker also bails out if the journal is already gone.

Tested with the ocfs2 testsuite (including aio-stress direct IO)
and umount/mount cycles on a CONFIG_PROVE_LOCKING kernel: the
circular locking dependency is gone and freed block groups are
still returned to the global bitmap.

Fixes: 4a54331616b3 ("ocfs2: give ocfs2 the ability to reclaim suballocator free bg")
Assisted-by: Qoder:Qwen3.8-Max
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/ocfs2.h    |   5 ++
 fs/ocfs2/suballoc.c | 202 +++++++++++++++++++++++++++++++++++++-------
 fs/ocfs2/suballoc.h |   2 +-
 fs/ocfs2/super.c    |  16 ++++
 4 files changed, 195 insertions(+), 30 deletions(-)

diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index 62cad6522c7a..b747cdec1787 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -502,6 +502,11 @@ struct ocfs2_super
 	 */
 	struct workqueue_struct *ocfs2_wq;
 
+	/* deferred reclaim of fully freed suballocator block groups */
+	spinlock_t os_suballoc_reclaim_lock;
+	struct list_head os_suballoc_reclaim_list;
+	struct work_struct os_suballoc_reclaim_work;
+
 	/* sysfs directory per partition */
 	struct kset *osb_dev_kset;
 
diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index 20c3aec6b987..453b56be9624 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -2687,16 +2687,24 @@ static int ocfs2_block_group_clear_bits(handle_t *handle,
  * cleanup rec/alloc_inode job, then switches to the main bitmap
  * to reclaim released space.
  *
+ * Callers must hold inode_lock() and ocfs2_inode_lock() on
+ * main_bm_inode, i.e. the global bitmap inode locks must be taken
+ * before starting the transaction.
+ *
  * handle: The transaction handle
  * alloc_inode: The suballoc inode
  * alloc_bh: The buffer_head of suballoc inode
  * group_bh: The group descriptor buffer_head of suballocator managed.
- *           Caller should release the input group_bh.
+ *           This function takes ownership of it and will release it.
+ * main_bm_inode: The global bitmap inode
+ * main_bm_bh: The buffer_head of the global bitmap inode
  */
 static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 			struct inode *alloc_inode,
 			struct buffer_head *alloc_bh,
-			struct buffer_head *group_bh)
+			struct buffer_head *group_bh,
+			struct inode *main_bm_inode,
+			struct buffer_head *main_bm_bh)
 {
 	int idx, status = 0;
 	int i, next_free_rec, len = 0;
@@ -2706,8 +2714,6 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 	u64 bg_blkno, start_blk;
 	unsigned int count;
 	struct ocfs2_chain_rec *rec;
-	struct buffer_head *main_bm_bh = NULL;
-	struct inode *main_bm_inode = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(alloc_inode->i_sb);
 	struct ocfs2_dinode *fe = (struct ocfs2_dinode *) alloc_bh->b_data;
 	struct ocfs2_chain_list *cl = &fe->id2.i_chain;
@@ -2794,24 +2800,12 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 	ocfs2_remove_from_cache(INODE_CACHE(alloc_inode), group_bh);
 	memset(group, 0, sizeof(struct ocfs2_group_desc));
 
-	/* prepare job for reclaim clusters */
-	main_bm_inode = ocfs2_get_system_file_inode(osb,
-						    GLOBAL_BITMAP_SYSTEM_INODE,
-						    OCFS2_INVALID_SLOT);
-	if (!main_bm_inode)
-		goto bail; /* ignore the error in reclaim path */
-
-	inode_lock(main_bm_inode);
-
-	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
-	if (status < 0)
-		goto free_bm_inode; /* ignore the error in reclaim path */
-
 	ocfs2_block_to_cluster_group(main_bm_inode, start_blk, &bg_blkno,
 				     &start_bit);
 	fe = (struct ocfs2_dinode *) main_bm_bh->b_data;
 	cl = &fe->id2.i_chain;
-	/* reuse group_bh, caller will release the input group_bh */
+	/* release the suballocator group descriptor before reuse */
+	brelse(group_bh);
 	group_bh = NULL;
 
 	/* reclaim clusters to global_bitmap */
@@ -2819,7 +2813,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 					     &group_bh);
 	if (status < 0) {
 		mlog_errno(status);
-		goto free_bm_bh;
+		goto bail;
 	}
 	group = (struct ocfs2_group_desc *) group_bh->b_data;
 
@@ -2827,7 +2821,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 		ocfs2_error(alloc_inode->i_sb,
 			"reclaim length (%d) beyands block group length (%d)",
 			count + start_bit, le16_to_cpu(group->bg_bits));
-		goto free_group_bh;
+		goto bail;
 	}
 
 	old_bg_contig_free_bits = group->bg_contig_free_bits;
@@ -2837,7 +2831,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 					      _ocfs2_clear_bit);
 	if (status < 0) {
 		mlog_errno(status);
-		goto free_group_bh;
+		goto bail;
 	}
 
 	status = ocfs2_journal_access_di(handle, INODE_CACHE(main_bm_inode),
@@ -2847,7 +2841,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 		ocfs2_block_group_set_bits(handle, main_bm_inode, group, group_bh,
 				start_bit, count,
 				le16_to_cpu(old_bg_contig_free_bits), 1);
-		goto free_group_bh;
+		goto bail;
 	}
 
 	idx = le16_to_cpu(group->bg_chain);
@@ -2858,19 +2852,168 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 	fe->id1.bitmap1.i_used = cpu_to_le32(tmp_used - count);
 	ocfs2_journal_dirty(handle, main_bm_bh);
 
-free_group_bh:
+bail:
 	brelse(group_bh);
+	return status;
+}
+
+/*
+ * When a suballocator block group becomes fully freed, its space is
+ * reclaimed back to the global bitmap. Taking the global bitmap inode
+ * lock inside the freeing transaction would create a lock dependency
+ * of "j_trans_barrier -> global bitmap inode i_rwsem", which forms a
+ * circular dependency with paths like ocfs2_shutdown_local_alloc() that
+ * take the inode lock before starting a transaction, and can lead to a
+ * real deadlock with the ocfs2cmt journal commit thread. So queue the
+ * reclaim to the workqueue and let it run outside the freeing
+ * transaction.
+ */
+struct ocfs2_suballoc_reclaim_work {
+	struct list_head list;
+	struct inode *alloc_inode;
+	u64 bg_blkno;
+};
+
+static void ocfs2_queue_suballoc_reclaim(struct ocfs2_super *osb,
+					 struct inode *alloc_inode,
+					 u64 bg_blkno)
+{
+	struct ocfs2_suballoc_reclaim_work *reclaim_work;
+
+	reclaim_work = kmalloc_obj(*reclaim_work, GFP_NOFS);
+	if (!reclaim_work) {
+		/*
+		 * Reclaim is only a space return optimization. If we can't
+		 * queue it, the freed block group just stays owned by the
+		 * suballocator.
+		 */
+		return;
+	}
+
+	igrab(alloc_inode);
+	reclaim_work->alloc_inode = alloc_inode;
+	reclaim_work->bg_blkno = bg_blkno;
+
+	spin_lock(&osb->os_suballoc_reclaim_lock);
+	list_add_tail(&reclaim_work->list, &osb->os_suballoc_reclaim_list);
+	spin_unlock(&osb->os_suballoc_reclaim_lock);
 
-free_bm_bh:
+	queue_work(osb->ocfs2_wq, &osb->os_suballoc_reclaim_work);
+}
+
+static void ocfs2_do_suballoc_reclaim(struct ocfs2_super *osb,
+				struct ocfs2_suballoc_reclaim_work *reclaim_work)
+{
+	int status, i;
+	handle_t *handle;
+	struct inode *alloc_inode = reclaim_work->alloc_inode;
+	struct inode *main_bm_inode;
+	struct buffer_head *alloc_bh = NULL, *group_bh = NULL;
+	struct buffer_head *main_bm_bh = NULL;
+	struct ocfs2_dinode *fe;
+	struct ocfs2_chain_list *cl;
+	struct ocfs2_chain_rec *rec;
+
+	/* journal already gone, e.g. during dismount cleanup */
+	if (!osb->journal)
+		return;
+
+	inode_lock(alloc_inode);
+	status = ocfs2_inode_lock(alloc_inode, &alloc_bh, 1);
+	if (status < 0)
+		goto out_alloc;
+
+	fe = (struct ocfs2_dinode *) alloc_bh->b_data;
+	cl = &fe->id2.i_chain;
+
+	/*
+	 * The block group may have been allocated from again since the
+	 * reclaim work was queued, re-check that it is still fully freed.
+	 * A stale work item can also reference a group that is no longer
+	 * chained, whose descriptor would fail validation and trigger a
+	 * spurious ocfs2_error(), so verify chain membership first.
+	 */
+	for (i = 0; i < le16_to_cpu(cl->cl_next_free_rec); i++) {
+		rec = &cl->cl_recs[i];
+		if (le64_to_cpu(rec->c_blkno) == reclaim_work->bg_blkno)
+			break;
+	}
+	if (i == le16_to_cpu(cl->cl_next_free_rec) ||
+	    ocfs2_is_cluster_bitmap(alloc_inode) ||
+	    (le32_to_cpu(rec->c_free) != (le32_to_cpu(rec->c_total) - 1)) ||
+	    (le16_to_cpu(cl->cl_next_free_rec) == 1))
+		goto out_alloc_unlock;
+
+	status = ocfs2_read_group_descriptor(alloc_inode, fe,
+					     reclaim_work->bg_blkno, &group_bh);
+	if (status < 0)
+		goto out_alloc_unlock;
+
+	main_bm_inode = ocfs2_get_system_file_inode(osb,
+						    GLOBAL_BITMAP_SYSTEM_INODE,
+						    OCFS2_INVALID_SLOT);
+	if (!main_bm_inode)
+		goto out_group;
+
+	inode_lock(main_bm_inode);
+	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
+	if (status < 0)
+		goto out_main;
+
+	handle = ocfs2_start_trans(osb, OCFS2_SUBALLOC_FREE);
+	if (IS_ERR(handle)) {
+		status = PTR_ERR(handle);
+		mlog_errno(status);
+		goto out_main_unlock;
+	}
+
+	status = _ocfs2_reclaim_suballoc_to_main(handle, alloc_inode,
+						 alloc_bh, group_bh,
+						 main_bm_inode, main_bm_bh);
+	/* group_bh ownership passed to _ocfs2_reclaim_suballoc_to_main() */
+	group_bh = NULL;
+	if (status < 0)
+		mlog_errno(status);
+
+	ocfs2_commit_trans(osb, handle);
+
+out_main_unlock:
 	ocfs2_inode_unlock(main_bm_inode, 1);
 	brelse(main_bm_bh);
-
-free_bm_inode:
+out_main:
 	inode_unlock(main_bm_inode);
 	iput(main_bm_inode);
+out_group:
+	brelse(group_bh);
+out_alloc_unlock:
+	ocfs2_inode_unlock(alloc_inode, 1);
+	brelse(alloc_bh);
+out_alloc:
+	inode_unlock(alloc_inode);
+}
 
-bail:
-	return status;
+void ocfs2_suballoc_reclaim_worker(struct work_struct *work)
+{
+	struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
+					       os_suballoc_reclaim_work);
+	struct ocfs2_suballoc_reclaim_work *reclaim_work;
+
+	while (1) {
+		spin_lock(&osb->os_suballoc_reclaim_lock);
+		if (list_empty(&osb->os_suballoc_reclaim_list)) {
+			spin_unlock(&osb->os_suballoc_reclaim_lock);
+			break;
+		}
+		reclaim_work = list_first_entry(&osb->os_suballoc_reclaim_list,
+					struct ocfs2_suballoc_reclaim_work,
+					list);
+		list_del(&reclaim_work->list);
+		spin_unlock(&osb->os_suballoc_reclaim_lock);
+
+		ocfs2_do_suballoc_reclaim(osb, reclaim_work);
+		iput(reclaim_work->alloc_inode);
+		kfree(reclaim_work);
+	}
 }
 
 /*
@@ -2955,7 +3098,8 @@ static int _ocfs2_free_suballoc_bits(handle_t *handle,
 		goto bail;
 	}
 
-	_ocfs2_reclaim_suballoc_to_main(handle, alloc_inode, alloc_bh, group_bh);
+	ocfs2_queue_suballoc_reclaim(OCFS2_SB(alloc_inode->i_sb), alloc_inode,
+				     bg_blkno);
 
 bail:
 	brelse(group_bh);
diff --git a/fs/ocfs2/suballoc.h b/fs/ocfs2/suballoc.h
index bcf2ed4a8631..6042abc032f9 100644
--- a/fs/ocfs2/suballoc.h
+++ b/fs/ocfs2/suballoc.h
@@ -206,7 +206,7 @@ int ocfs2_lock_allocators(struct inode *inode, struct ocfs2_extent_tree *et,
 
 int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res);
 
-
+void ocfs2_suballoc_reclaim_worker(struct work_struct *work);
 
 /*
  * The following two interfaces are for ocfs2_create_inode_in_orphan().
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index f785c39d1fb8..b7c29d3c0321 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1787,6 +1787,9 @@ static int ocfs2_mount_volume(struct super_block *sb)
 out_system_inodes:
 	if (osb->local_alloc_state == OCFS2_LA_ENABLED)
 		ocfs2_shutdown_local_alloc(osb);
+	/* Drain pending suballoc reclaim work before the journal goes away */
+	if (osb->ocfs2_wq)
+		flush_workqueue(osb->ocfs2_wq);
 	ocfs2_release_system_inodes(osb);
 	/* before journal shutdown, we should release slot_info */
 	ocfs2_free_slot_info(osb);
@@ -1857,6 +1860,14 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
 	if (osb->cconn)
 		ocfs2_super_unlock(osb, 1);
 
+	/*
+	 * Drain pending suballoc reclaim work while the system inodes and
+	 * the journal are still alive, since the worker needs to look up
+	 * the global bitmap inode and start a transaction.
+	 */
+	if (osb->ocfs2_wq)
+		flush_workqueue(osb->ocfs2_wq);
+
 	ocfs2_release_system_inodes(osb);
 
 	ocfs2_journal_shutdown(osb);
@@ -2143,6 +2154,11 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
 	init_llist_head(&osb->dquot_drop_list);
 
+	spin_lock_init(&osb->os_suballoc_reclaim_lock);
+	INIT_LIST_HEAD(&osb->os_suballoc_reclaim_list);
+	INIT_WORK(&osb->os_suballoc_reclaim_work,
+		  ocfs2_suballoc_reclaim_worker);
+
 	/* get some pseudo constants for clustersize bits */
 	osb->s_clustersize_bits =
 		le32_to_cpu(di->id2.i_super.s_clustersize_bits);
-- 
2.39.3

Re: [PATCH v2 1/2] ocfs2: exit recovery thread on mount error path

[Heming Zhao]

On Fri, Aug 28, 2026 at 03:15:52PM +0800, Joseph Qi wrote:
> When a mount fails after the cluster connection has been established,
> e.g. in ocfs2_mount_volume(), ocfs2_fill_super() unwinds via
> out_debugfs/out_super and frees the osb without disabling recovery.
> 
> A node failure event can concurrently launch the recovery thread, which
> blocks in __ocfs2_wait_on_mount() waiting for the volume state to
> become VOLUME_MOUNTED or VOLUME_DISABLED.  As the mount error path
> neither sets VOLUME_DISABLED nor wakes osb_mount_event, the thread can
> never make progress: the kthread leaks and stays blocked on the wait
> queue embedded in the freed osb, which may then be accessed as freed
> memory.
> 
> Fix it by setting VOLUME_DISABLED and waking osb_mount_event on this
> path so the thread bails out, and replace the plain
> kfree(osb->recovery_map) with ocfs2_recovery_exit(), which waits for a
> running recovery thread to exit before the recovery map is freed.
> 
> Fixes: f1e75d128b46 ("ocfs2: rewrite error handling of ocfs2_fill_super")
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

LGTM
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  fs/ocfs2/super.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index c62e389d4dd6..f785c39d1fb8 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -1169,8 +1169,17 @@ static int ocfs2_fill_super(struct super_block *sb, struct fs_context *fc)
>  out_debugfs:
>  	debugfs_remove_recursive(osb->osb_debug_root);
>  out_super:
> +	/*
> +	 * A recovery thread launched by a node failure event may still be
> +	 * waiting for the volume to be mounted.  Set VOLUME_DISABLED and
> +	 * wake it up, then wait for it to exit before osb is freed,
> +	 * otherwise the kthread would leak and stay blocked on the wait
> +	 * queue embedded in the freed osb.
> +	 */
> +	atomic_set(&osb->vol_state, VOLUME_DISABLED);
> +	wake_up(&osb->osb_mount_event);
>  	ocfs2_release_system_inodes(osb);
> -	kfree(osb->recovery_map);
> +	ocfs2_recovery_exit(osb);
>  	ocfs2_delete_osb(osb);
>  	kfree(osb);
>  out:
> -- 
> 2.39.3
> 

Re: [PATCH v2 2/2] ocfs2: defer suballocator block group reclaim to workqueue

[Heming Zhao]

On Fri, Aug 28, 2026 at 03:15:53PM +0800, Joseph Qi wrote:
> When the last bit in a suballocator block group is freed,
> _ocfs2_free_suballoc_bits() reclaims the group back to the global
> bitmap.  The reclaim takes inode_lock() on the global bitmap inode
> while running inside the freeing transaction, adding a lock
> dependency of
> 
>   j_trans_barrier -> global bitmap inode i_rwsem
> 
> This forms a circular dependency with paths such as
> ocfs2_shutdown_local_alloc(), which take the global bitmap inode
> lock before starting a transaction:
> 
>   Task1 (dealloc):
>     ocfs2_run_deallocs
>       ocfs2_free_cached_blocks
>         ocfs2_start_trans
>           down_read(j_trans_barrier)
>         _ocfs2_free_suballoc_bits
>           _ocfs2_reclaim_suballoc_to_main
>             inode_lock(main_bm_inode)  <- wait on Task2
> 
>   Task2 (dismount):
>     ocfs2_shutdown_local_alloc
>       inode_lock(main_bm_inode)
>       ocfs2_start_trans
>         down_read(j_trans_barrier)  <- wait on Task3
> 
>   Task3 (ocfs2cmt):
>     ocfs2_commit_cache
>       down_write(j_trans_barrier)  <- wait on Task1's handle
>       jbd2_journal_flush
> 
> Task1 waits for Task2's inode_lock(), Task2 waits for the
> j_trans_barrier down_write() held by ocfs2cmt, and ocfs2cmt waits
> for Task1's running transaction to commit - a real deadlock,
> observed with aio-stress direct IO writes racing dismount.
> 
> Fix it by deferring the reclaim to the per-superblock ocfs2_wq
> workqueue, so the freeing transaction no longer takes the global
> bitmap inode lock.  The worker re-checks under the suballocator
> locks that the block group is still fully freed (it may have been
> allocated from again in the meantime), takes the global bitmap
> inode locks before starting its own transaction, and performs the
> same suballocator cleanup and space return.  The inode lock order
> (suballocator inode -> global bitmap inode) is consistent with the
> existing "inode lock before transaction" order, breaking the cycle.
> 
> Reclaim work can still be queued late in dismount, e.g. when the
> truncate log is flushed or orphan dir recovery frees inode bits, so
> both ocfs2_dismount_volume() and the mount error path flush
> ocfs2_wq right before the system inodes are released, while the
> journal is still alive, to make sure no reclaim work is left
> running.  The worker also bails out if the journal is already gone.
> 
> Tested with the ocfs2 testsuite (including aio-stress direct IO)
> and umount/mount cycles on a CONFIG_PROVE_LOCKING kernel: the
> circular locking dependency is gone and freed block groups are
> still returned to the global bitmap.
> 
> Fixes: 4a54331616b3 ("ocfs2: give ocfs2 the ability to reclaim suballocator free bg")
> Assisted-by: Qoder:Qwen3.8-Max
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

LGTM
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  fs/ocfs2/ocfs2.h    |   5 ++
>  fs/ocfs2/suballoc.c | 202 +++++++++++++++++++++++++++++++++++++-------
>  fs/ocfs2/suballoc.h |   2 +-
>  fs/ocfs2/super.c    |  16 ++++
>  4 files changed, 195 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
> index 62cad6522c7a..b747cdec1787 100644
> --- a/fs/ocfs2/ocfs2.h
> +++ b/fs/ocfs2/ocfs2.h
> @@ -502,6 +502,11 @@ struct ocfs2_super
>  	 */
>  	struct workqueue_struct *ocfs2_wq;
>  
> +	/* deferred reclaim of fully freed suballocator block groups */
> +	spinlock_t os_suballoc_reclaim_lock;
> +	struct list_head os_suballoc_reclaim_list;
> +	struct work_struct os_suballoc_reclaim_work;
> +
>  	/* sysfs directory per partition */
>  	struct kset *osb_dev_kset;
>  
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index 20c3aec6b987..453b56be9624 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -2687,16 +2687,24 @@ static int ocfs2_block_group_clear_bits(handle_t *handle,
>   * cleanup rec/alloc_inode job, then switches to the main bitmap
>   * to reclaim released space.
>   *
> + * Callers must hold inode_lock() and ocfs2_inode_lock() on
> + * main_bm_inode, i.e. the global bitmap inode locks must be taken
> + * before starting the transaction.
> + *
>   * handle: The transaction handle
>   * alloc_inode: The suballoc inode
>   * alloc_bh: The buffer_head of suballoc inode
>   * group_bh: The group descriptor buffer_head of suballocator managed.
> - *           Caller should release the input group_bh.
> + *           This function takes ownership of it and will release it.
> + * main_bm_inode: The global bitmap inode
> + * main_bm_bh: The buffer_head of the global bitmap inode
>   */
>  static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  			struct inode *alloc_inode,
>  			struct buffer_head *alloc_bh,
> -			struct buffer_head *group_bh)
> +			struct buffer_head *group_bh,
> +			struct inode *main_bm_inode,
> +			struct buffer_head *main_bm_bh)
>  {
>  	int idx, status = 0;
>  	int i, next_free_rec, len = 0;
> @@ -2706,8 +2714,6 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  	u64 bg_blkno, start_blk;
>  	unsigned int count;
>  	struct ocfs2_chain_rec *rec;
> -	struct buffer_head *main_bm_bh = NULL;
> -	struct inode *main_bm_inode = NULL;
>  	struct ocfs2_super *osb = OCFS2_SB(alloc_inode->i_sb);
>  	struct ocfs2_dinode *fe = (struct ocfs2_dinode *) alloc_bh->b_data;
>  	struct ocfs2_chain_list *cl = &fe->id2.i_chain;
> @@ -2794,24 +2800,12 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  	ocfs2_remove_from_cache(INODE_CACHE(alloc_inode), group_bh);
>  	memset(group, 0, sizeof(struct ocfs2_group_desc));
>  
> -	/* prepare job for reclaim clusters */
> -	main_bm_inode = ocfs2_get_system_file_inode(osb,
> -						    GLOBAL_BITMAP_SYSTEM_INODE,
> -						    OCFS2_INVALID_SLOT);
> -	if (!main_bm_inode)
> -		goto bail; /* ignore the error in reclaim path */
> -
> -	inode_lock(main_bm_inode);
> -
> -	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
> -	if (status < 0)
> -		goto free_bm_inode; /* ignore the error in reclaim path */
> -
>  	ocfs2_block_to_cluster_group(main_bm_inode, start_blk, &bg_blkno,
>  				     &start_bit);
>  	fe = (struct ocfs2_dinode *) main_bm_bh->b_data;
>  	cl = &fe->id2.i_chain;
> -	/* reuse group_bh, caller will release the input group_bh */
> +	/* release the suballocator group descriptor before reuse */
> +	brelse(group_bh);
>  	group_bh = NULL;
>  
>  	/* reclaim clusters to global_bitmap */
> @@ -2819,7 +2813,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  					     &group_bh);
>  	if (status < 0) {
>  		mlog_errno(status);
> -		goto free_bm_bh;
> +		goto bail;
>  	}
>  	group = (struct ocfs2_group_desc *) group_bh->b_data;
>  
> @@ -2827,7 +2821,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  		ocfs2_error(alloc_inode->i_sb,
>  			"reclaim length (%d) beyands block group length (%d)",
>  			count + start_bit, le16_to_cpu(group->bg_bits));
> -		goto free_group_bh;
> +		goto bail;
>  	}
>  
>  	old_bg_contig_free_bits = group->bg_contig_free_bits;
> @@ -2837,7 +2831,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  					      _ocfs2_clear_bit);
>  	if (status < 0) {
>  		mlog_errno(status);
> -		goto free_group_bh;
> +		goto bail;
>  	}
>  
>  	status = ocfs2_journal_access_di(handle, INODE_CACHE(main_bm_inode),
> @@ -2847,7 +2841,7 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  		ocfs2_block_group_set_bits(handle, main_bm_inode, group, group_bh,
>  				start_bit, count,
>  				le16_to_cpu(old_bg_contig_free_bits), 1);
> -		goto free_group_bh;
> +		goto bail;
>  	}
>  
>  	idx = le16_to_cpu(group->bg_chain);
> @@ -2858,19 +2852,168 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  	fe->id1.bitmap1.i_used = cpu_to_le32(tmp_used - count);
>  	ocfs2_journal_dirty(handle, main_bm_bh);
>  
> -free_group_bh:
> +bail:
>  	brelse(group_bh);
> +	return status;
> +}
> +
> +/*
> + * When a suballocator block group becomes fully freed, its space is
> + * reclaimed back to the global bitmap. Taking the global bitmap inode
> + * lock inside the freeing transaction would create a lock dependency
> + * of "j_trans_barrier -> global bitmap inode i_rwsem", which forms a
> + * circular dependency with paths like ocfs2_shutdown_local_alloc() that
> + * take the inode lock before starting a transaction, and can lead to a
> + * real deadlock with the ocfs2cmt journal commit thread. So queue the
> + * reclaim to the workqueue and let it run outside the freeing
> + * transaction.
> + */
> +struct ocfs2_suballoc_reclaim_work {
> +	struct list_head list;
> +	struct inode *alloc_inode;
> +	u64 bg_blkno;
> +};
> +
> +static void ocfs2_queue_suballoc_reclaim(struct ocfs2_super *osb,
> +					 struct inode *alloc_inode,
> +					 u64 bg_blkno)
> +{
> +	struct ocfs2_suballoc_reclaim_work *reclaim_work;
> +
> +	reclaim_work = kmalloc_obj(*reclaim_work, GFP_NOFS);
> +	if (!reclaim_work) {
> +		/*
> +		 * Reclaim is only a space return optimization. If we can't
> +		 * queue it, the freed block group just stays owned by the
> +		 * suballocator.
> +		 */
> +		return;
> +	}
> +
> +	igrab(alloc_inode);
> +	reclaim_work->alloc_inode = alloc_inode;
> +	reclaim_work->bg_blkno = bg_blkno;
> +
> +	spin_lock(&osb->os_suballoc_reclaim_lock);
> +	list_add_tail(&reclaim_work->list, &osb->os_suballoc_reclaim_list);
> +	spin_unlock(&osb->os_suballoc_reclaim_lock);
>  
> -free_bm_bh:
> +	queue_work(osb->ocfs2_wq, &osb->os_suballoc_reclaim_work);
> +}
> +
> +static void ocfs2_do_suballoc_reclaim(struct ocfs2_super *osb,
> +				struct ocfs2_suballoc_reclaim_work *reclaim_work)
> +{
> +	int status, i;
> +	handle_t *handle;
> +	struct inode *alloc_inode = reclaim_work->alloc_inode;
> +	struct inode *main_bm_inode;
> +	struct buffer_head *alloc_bh = NULL, *group_bh = NULL;
> +	struct buffer_head *main_bm_bh = NULL;
> +	struct ocfs2_dinode *fe;
> +	struct ocfs2_chain_list *cl;
> +	struct ocfs2_chain_rec *rec;
> +
> +	/* journal already gone, e.g. during dismount cleanup */
> +	if (!osb->journal)
> +		return;
> +
> +	inode_lock(alloc_inode);
> +	status = ocfs2_inode_lock(alloc_inode, &alloc_bh, 1);
> +	if (status < 0)
> +		goto out_alloc;
> +
> +	fe = (struct ocfs2_dinode *) alloc_bh->b_data;
> +	cl = &fe->id2.i_chain;
> +
> +	/*
> +	 * The block group may have been allocated from again since the
> +	 * reclaim work was queued, re-check that it is still fully freed.
> +	 * A stale work item can also reference a group that is no longer
> +	 * chained, whose descriptor would fail validation and trigger a
> +	 * spurious ocfs2_error(), so verify chain membership first.
> +	 */
> +	for (i = 0; i < le16_to_cpu(cl->cl_next_free_rec); i++) {
> +		rec = &cl->cl_recs[i];
> +		if (le64_to_cpu(rec->c_blkno) == reclaim_work->bg_blkno)
> +			break;
> +	}
> +	if (i == le16_to_cpu(cl->cl_next_free_rec) ||
> +	    ocfs2_is_cluster_bitmap(alloc_inode) ||
> +	    (le32_to_cpu(rec->c_free) != (le32_to_cpu(rec->c_total) - 1)) ||
> +	    (le16_to_cpu(cl->cl_next_free_rec) == 1))
> +		goto out_alloc_unlock;
> +
> +	status = ocfs2_read_group_descriptor(alloc_inode, fe,
> +					     reclaim_work->bg_blkno, &group_bh);
> +	if (status < 0)
> +		goto out_alloc_unlock;
> +
> +	main_bm_inode = ocfs2_get_system_file_inode(osb,
> +						    GLOBAL_BITMAP_SYSTEM_INODE,
> +						    OCFS2_INVALID_SLOT);
> +	if (!main_bm_inode)
> +		goto out_group;
> +
> +	inode_lock(main_bm_inode);
> +	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
> +	if (status < 0)
> +		goto out_main;
> +
> +	handle = ocfs2_start_trans(osb, OCFS2_SUBALLOC_FREE);
> +	if (IS_ERR(handle)) {
> +		status = PTR_ERR(handle);
> +		mlog_errno(status);
> +		goto out_main_unlock;
> +	}
> +
> +	status = _ocfs2_reclaim_suballoc_to_main(handle, alloc_inode,
> +						 alloc_bh, group_bh,
> +						 main_bm_inode, main_bm_bh);
> +	/* group_bh ownership passed to _ocfs2_reclaim_suballoc_to_main() */
> +	group_bh = NULL;
> +	if (status < 0)
> +		mlog_errno(status);
> +
> +	ocfs2_commit_trans(osb, handle);
> +
> +out_main_unlock:
>  	ocfs2_inode_unlock(main_bm_inode, 1);
>  	brelse(main_bm_bh);
> -
> -free_bm_inode:
> +out_main:
>  	inode_unlock(main_bm_inode);
>  	iput(main_bm_inode);
> +out_group:
> +	brelse(group_bh);
> +out_alloc_unlock:
> +	ocfs2_inode_unlock(alloc_inode, 1);
> +	brelse(alloc_bh);
> +out_alloc:
> +	inode_unlock(alloc_inode);
> +}
>  
> -bail:
> -	return status;
> +void ocfs2_suballoc_reclaim_worker(struct work_struct *work)
> +{
> +	struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
> +					       os_suballoc_reclaim_work);
> +	struct ocfs2_suballoc_reclaim_work *reclaim_work;
> +
> +	while (1) {
> +		spin_lock(&osb->os_suballoc_reclaim_lock);
> +		if (list_empty(&osb->os_suballoc_reclaim_list)) {
> +			spin_unlock(&osb->os_suballoc_reclaim_lock);
> +			break;
> +		}
> +		reclaim_work = list_first_entry(&osb->os_suballoc_reclaim_list,
> +					struct ocfs2_suballoc_reclaim_work,
> +					list);
> +		list_del(&reclaim_work->list);
> +		spin_unlock(&osb->os_suballoc_reclaim_lock);
> +
> +		ocfs2_do_suballoc_reclaim(osb, reclaim_work);
> +		iput(reclaim_work->alloc_inode);
> +		kfree(reclaim_work);
> +	}
>  }
>  
>  /*
> @@ -2955,7 +3098,8 @@ static int _ocfs2_free_suballoc_bits(handle_t *handle,
>  		goto bail;
>  	}
>  
> -	_ocfs2_reclaim_suballoc_to_main(handle, alloc_inode, alloc_bh, group_bh);
> +	ocfs2_queue_suballoc_reclaim(OCFS2_SB(alloc_inode->i_sb), alloc_inode,
> +				     bg_blkno);
>  
>  bail:
>  	brelse(group_bh);
> diff --git a/fs/ocfs2/suballoc.h b/fs/ocfs2/suballoc.h
> index bcf2ed4a8631..6042abc032f9 100644
> --- a/fs/ocfs2/suballoc.h
> +++ b/fs/ocfs2/suballoc.h
> @@ -206,7 +206,7 @@ int ocfs2_lock_allocators(struct inode *inode, struct ocfs2_extent_tree *et,
>  
>  int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res);
>  
> -
> +void ocfs2_suballoc_reclaim_worker(struct work_struct *work);
>  
>  /*
>   * The following two interfaces are for ocfs2_create_inode_in_orphan().
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index f785c39d1fb8..b7c29d3c0321 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -1787,6 +1787,9 @@ static int ocfs2_mount_volume(struct super_block *sb)
>  out_system_inodes:
>  	if (osb->local_alloc_state == OCFS2_LA_ENABLED)
>  		ocfs2_shutdown_local_alloc(osb);
> +	/* Drain pending suballoc reclaim work before the journal goes away */
> +	if (osb->ocfs2_wq)
> +		flush_workqueue(osb->ocfs2_wq);
>  	ocfs2_release_system_inodes(osb);
>  	/* before journal shutdown, we should release slot_info */
>  	ocfs2_free_slot_info(osb);
> @@ -1857,6 +1860,14 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
>  	if (osb->cconn)
>  		ocfs2_super_unlock(osb, 1);
>  
> +	/*
> +	 * Drain pending suballoc reclaim work while the system inodes and
> +	 * the journal are still alive, since the worker needs to look up
> +	 * the global bitmap inode and start a transaction.
> +	 */
> +	if (osb->ocfs2_wq)
> +		flush_workqueue(osb->ocfs2_wq);
> +
>  	ocfs2_release_system_inodes(osb);
>  
>  	ocfs2_journal_shutdown(osb);
> @@ -2143,6 +2154,11 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  	INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
>  	init_llist_head(&osb->dquot_drop_list);
>  
> +	spin_lock_init(&osb->os_suballoc_reclaim_lock);
> +	INIT_LIST_HEAD(&osb->os_suballoc_reclaim_list);
> +	INIT_WORK(&osb->os_suballoc_reclaim_work,
> +		  ocfs2_suballoc_reclaim_worker);
> +
>  	/* get some pseudo constants for clustersize bits */
>  	osb->s_clustersize_bits =
>  		le32_to_cpu(di->id2.i_super.s_clustersize_bits);
> -- 
> 2.39.3
> 

Re: [PATCH v2 1/2] ocfs2: exit recovery thread on mount error path

[Joseph Qi]

On 8/28/26 3:15 PM, Joseph Qi wrote:
> When a mount fails after the cluster connection has been established,
> e.g. in ocfs2_mount_volume(), ocfs2_fill_super() unwinds via
> out_debugfs/out_super and frees the osb without disabling recovery.
> 
> A node failure event can concurrently launch the recovery thread, which
> blocks in __ocfs2_wait_on_mount() waiting for the volume state to
> become VOLUME_MOUNTED or VOLUME_DISABLED.  As the mount error path
> neither sets VOLUME_DISABLED nor wakes osb_mount_event, the thread can
> never make progress: the kthread leaks and stays blocked on the wait
> queue embedded in the freed osb, which may then be accessed as freed
> memory.
> 
> Fix it by setting VOLUME_DISABLED and waking osb_mount_event on this
> path so the thread bails out, and replace the plain
> kfree(osb->recovery_map) with ocfs2_recovery_exit(), which waits for a
> running recovery thread to exit before the recovery map is freed.
> 
> Fixes: f1e75d128b46 ("ocfs2: rewrite error handling of ocfs2_fill_super")
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
>  fs/ocfs2/super.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index c62e389d4dd6..f785c39d1fb8 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -1169,8 +1169,17 @@ static int ocfs2_fill_super(struct super_block *sb, struct fs_context *fc)
>  out_debugfs:
>  	debugfs_remove_recursive(osb->osb_debug_root);
>  out_super:
> +	/*
> +	 * A recovery thread launched by a node failure event may still be
> +	 * waiting for the volume to be mounted.  Set VOLUME_DISABLED and
> +	 * wake it up, then wait for it to exit before osb is freed,
> +	 * otherwise the kthread would leak and stay blocked on the wait
> +	 * queue embedded in the freed osb.
> +	 */
> +	atomic_set(&osb->vol_state, VOLUME_DISABLED);
> +	wake_up(&osb->osb_mount_event);
>  	ocfs2_release_system_inodes(osb);
> -	kfree(osb->recovery_map);
> +	ocfs2_recovery_exit(osb);
>  	ocfs2_delete_osb(osb);
>  	kfree(osb);
>  out:

Sashiko has found 2 more issues:
https://sashiko.dev/#/patchset/20260828071553.262254-1-joseph.qi@linux.alibaba.com?part=1

But both are pre-existing issues, so I'd rather fix them in a separate
thread. More specifically,
1. The double free race of osb->replay_map looks real, will send a fix
later.
2. The early return on ocfs2_super_lock() failure leaks resources,
forcing teardown of cluster state would be worse than the leak, so I'd
keep as it is.

Thanks,
Joseph