xfs
[Top] [All Lists]

Re: [PATCH v3 1/4] xfs: push down inactive transaction mgmt for remote s

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH v3 1/4] xfs: push down inactive transaction mgmt for remote symlinks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 30 Sep 2013 10:29:25 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1379689572-5543-2-git-send-email-bfoster@xxxxxxxxxx>
References: <1379689572-5543-1-git-send-email-bfoster@xxxxxxxxxx> <1379689572-5543-2-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Sep 20, 2013 at 11:06:09AM -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>

Looks good. One minor quibble if you need to respin the patches
again, but otherwise:

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

>  
> @@ -563,41 +552,46 @@ 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);
>  
> +     xfs_ilock(ip, XFS_ILOCK_EXCL);
> +
>       /*
>        * Zero length symlinks _can_ exist.
>        */
>       pathlen = (int)ip->i_d.di_size;
> -     if (!pathlen)
> +     if (!pathlen) {
> +             xfs_iunlock(ip, XFS_ILOCK_EXCL);
>               return 0;
> +     }

Minor quibble: this repeated "unlock, return error" pattern could
be done with:

....
        if (!pathlen)
                goto out_unlock;
....
        /* remove the remote symlink */
        return xfs_inactive_symlink_rmt(ip);

out_unlock:
        xfs_iunlock(ip, XFS_ILOCK_EXCL);
        return error;
}

But, like I said, there's no need to do this unless I find other
things in the rest of the series that require a respin...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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