On 11/06/13 16:41, Dave Chinner wrote:
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@xxxxxxx>
All good, except for:
...
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.
Thanks for catching that.
--Mark.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
