xfs
[Top] [All Lists]

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote syml

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 20 Sep 2013 09:05:26 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130918221729.GB9901@dastard>
References: <1379520960-22972-1-git-send-email-bfoster@xxxxxxxxxx> <1379520960-22972-2-git-send-email-bfoster@xxxxxxxxxx> <20130918221729.GB9901@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8
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.
> 

<Prev in Thread] Current Thread [Next in Thread>