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

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 05/07/2018 03:30 PM, Darrick J. Wong wrote:
> 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@xxxxxxxxxx>
> > ---
> >   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

Sure, I can update the comment.  Hmm, would an op_flag be a more 
appropriate parameter in this case?  Thx!

Allison

> >   {
> >   	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@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html