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

Brian Foster
2013-09-18T18:19:56Z

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 */