Re: [PATCH v2] ocfs2: fix deadlock in inline-data truncate transactions

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 8/6/26 11:59 AM, ZhengYuan Huang wrote:
> [BUG]
> Updating an inode xattr can cause an ABBA deadlock with 
> inline file truncation.
> 
>   ocfs2_truncate_file()
>     down_write(&oi->ip_alloc_sem)
>     ocfs2_truncate_inline()
>       ocfs2_start_trans()
> 
>   ocfs2_xattr_set()
>     ocfs2_start_trans()
>     ocfs2_xattr_ibody_set()
>       down_write(&oi->ip_alloc_sem)
> 
> [CAUSE]
> The xattr set path starts its merged transaction before the helpers which
> modify inode-body xattrs acquire ip_alloc_sem. This creates the reverse of
> the ip_alloc_sem -> transaction order used by the allocation and truncate
> paths. The transaction merge in 
> commit 85db90e77806 ("ocfs2/xattr: Merge xattr set transaction.")
> introduced this ordering for ordinary xattr updates.
> 
> The create-time transaction -> ip_xattr_sem dependency is a separate case.
> The fix in 
> commit c13024342c82 ("ocfs2: fix circular locking dependency in ocfs2_init_acl()")
> prepares the parent ACL before the create transaction and avoids the xattr
> semaphore for initial xattrs on an unpublished inode. That change does not
> cover ordinary ocfs2_xattr_set().
> 
> [FIX]
> Acquire ip_alloc_sem in ocfs2_xattr_set() before xattr preparation,
> allocation reservations, and transaction start.  Tell the inode-body and
> indexed-block helpers when the semaphore is already held, while preserving
> their local locking for callers that do not provide outer protection.
> 
> Keep the existing ip_alloc_sem -> transaction order in allocation and
> truncate paths; fixing the xattr side avoids changing that established
> ordering.
> 

I'd rather fix it in an alternative way by pushing the semaphore fully
up into ocfs2_xattr_set() and strip it from the helpers entirely, which
seems more cleaner.

I have a local prototype and will send out when pass regression tests.

Thanks,
Joseph

> Fixes: 85db90e77806 ("ocfs2/xattr: Merge xattr set transaction.")
> Signed-off-by: ZhengYuan Huang <gality369@gmail.com>
> ---
> v2:
> - Fix the xattr side of the lock ordering based on Joseph's feedback.
> - Explain why the remaining fix belongs on the ordinary xattr side.
> ---
>  fs/ocfs2/xattr.c | 49 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 35bcbb0ff607..27bea1f360a1 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -74,6 +74,7 @@ struct ocfs2_xattr_set_ctxt {
>  	struct ocfs2_alloc_context *data_ac;
>  	struct ocfs2_cached_dealloc_ctxt dealloc;
>  	int set_abort;
> +	int alloc_sem_protected;
>  };
>  
>  #define OCFS2_XATTR_ROOT_SIZE	(sizeof(struct ocfs2_xattr_def_value_root))
> @@ -2916,7 +2917,8 @@ static int ocfs2_xattr_has_space_inline(struct inode *inode,
>  static int ocfs2_xattr_ibody_find(struct inode *inode,
>  				  int name_index,
>  				  const char *name,
> -				  struct ocfs2_xattr_search *xs)
> +				  struct ocfs2_xattr_search *xs,
> +				  int lock_alloc_sem)
>  {
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
>  	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
> @@ -2927,9 +2929,11 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
>  		return 0;
>  
>  	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
> -		down_read(&oi->ip_alloc_sem);
> +		if (lock_alloc_sem)
> +			down_read(&oi->ip_alloc_sem);
>  		has_space = ocfs2_xattr_has_space_inline(inode, di);
> -		up_read(&oi->ip_alloc_sem);
> +		if (lock_alloc_sem)
> +			up_read(&oi->ip_alloc_sem);
>  		if (!has_space)
>  			return 0;
>  	}
> @@ -3016,14 +3020,17 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
>  				 struct ocfs2_xattr_search *xs,
>  				 struct ocfs2_xattr_set_ctxt *ctxt)
>  {
> -	int ret;
> +	int ret, took_alloc_sem = 0;
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
>  	struct ocfs2_xa_loc loc;
>  
>  	if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
>  		return -ENOSPC;
>  
> -	down_write(&oi->ip_alloc_sem);
> +	if (!ctxt->alloc_sem_protected) {
> +		down_write(&oi->ip_alloc_sem);
> +		took_alloc_sem = 1;
> +	}
>  	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
>  		ret = ocfs2_xattr_ibody_init(inode, xs->inode_bh, ctxt);
>  		if (ret) {
> @@ -3044,7 +3051,8 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
>  	xs->here = loc.xl_entry;
>  
>  out:
> -	up_write(&oi->ip_alloc_sem);
> +	if (took_alloc_sem)
> +		up_write(&oi->ip_alloc_sem);
>  
>  	return ret;
>  }
> @@ -3729,6 +3737,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
>  		.handle = handle,
>  		.meta_ac = meta_ac,
>  		.data_ac = data_ac,
> +		.alloc_sem_protected = 1,
>  	};
>  
>  	if (!ocfs2_supports_xattr(OCFS2_SB(inode->i_sb)))
> @@ -3750,7 +3759,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
>  	xis.inode_bh = xbs.inode_bh = di_bh;
>  	di = (struct ocfs2_dinode *)di_bh->b_data;
>  
> -	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis);
> +	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis, 0);
>  	if (ret)
>  		goto cleanup;
>  	if (xis.not_found) {
> @@ -3834,7 +3843,7 @@ int ocfs2_xattr_set(struct inode *inode,
>  	 * Scan inode and external block to find the same name
>  	 * extended attribute and collect search information.
>  	 */
> -	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis);
> +	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis, 1);
>  	if (ret)
>  		goto cleanup;
>  	if (xis.not_found) {
> @@ -3856,6 +3865,8 @@ int ocfs2_xattr_set(struct inode *inode,
>  			goto cleanup;
>  	}
>  
> +	down_write(&OCFS2_I(inode)->ip_alloc_sem);
> +
>  	/* Check whether the value is refcounted and do some preparation. */
>  	if (ocfs2_is_refcount_inode(inode) &&
>  	    (!xis.not_found || !xbs.not_found)) {
> @@ -3864,7 +3875,7 @@ int ocfs2_xattr_set(struct inode *inode,
>  						   &ref_meta, &ref_credits);
>  		if (ret) {
>  			mlog_errno(ret);
> -			goto cleanup;
> +			goto out_unlock_alloc;
>  		}
>  	}
>  
> @@ -3875,7 +3886,7 @@ int ocfs2_xattr_set(struct inode *inode,
>  		if (ret < 0) {
>  			inode_unlock(tl_inode);
>  			mlog_errno(ret);
> -			goto cleanup;
> +			goto out_unlock_alloc;
>  		}
>  	}
>  	inode_unlock(tl_inode);
> @@ -3884,8 +3895,9 @@ int ocfs2_xattr_set(struct inode *inode,
>  					&xbs, &ctxt, ref_meta, &credits);
>  	if (ret) {
>  		mlog_errno(ret);
> -		goto cleanup;
> +		goto out_unlock_alloc;
>  	}
> +	ctxt.alloc_sem_protected = 1;
>  
>  	/* we need to update inode's ctime field, so add credit for it. */
>  	credits += OCFS2_INODE_UPDATE_CREDITS;
> @@ -3893,15 +3905,15 @@ int ocfs2_xattr_set(struct inode *inode,
>  	if (IS_ERR(ctxt.handle)) {
>  		ret = PTR_ERR(ctxt.handle);
>  		mlog_errno(ret);
> -		goto out_free_ac;
> +		goto out_unlock_alloc;
>  	}
>  
>  	ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
>  	ocfs2_update_inode_fsync_trans(ctxt.handle, inode, 0);
>  
>  	ocfs2_commit_trans(osb, ctxt.handle);
> -
> -out_free_ac:
> +out_unlock_alloc:
> +	up_write(&OCFS2_I(inode)->ip_alloc_sem);
>  	if (ctxt.data_ac)
>  		ocfs2_free_alloc_context(ctxt.data_ac);
>  	if (ctxt.meta_ac)
> @@ -4520,6 +4532,7 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
>  	u64 blkno;
>  	handle_t *handle = ctxt->handle;
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> +	int took_alloc_sem = 0;
>  	struct buffer_head *xb_bh = xs->xattr_bh;
>  	struct ocfs2_xattr_block *xb =
>  			(struct ocfs2_xattr_block *)xb_bh->b_data;
> @@ -4537,7 +4550,10 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
>  	 * We can use this lock for now, and maybe move to a dedicated mutex
>  	 * if performance becomes a problem later.
>  	 */
> -	down_write(&oi->ip_alloc_sem);
> +	if (!ctxt->alloc_sem_protected) {
> +		down_write(&oi->ip_alloc_sem);
> +		took_alloc_sem = 1;
> +	}
>  
>  	ret = ocfs2_journal_access_xb(handle, INODE_CACHE(inode), xb_bh,
>  				      OCFS2_JOURNAL_ACCESS_WRITE);
> @@ -4600,7 +4616,8 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
>  	ocfs2_journal_dirty(handle, xb_bh);
>  
>  out:
> -	up_write(&oi->ip_alloc_sem);
> +	if (took_alloc_sem)
> +		up_write(&oi->ip_alloc_sem);
>  
>  	return ret;
>  }