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>
>> ---
>> fs/xfs/xfs_inode.c | 15 ++++++------
>> fs/xfs/xfs_symlink.c | 64 ++++++++++++++++++----------------------------------
>> fs/xfs/xfs_symlink.h | 2 +-
>> 3 files changed, 31 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index e3d7538..30db70e 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -1724,9 +1724,14 @@ xfs_inactive(
>> if (error)
>> return VN_INACTIVE_CACHE;
>>
>> + if (S_ISLNK(ip->i_d.di_mode)) {
>> + error = xfs_inactive_symlink(ip);
>> + if (error)
>> + goto out;
>> + }
>> +
>> tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
>> - resp = (truncate || S_ISLNK(ip->i_d.di_mode)) ?
>> - &M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
>> + resp = truncate ? &M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
>>
>> error = xfs_trans_reserve(tp, resp, 0, 0);
>> if (error) {
>> @@ -1738,11 +1743,7 @@ xfs_inactive(
>> xfs_ilock(ip, XFS_ILOCK_EXCL);
>> xfs_trans_ijoin(tp, ip, 0);
>>
>> - if (S_ISLNK(ip->i_d.di_mode)) {
>> - error = xfs_inactive_symlink(ip, &tp);
>> - if (error)
>> - goto out_cancel;
>> - } else if (truncate) {
>> + if (truncate) {
>> ip->i_d.di_size = 0;
>> xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>
> Just to maintain the same logic here, if it is a symlink shouldn't
> we be ensuring that truncate is zero so we don't go down that path
> (even if it's just an assert that you add)?
>
I missed this on my first pass of your review, but this actually turns
back into an if/else after patch 2. E.g., once we pull the truncate case
back before the transaction allocation still being used for the ifree
(until patch 3). So I think this is probably unnecessary.
Brian
>> 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(
>> - xfs_inode_t *ip,
>> - xfs_trans_t **tpp)
>> + xfs_inode_t *ip)
>> {
>> xfs_buf_t *bp;
>> int committed;
>> @@ -437,11 +436,9 @@ xfs_inactive_symlink_rmt(
>> xfs_mount_t *mp;
>> xfs_bmbt_irec_t mval[XFS_SYMLINK_MAPS];
>> int nmaps;
>> - xfs_trans_t *ntp;
>> int size;
>> xfs_trans_t *tp;
>>
>> - tp = *tpp;
>> mp = ip->i_mount;
>> ASSERT(ip->i_df.if_flags & XFS_IFEXTENTS);
>> /*
>> @@ -453,6 +450,14 @@ xfs_inactive_symlink_rmt(
>> */
>> ASSERT(ip->i_d.di_nextents > 0 && ip->i_d.di_nextents <= 2);
>>
>> + tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
>> + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
>> + if (error)
>> + goto error0;
>
> goto error_trans_cancel;
>> +
>> + xfs_ilock(ip, XFS_ILOCK_EXCL);
>> + xfs_trans_ijoin(tp, ip, 0);
>> +
>> /*
>> * Lock the inode, fix the size, and join it to the transaction.
>> * Hold it so in the normal path, we still have it locked for
>> @@ -471,7 +476,7 @@ xfs_inactive_symlink_rmt(
>> error = xfs_bmapi_read(ip, 0, xfs_symlink_blocks(mp, size),
>> mval, &nmaps, 0);
>> if (error)
>> - goto error0;
>> + goto error1;
>
> goto error_unlock;
>
>> /*
>> * Invalidate the block(s). No validation is done.
>> */
>> @@ -481,7 +486,7 @@ xfs_inactive_symlink_rmt(
>> XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
>> if (!bp) {
>> error = ENOMEM;
>> - goto error1;
>> + goto error2;
>
> goto error_bmap_cancel;
>
>> }
>> xfs_trans_binval(tp, bp);
>> }
>> @@ -490,13 +495,13 @@ xfs_inactive_symlink_rmt(
>> */
>> if ((error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
>> &first_block, &free_list, &done)))
>> - goto error1;
>> + goto error2;
>
> Can you convert this to:
>
> error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
> &first_block, &free_list, &done);
> if (error)
> goto error_bmap_cancel;
>
>> ASSERT(done);
>> /*
>> * Commit the first transaction. This logs the EFI and the inode.
>> */
>> if ((error = xfs_bmap_finish(&tp, &free_list, &committed)))
>> - goto error1;
>> + goto error2;
>
> Same here.
>
>> /*
>> * 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
> :)
>
>>
>> - 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
