On 09/18/2013 06:17 PM, Dave Chinner wrote:
> On Wed, Sep 18, 2013 at 12:15:58PM -0400, Brian Foster wrote:
>> Push down the transaction management for remote symlinks from
>> xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
>> cleaned up to avoid transaction management intended for the
>> calling context (i.e., trans duplication, reservation, item
>> attachment).
>>
>> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
>> ---
...
>> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
>> index f622a97..f85f6f2 100644
>> --- a/fs/xfs/xfs_symlink.c
>> +++ b/fs/xfs/xfs_symlink.c
>> @@ -424,8 +424,7 @@ xfs_symlink(
>> */
>> STATIC int
>> xfs_inactive_symlink_rmt(
...
>> /*
>> * The transaction must have been committed, since there were
>> * actually extents freed by xfs_bunmapi. See xfs_bmap_finish.
>> @@ -508,29 +513,16 @@ xfs_inactive_symlink_rmt(
>> * Mark it dirty so it will be logged and moved forward in the log as
>> * part of every commit.
>> */
>> - xfs_trans_ijoin(tp, ip, 0);
>> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>
> Oh, good, you caught the "need to unlock the inode at commit" case
> :)
>
Yeah, and while fixing the error handling order issues in v2 I just
noticed that this leaves the final xfs_idata_realloc() call in
xfs_inactive_symlink_rmt() unprotected wrt to the ilock. ;) I'll fix
that up to just do the (un)locking manually here as well for v3...
Brian
>>
>> - error1:
>> +error2:
>> xfs_bmap_cancel(&free_list);
>> - error0:
>> +error1:
>> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +error0:
>> + xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
>> return error;
>
> And the error labels need reworking appropriately.
>
>> }
>>
>> @@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
>> */
>> int
>> xfs_inactive_symlink(
>> - struct xfs_inode *ip,
>> - struct xfs_trans **tp)
>> + struct xfs_inode *ip)
>> {
>> struct xfs_mount *mp = ip->i_mount;
>> int pathlen;
>>
>> trace_xfs_inactive_symlink(ip);
>>
>> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>> -
>> if (XFS_FORCED_SHUTDOWN(mp))
>> return XFS_ERROR(EIO);
>
> The call to xfs_idata_realloc() needs to be done under the
> XFS_ILOCK_EXCL here. We can race with other inode cache lookups
> that do work, so we do need to ensure that we correctly lock
> everything for modifications that are to be made to the inode state.
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
