Re: [PATCH 14/21] Add lock_flags to xfs_ialloc and xfs_dir_ialloc

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sun, May 06, 2018 at 10:24:47AM -0700, Allison Henderson wrote:
> Add lock_flags to  xfs_ialloc and xfs_dir_ialloc to control
> whick locks are released by xfs_trans_ijoin.  We will need this
> later in defered parent pointers
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_inode.c   | 17 +++++++++--------
>  fs/xfs/xfs_inode.h   |  2 +-
>  fs/xfs/xfs_qm.c      |  2 +-
>  fs/xfs/xfs_symlink.c |  2 +-
>  4 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 5c291d2..2859a697 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -766,7 +766,8 @@ xfs_ialloc(
>  	dev_t		rdev,
>  	prid_t		prid,
>  	xfs_buf_t	**ialloc_context,
> -	xfs_inode_t	**ipp)
> +	xfs_inode_t	**ipp,
> +	int		lock_flags)

Wait, what?

Oh, these are the locks we want *dropped* at the first _trans_commit
after this call returns, and for xfs_create we need to retain the ilock
while we roll the transaction(s) during _defer_finish; and for
everything else (create temp file, create quota inode, and symlink??) we
want the ilock dropped as soon as the transaction commits.

I dislike having this oddly named parameter, can we amend the comment to
say that the caller is responsible for unlocking the inode manually
(i.e. we're going to xfs_trans_ijoin(tp, ip, 0)) , and then change all
the callers to do the iunlock explicitly if they need to?

--D

>  {
>  	struct xfs_mount *mp = tp->t_mountp;
>  	xfs_ino_t	ino;
> @@ -942,7 +943,7 @@ xfs_ialloc(
>  	/*
>  	 * Log the new values stuffed into the inode.
>  	 */
> -	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, lock_flags);
>  	xfs_trans_log_inode(tp, ip, flags);
>  
>  	/* now that we have an i_mode we can setup the inode structure */
> @@ -972,8 +973,8 @@ xfs_dir_ialloc(
>  	xfs_nlink_t	nlink,
>  	dev_t		rdev,
>  	prid_t		prid,		/* project id */
> -	xfs_inode_t	**ipp)		/* pointer to inode; it will be
> -					   locked. */
> +	xfs_inode_t	**ipp,		/* pointer to inode; it will be locked. */
> +	int		lock_flags)
>  {
>  	xfs_trans_t	*tp;
>  	xfs_inode_t	*ip;
> @@ -1001,7 +1002,7 @@ xfs_dir_ialloc(
>  	 * the inode(s) that we've just allocated.
>  	 */
>  	code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
> -			&ip);
> +			&ip, lock_flags);
>  
>  	/*
>  	 * Return an error if we were unable to allocate a new inode.
> @@ -1071,7 +1072,7 @@ xfs_dir_ialloc(
>  		 * this call should always succeed.
>  		 */
>  		code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
> -				  &ialloc_context, &ip);
> +				  &ialloc_context, &ip, lock_flags);
>  
>  		/*
>  		 * If we get an error at this point, return to the caller
> @@ -1210,7 +1211,7 @@ xfs_create(
>  	 * entry pointing to them, but a directory also the "." entry
>  	 * pointing to itself.
>  	 */
> -	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid, &ip);
> +	error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid, &ip, XFS_ILOCK_EXCL);
>  	if (error)
>  		goto out_trans_cancel;
>  
> @@ -1343,7 +1344,7 @@ xfs_create_tmpfile(
>  	if (error)
>  		goto out_trans_cancel;
>  
> -	error = xfs_dir_ialloc(&tp, dp, mode, 1, 0, prid, &ip);
> +	error = xfs_dir_ialloc(&tp, dp, mode, 1, 0, prid, &ip, XFS_ILOCK_EXCL);
>  	if (error)
>  		goto out_trans_cancel;
>  
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 1eebc53..466f252 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -431,7 +431,7 @@ xfs_extlen_t	xfs_get_cowextsz_hint(struct xfs_inode *ip);
>  
>  int		xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t,
>  			       xfs_nlink_t, dev_t, prid_t,
> -			       struct xfs_inode **);
> +			       struct xfs_inode **, int lock_flags);
>  
>  /* from xfs_file.c */
>  enum xfs_prealloc_flags {
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index ec39ae2..3e68a52 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -787,7 +787,7 @@ xfs_qm_qino_alloc(
>  		return error;
>  
>  	if (need_alloc) {
> -		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, ip);
> +		error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, ip, XFS_ILOCK_EXCL);
>  		if (error) {
>  			xfs_trans_cancel(tp);
>  			return error;
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index b1d3301..ce8dbea 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -264,7 +264,7 @@ xfs_symlink(
>  	 * Allocate an inode for the symlink.
>  	 */
>  	error = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0,
> -			       prid, &ip);
> +			       prid, &ip, XFS_ILOCK_EXCL);
>  	if (error)
>  		goto out_trans_cancel;
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html