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

From: Brian Foster <bfoster@redhat.com>
To: xfs@oss.sgi.com
Subject: Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks
Date: Wed, 18 Sep 2013 14:06:26 -0400	[thread overview]
Message-ID: <5239EBA2.4070207@redhat.com> (raw)
In-Reply-To: <1379520960-22972-2-git-send-email-bfoster@redhat.com>

On 09/18/2013 12:15 PM, 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@redhat.com>
> ---
>  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_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(
>   */
...
>  
> @@ -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));
> -

I just want to call out one thing here in case it isn't noticed on
review... the safety of this is something I was curious about.
Specifically, note that I've removed the inode locking from
xfs_inactive(), which previously covered xfs_inactive_symlink() (for
xfs_idata_realloc()), down into xfs_inactive_symlink_rmt().

My assumption was that this is currently ok since at this point we have
an inode with di_nlink == 0. If that's not accurate or not expected to
remain so after O_TMPFILE related work, I suppose I could pull the
locking back up into xfs_inactive_symlink().

Brian

>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return XFS_ERROR(EIO);
>  
> @@ -599,5 +579,5 @@ xfs_inactive_symlink(
>  	}
>  
>  	/* remove the remote symlink */
> -	return xfs_inactive_symlink_rmt(ip, tp);
> +	return xfs_inactive_symlink_rmt(ip);
>  }
> diff --git a/fs/xfs/xfs_symlink.h b/fs/xfs/xfs_symlink.h
> index 99338ba..e75245d 100644
> --- a/fs/xfs/xfs_symlink.h
> +++ b/fs/xfs/xfs_symlink.h
> @@ -22,6 +22,6 @@
>  int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
>  		const char *target_path, umode_t mode, struct xfs_inode **ipp);
>  int xfs_readlink(struct xfs_inode *ip, char *link);
> -int xfs_inactive_symlink(struct xfs_inode *ip, struct xfs_trans **tpp);
> +int xfs_inactive_symlink(struct xfs_inode *ip);
>  
>  #endif /* __XFS_SYMLINK_H */
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs