Re: [PATCH] xfs: fix unlock in xfs_bmap_add_attrfork

[Dave Chinner <david@fromorbit.com>]

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