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

[Heming Zhao <heming.zhao@suse.com>]

On Thu, Aug 27, 2026 at 08:43:13PM +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 orphan
> dir recovery frees inode bits, so both ocfs2_dismount_volume() and
> the mount error path flush ocfs2_wq right before the journal is
> shut down 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    |  12 +++
>  4 files changed, 191 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 c62e389d4dd6..c1aafbb9500a 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -1779,6 +1779,9 @@ static int ocfs2_mount_volume(struct super_block *sb)
>  	if (osb->local_alloc_state == OCFS2_LA_ENABLED)
>  		ocfs2_shutdown_local_alloc(osb);
>  	ocfs2_release_system_inodes(osb);
> +	/* Drain pending suballoc reclaim work before the journal goes away */
> +	if (osb->ocfs2_wq)
> +		flush_workqueue(osb->ocfs2_wq);

The flush_workqueue() calls ocfs2_get_system_file_inode() to grab main_bm_inode.
However, after ocfs2_release_system_inodes(), all the cached system inodes are gone,
And the _ocfs2_get_system_file_inode() uses osb->sys_root_inode, which is already
freed by ocfs2_release_system_inodes(). So we should move the
flush_workqueue() calls to before ocfs2_release_system_inodes().

>  	/* before journal shutdown, we should release slot_info */
>  	ocfs2_free_slot_info(osb);
>  	ocfs2_journal_shutdown(osb);
> @@ -1850,6 +1853,10 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
>  
>  	ocfs2_release_system_inodes(osb);
>  
> +	/* Drain pending suballoc reclaim work before the journal goes away */
> +	if (osb->ocfs2_wq)
> +		flush_workqueue(osb->ocfs2_wq);
> +

ditto

Thanks,
Heming
>  	ocfs2_journal_shutdown(osb);
>  
>  	/*
> @@ -2134,6 +2141,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
>