* [PATCH] xfs: fix unlock in xfs_bmap_add_attrfork
@ 2013-11-05 20:27 Mark Tinguely
2013-11-06 5:39 ` Dave Chinner
0 siblings, 1 reply; 3+ messages in thread
From: Mark Tinguely @ 2013-11-05 20:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-fix-unlock-in-xfs_bmap_add_attrfork.patch --]
[-- Type: text/plain, Size: 3068 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 adds the XFS_ILOCK_EXCL flag when calling
xfs_trans_ijoin() so it wrong to also free this lock before doing
a xfs_trans_cancel. Add the unlock to the error case before the
xfs_trans_ijoin and remove the unlock from the error recovery.
While here, clean up the goto names.
It seem doubtful that this routine fails, I found this visually
looking for another issue.
Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
fs/xfs/xfs_bmap.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
Index: b/fs/xfs/xfs_bmap.c
===================================================================
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1137,9 +1137,11 @@ xfs_bmap_add_attrfork(
int committed; /* xaction was committed */
int logflags; /* logging flags */
int error; /* error return value */
+ int cancel_flags;
ASSERT(XFS_IFORK_Q(ip) == 0);
+ cancel_flags = XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT;
mp = ip->i_mount;
ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
@@ -1148,18 +1150,20 @@ xfs_bmap_add_attrfork(
tp->t_flags |= XFS_TRANS_RESERVE;
error = xfs_trans_reserve(tp, &M_RES(mp)->tr_addafork, blks, 0);
if (error)
- goto error0;
+ goto trans_cancel;
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) {
+ cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
xfs_iunlock(ip, XFS_ILOCK_EXCL);
- xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
- return error;
+ goto trans_cancel;
+ }
+ if (XFS_IFORK_Q(ip)) {
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ goto trans_cancel;
}
- if (XFS_IFORK_Q(ip))
- goto error1;
if (ip->i_d.di_aformat != XFS_DINODE_FMT_EXTENTS) {
/*
* For inodes coming from pre-6.2 filesystems.
@@ -1191,7 +1195,7 @@ xfs_bmap_add_attrfork(
default:
ASSERT(0);
error = XFS_ERROR(EINVAL);
- goto error1;
+ goto trans_cancel;
}
ASSERT(ip->i_afp == NULL);
@@ -1219,7 +1223,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 +1246,12 @@ xfs_bmap_add_attrfork(
error = xfs_bmap_finish(&tp, &flist, &committed);
if (error)
- goto error2;
+ goto bmap_cancel;
return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
-error2:
+bmap_cancel:
xfs_bmap_cancel(&flist);
-error1:
- xfs_iunlock(ip, XFS_ILOCK_EXCL);
-error0:
- xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
+trans_cancel:
+ xfs_trans_cancel(tp, cancel_flags);
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] xfs: fix unlock in xfs_bmap_add_attrfork
2013-11-05 20:27 [PATCH] xfs: fix unlock in xfs_bmap_add_attrfork Mark Tinguely
@ 2013-11-06 5:39 ` Dave Chinner
2013-11-06 14:59 ` Mark Tinguely
0 siblings, 1 reply; 3+ messages in thread
From: Dave Chinner @ 2013-11-06 5:39 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On Tue, Nov 05, 2013 at 02:27:07PM -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 adds the XFS_ILOCK_EXCL flag when calling
> xfs_trans_ijoin() so it wrong to also free this lock before doing
> a xfs_trans_cancel. Add the unlock to the error case before the
> xfs_trans_ijoin and remove the unlock from the error recovery.
> While here, clean up the goto names.
Definitely a bug, but I suspect it is the wrong fix. This is a
permanent log transaction, which means there are multiple commits
before the final commit, and xfs_trans_ijoin(XFS_ILOCK_EXCL) means
we will unlock the inode at the first commit that is made after the
join, not after the modifications are completed. Hence someone else
can come in and lock and modify the inode before we've finish all
the work in this function.
Hence I suspect the right fix is to make this code use
xfs_trans_ijoin(0) and do manual unlocking of the inode after the
last commit/cancel is done, similar to how it is done in
xfs_iomap_write_allocate/direct...
Coupl eof other minor things:
> ===================================================================
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -1137,9 +1137,11 @@ xfs_bmap_add_attrfork(
> int committed; /* xaction was committed */
> int logflags; /* logging flags */
> int error; /* error return value */
> + int cancel_flags;
>
> ASSERT(XFS_IFORK_Q(ip) == 0);
>
> + cancel_flags = XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT;
> mp = ip->i_mount;
> ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
> tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
> @@ -1148,18 +1150,20 @@ xfs_bmap_add_attrfork(
> tp->t_flags |= XFS_TRANS_RESERVE;
> error = xfs_trans_reserve(tp, &M_RES(mp)->tr_addafork, blks, 0);
> if (error)
> - goto error0;
> + goto trans_cancel;
No reservation exists, so xfs_trans_cancel(tp, 0) is appropriate
here. i.e. cancel_flags gets initialised to 0, and after we have a
reservation we do:
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) {
> + cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> - xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
> - return error;
> + goto trans_cancel;
> + }
Now we potentially have dirty objects in the transaction, so
cancel_flags |= XFS_TRANS_ABORT;
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] xfs: fix unlock in xfs_bmap_add_attrfork
2013-11-06 5:39 ` Dave Chinner
@ 2013-11-06 14:59 ` Mark Tinguely
0 siblings, 0 replies; 3+ messages in thread
From: Mark Tinguely @ 2013-11-06 14:59 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 11/05/13 23:39, Dave Chinner wrote:
> On Tue, Nov 05, 2013 at 02:27:07PM -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 adds the XFS_ILOCK_EXCL flag when calling
>> xfs_trans_ijoin() so it wrong to also free this lock before doing
>> a xfs_trans_cancel. Add the unlock to the error case before the
>> xfs_trans_ijoin and remove the unlock from the error recovery.
>> While here, clean up the goto names.
>
> Definitely a bug, but I suspect it is the wrong fix. This is a
> permanent log transaction, which means there are multiple commits
> before the final commit, and xfs_trans_ijoin(XFS_ILOCK_EXCL) means
> we will unlock the inode at the first commit that is made after the
> join, not after the modifications are completed. Hence someone else
> can come in and lock and modify the inode before we've finish all
> the work in this function.
>
> Hence I suspect the right fix is to make this code use
> xfs_trans_ijoin(0) and do manual unlocking of the inode after the
> last commit/cancel is done, similar to how it is done in
> xfs_iomap_write_allocate/direct...
>
> Coupl eof other minor things:
>
>> ===================================================================
>> --- a/fs/xfs/xfs_bmap.c
>> +++ b/fs/xfs/xfs_bmap.c
>> @@ -1137,9 +1137,11 @@ xfs_bmap_add_attrfork(
>> int committed; /* xaction was committed */
>> int logflags; /* logging flags */
>> int error; /* error return value */
>> + int cancel_flags;
>>
>> ASSERT(XFS_IFORK_Q(ip) == 0);
>>
>> + cancel_flags = XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT;
>> mp = ip->i_mount;
>> ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
>> tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
>> @@ -1148,18 +1150,20 @@ xfs_bmap_add_attrfork(
>> tp->t_flags |= XFS_TRANS_RESERVE;
>> error = xfs_trans_reserve(tp,&M_RES(mp)->tr_addafork, blks, 0);
>> if (error)
>> - goto error0;
>> + goto trans_cancel;
>
> No reservation exists, so xfs_trans_cancel(tp, 0) is appropriate
> here. i.e. cancel_flags gets initialised to 0, and after we have a
> reservation we do:
nod.
> 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) {
>> + cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>> xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> - xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
>> - return error;
>> + goto trans_cancel;
>> + }
>
> Now we potentially have dirty objects in the transaction, so
>
> cancel_flags |= XFS_TRANS_ABORT;
Okay. I will clean it up and repost.
--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-06 14:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-05 20:27 [PATCH] xfs: fix unlock in xfs_bmap_add_attrfork Mark Tinguely
2013-11-06 5:39 ` Dave Chinner
2013-11-06 14:59 ` Mark Tinguely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox