Re: [PATCH v2] xfs: fix unlock in xfs_bmap_add_attrfork

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 06, 2013 at 03:14:58PM -0600, Mark Tinguely wrote:
> xfs_trans_ijoin() activates the inode in a transaction and
> also can specify which lock to free when the transaction is
> committed or canceled.
> 
> xfs_bmap_add_attrfork call locks and add the lock to the
> transaction but also manually removes the lock. Change the
> routine to not add the lock to the transaction and manually
> remove lock on completion.
> 
> While here, clean up the xfs_trans_cancel flags and goto names.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>

All good, except for:

> @@ -1242,14 +1244,15 @@ xfs_bmap_add_attrfork(
>  
>  	error = xfs_bmap_finish(&tp, &flist, &committed);
>  	if (error)
> -		goto error2;
> -	return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> -error2:
> +		goto bmap_cancel;
> +	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> +	goto unlock_return;
> +bmap_cancel:
>  	xfs_bmap_cancel(&flist);
> -error1:
> +trans_cancel:
> +	xfs_trans_cancel(tp, cancel_flags);
> +unlock_return:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -error0:
> -	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
>  	return error;
>  }

This hunk. It ends up looking like this:

	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
	goto unlock_return;
bmap_cancel:
	xfs_bmap_cancel(&flist);
trans_cancel:
	xfs_trans_cancel(tp, cancel_flags);
unlock_return:
	xfs_iunlock(ip, XFS_ILOCK_EXCL);
	return error;
}

Which jumps over error handling cases on a successful completion.
The conventional way of doing this is having the successful return
case run straight through to the return, and having the error stack
either jump back into it like so:

	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
unlock_return:
	xfs_iunlock(ip, XFS_ILOCK_EXCL);
	return error;

bmap_cancel:
	xfs_bmap_cancel(&flist);
trans_cancel:
	xfs_trans_cancel(tp, cancel_flags);
	goto unlock_return;
}

or do an additional unlock and return directly in the error stack
and let the compiler optimise it appropriately.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs