* [PATCH v2] xfs: fix unlock in xfs_bmap_add_attrfork
@ 2013-11-06 21:14 Mark Tinguely
2013-11-06 22:41 ` Dave Chinner
0 siblings, 1 reply; 3+ messages in thread
From: Mark Tinguely @ 2013-11-06 21:14 UTC (permalink / raw)
To: xfs
[-- Attachment #1: v2-xfs-fix-unlock-in-xfs_bmap_add_attrfork.patch --]
[-- Type: text/plain, Size: 3263 bytes --]
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>
---
v2 per Dave's suggestion:
remove the lock from the transaction
adjust the cancel_flags.
fs/xfs/xfs_bmap.c | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)
Index: b/fs/xfs/xfs_bmap.c
===================================================================
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1137,6 +1137,7 @@ xfs_bmap_add_attrfork(
int committed; /* xaction was committed */
int logflags; /* logging flags */
int error; /* error return value */
+ int cancel_flags = 0;
ASSERT(XFS_IFORK_Q(ip) == 0);
@@ -1147,19 +1148,20 @@ xfs_bmap_add_attrfork(
if (rsvd)
tp->t_flags |= XFS_TRANS_RESERVE;
error = xfs_trans_reserve(tp, &M_RES(mp)->tr_addafork, blks, 0);
- if (error)
- goto error0;
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ return error;
+ }
+ cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
xfs_ilock(ip, XFS_ILOCK_EXCL);
error = xfs_trans_reserve_quota_nblks(tp, ip, blks, 0, rsvd ?
XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
XFS_QMOPT_RES_REGBLKS);
- if (error) {
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
- xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
- return error;
- }
+ if (error)
+ goto trans_cancel;
+ cancel_flags |= XFS_TRANS_ABORT;
if (XFS_IFORK_Q(ip))
- goto error1;
+ goto trans_cancel;
if (ip->i_d.di_aformat != XFS_DINODE_FMT_EXTENTS) {
/*
* For inodes coming from pre-6.2 filesystems.
@@ -1169,7 +1171,7 @@ xfs_bmap_add_attrfork(
}
ASSERT(ip->i_d.di_anextents == 0);
- xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, ip, 0);
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
switch (ip->i_d.di_format) {
@@ -1191,7 +1193,7 @@ xfs_bmap_add_attrfork(
default:
ASSERT(0);
error = XFS_ERROR(EINVAL);
- goto error1;
+ goto trans_cancel;
}
ASSERT(ip->i_afp == NULL);
@@ -1219,7 +1221,7 @@ xfs_bmap_add_attrfork(
if (logflags)
xfs_trans_log_inode(tp, ip, logflags);
if (error)
- goto error2;
+ goto bmap_cancel;
if (!xfs_sb_version_hasattr(&mp->m_sb) ||
(!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2)) {
__int64_t sbfields = 0;
@@ -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;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] xfs: fix unlock in xfs_bmap_add_attrfork
2013-11-06 21:14 [PATCH v2] xfs: fix unlock in xfs_bmap_add_attrfork Mark Tinguely
@ 2013-11-06 22:41 ` Dave Chinner
2013-11-07 13:17 ` Mark Tinguely
0 siblings, 1 reply; 3+ messages in thread
From: Dave Chinner @ 2013-11-06 22:41 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] xfs: fix unlock in xfs_bmap_add_attrfork
2013-11-06 22:41 ` Dave Chinner
@ 2013-11-07 13:17 ` Mark Tinguely
0 siblings, 0 replies; 3+ messages in thread
From: Mark Tinguely @ 2013-11-07 13:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
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@sgi.com>
>
> 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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-11-07 13:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-06 21:14 [PATCH v2] xfs: fix unlock in xfs_bmap_add_attrfork Mark Tinguely
2013-11-06 22:41 ` Dave Chinner
2013-11-07 13:17 ` Mark Tinguely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox