Re: [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()

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

On Fri, Dec 04, 2020 at 12:10:25AM +0800, Gao Xiang wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> So xfs_ialloc() will only address in-core inode allocation then.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Looks good to me, I never liked ialloc_context anyway.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_inode.c | 200 +++++++++++++++------------------------------
>  1 file changed, 65 insertions(+), 135 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 4ebfb1a18f0f..34eca1624397 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -761,68 +761,25 @@ xfs_inode_inherit_flags2(
>  }
>  
>  /*
> - * Allocate an inode on disk and return a copy of its in-core version.
> - * The in-core inode is locked exclusively.  Set mode, nlink, and rdev
> - * appropriately within the inode.  The uid and gid for the inode are
> - * set according to the contents of the given cred structure.
> - *
> - * Use xfs_dialloc() to allocate the on-disk inode. If xfs_dialloc()
> - * has a free inode available, call xfs_iget() to obtain the in-core
> - * version of the allocated inode.  Finally, fill in the inode and
> - * log its initial contents.  In this case, ialloc_context would be
> - * set to NULL.
> - *
> - * If xfs_dialloc() does not have an available inode, it will replenish
> - * its supply by doing an allocation. Since we can only do one
> - * allocation within a transaction without deadlocks, we must commit
> - * the current transaction before returning the inode itself.
> - * In this case, therefore, we will set ialloc_context and return.
> - * The caller should then commit the current transaction, start a new
> - * transaction, and call xfs_ialloc() again to actually get the inode.
> - *
> - * To ensure that some other process does not grab the inode that
> - * was allocated during the first call to xfs_ialloc(), this routine
> - * also returns the [locked] bp pointing to the head of the freelist
> - * as ialloc_context.  The caller should hold this buffer across
> - * the commit and pass it back into this routine on the second call.
> - *
> - * If we are allocating quota inodes, we do not have a parent inode
> - * to attach to or associate with (i.e. pip == NULL) because they
> - * are not linked into the directory structure - they are attached
> - * directly to the superblock - and so have no parent.
> + * Initialise a newly allocated inode and return the in-core inode to the
> + * caller locked exclusively.
>   */
> -static int
> +static struct xfs_inode *
>  xfs_ialloc(
> -	xfs_trans_t	*tp,
> -	xfs_inode_t	*pip,
> -	umode_t		mode,
> -	xfs_nlink_t	nlink,
> -	dev_t		rdev,
> -	prid_t		prid,
> -	xfs_buf_t	**ialloc_context,
> -	xfs_inode_t	**ipp)
> +	struct xfs_trans	*tp,
> +	struct xfs_inode	*pip,
> +	xfs_ino_t		ino,
> +	umode_t			mode,
> +	xfs_nlink_t		nlink,
> +	dev_t			rdev,
> +	prid_t			prid)
>  {
> -	struct xfs_mount *mp = tp->t_mountp;
> -	xfs_ino_t	ino;
> -	xfs_inode_t	*ip;
> -	uint		flags;
> -	int		error;
> -	struct timespec64 tv;
> -	struct inode	*inode;
> -
> -	/*
> -	 * Call the space management code to pick
> -	 * the on-disk inode to be allocated.
> -	 */
> -	error = xfs_dialloc(tp, pip ? pip->i_ino : 0, mode,
> -			    ialloc_context, &ino);
> -	if (error)
> -		return error;
> -	if (*ialloc_context || ino == NULLFSINO) {
> -		*ipp = NULL;
> -		return 0;
> -	}
> -	ASSERT(*ialloc_context == NULL);
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	struct xfs_inode	*ip;
> +	unsigned int		flags;
> +	int			error;
> +	struct timespec64	tv;
> +	struct inode		*inode;
>  
>  	/*
>  	 * Protect against obviously corrupt allocation btree records. Later
> @@ -833,18 +790,16 @@ xfs_ialloc(
>  	 */
>  	if ((pip && ino == pip->i_ino) || !xfs_verify_dir_ino(mp, ino)) {
>  		xfs_alert(mp, "Allocated a known in-use inode 0x%llx!", ino);
> -		return -EFSCORRUPTED;
> +		return ERR_PTR(-EFSCORRUPTED);
>  	}
>  
>  	/*
> -	 * Get the in-core inode with the lock held exclusively.
> -	 * This is because we're setting fields here we need
> -	 * to prevent others from looking at until we're done.
> +	 * Get the in-core inode with the lock held exclusively to prevent
> +	 * others from looking at until we're done.
>  	 */
> -	error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE,
> -			 XFS_ILOCK_EXCL, &ip);
> +	error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, &ip);
>  	if (error)
> -		return error;
> +		return ERR_PTR(error);
>  	ASSERT(ip != NULL);
>  	inode = VFS_I(ip);
>  	inode->i_mode = mode;
> @@ -926,20 +881,19 @@ xfs_ialloc(
>  
>  	/* now that we have an i_mode we can setup the inode structure */
>  	xfs_setup_inode(ip);
> -
> -	*ipp = ip;
> -	return 0;
> +	return ip;
>  }
>  
>  /*
> - * Allocates a new inode from disk and return a pointer to the
> - * incore copy. This routine will internally commit the current
> - * transaction and allocate a new one if the Space Manager needed
> - * to do an allocation to replenish the inode free-list.
> - *
> - * This routine is designed to be called from xfs_create and
> - * xfs_create_dir.
> + * Allocates a new inode from disk and return a pointer to the incore copy. This
> + * routine will internally commit the current transaction and allocate a new one
> + * if we needed to allocate more on-disk free inodes to perform the requested
> + * operation.
>   *
> + * If we are allocating quota inodes, we do not have a parent inode to attach to
> + * or associate with (i.e. dp == NULL) because they are not linked into the
> + * directory structure - they are attached directly to the superblock - and so
> + * have no parent.
>   */
>  int
>  xfs_dir_ialloc(
> @@ -954,83 +908,59 @@ xfs_dir_ialloc(
>  	xfs_inode_t	**ipp)		/* pointer to inode; it will be
>  					   locked. */
>  {
> -	xfs_trans_t	*tp;
>  	xfs_inode_t	*ip;
>  	xfs_buf_t	*ialloc_context = NULL;
> -	int		code;
> -
> -	tp = *tpp;
> -	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> +	xfs_ino_t	pino = dp ? dp->i_ino : 0;
> +	xfs_ino_t	ino;
> +	int		error;
>  
> -	/*
> -	 * xfs_ialloc will return a pointer to an incore inode if
> -	 * the Space Manager has an available inode on the free
> -	 * list. Otherwise, it will do an allocation and replenish
> -	 * the freelist.  Since we can only do one allocation per
> -	 * transaction without deadlocks, we will need to commit the
> -	 * current transaction and start a new one.  We will then
> -	 * need to call xfs_ialloc again to get the inode.
> -	 *
> -	 * If xfs_ialloc did an allocation to replenish the freelist,
> -	 * it returns the bp containing the head of the freelist as
> -	 * ialloc_context. We will hold a lock on it across the
> -	 * transaction commit so that no other process can steal
> -	 * the inode(s) that we've just allocated.
> -	 */
> -	code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
> -			&ip);
> +	ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
> +	*ipp = NULL;
>  
>  	/*
> -	 * Return an error if we were unable to allocate a new inode.
> -	 * This should only happen if we run out of space on disk or
> -	 * encounter a disk error.
> +	 * Call the space management code to pick the on-disk inode to be
> +	 * allocated and replenish the freelist.  Since we can only do one
> +	 * allocation per transaction without deadlocks, we will need to
> +	 * commit the current transaction and start a new one.
> +	 * If xfs_dialloc did an allocation to replenish the freelist, it
> +	 * returns the bp containing the head of the freelist as
> +	 * ialloc_context. We will hold a lock on it across the transaction
> +	 * commit so that no other process can steal the inode(s) that we've
> +	 * just allocated.
>  	 */
> -	if (code) {
> -		*ipp = NULL;
> -		return code;
> -	}
> -	if (!ialloc_context && !ip) {
> -		*ipp = NULL;
> -		return -ENOSPC;
> -	}
> +	error = xfs_dialloc(*tpp, pino, mode, ialloc_context, &ino);
> +	if (error)
> +		return error;
>  
>  	/*
>  	 * If the AGI buffer is non-NULL, then we were unable to get an
>  	 * inode in one operation.  We need to commit the current
> -	 * transaction and call xfs_ialloc() again.  It is guaranteed
> +	 * transaction and call xfs_ialloc() then.  It is guaranteed
>  	 * to succeed the second time.
>  	 */
>  	if (ialloc_context) {
> -		code = xfs_dialloc_roll(&tp, ialloc_context);
> -		if (code) {
> -			*ipp = NULL;
> -			return code;
> -		}
> -
> -		/*
> -		 * Call ialloc again. Since we've locked out all
> -		 * other allocations in this allocation group,
> -		 * this call should always succeed.
> -		 */
> -		code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
> -				  &ialloc_context, &ip);
> -
> +		error = xfs_dialloc_roll(tpp, ialloc_context);
> +		if (error)
> +			return error;
>  		/*
> -		 * If we get an error at this point, return to the caller
> -		 * so that the current transaction can be aborted.
> +		 * Call dialloc again. Since we've locked out all other
> +		 * allocations in this allocation group, this call should
> +		 * always succeed.
>  		 */
> -		if (code) {
> -			*tpp = tp;
> -			*ipp = NULL;
> -			return code;
> -		}
> -		ASSERT(!ialloc_context && ip);
> -
> +		error = xfs_dialloc(*tpp, pino, mode, ialloc_context, &ino);
> +		if (error)
> +			return error;
> +		ASSERT(!ialloc_context);
>  	}
>  
> -	*ipp = ip;
> -	*tpp = tp;
> +	if (ino == NULLFSINO)
> +		return -ENOSPC;
>  
> +	/* Initialise the newly allocated inode. */
> +	ip = xfs_ialloc(*tpp, dp, ino, mode, nlink, rdev, prid);
> +	if (IS_ERR(ip))
> +		return PTR_ERR(ip);
> +	*ipp = ip;
>  	return 0;
>  }
>  
> -- 
> 2.18.4
>