Re: [PATCH v2 5/6] xfs: spilt xfs_dialloc() into 2 functions

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

On Fri, Dec 04, 2020 at 12:10:27AM +0800, Gao Xiang wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> This patch explicitly separates free inode chunk allocation and
> inode allocation into two individual high level operations.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

FWIW I thought about doing some similar things with the xfs_dir_ialloc
in the metadata directory tree patchset, so this makes sense to me (and
will probably simplify things) so:

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

--D

> ---
>  fs/xfs/libxfs/xfs_ialloc.c | 59 +++++++++++++++++---------------------
>  fs/xfs/libxfs/xfs_ialloc.h | 20 +++++++++----
>  fs/xfs/xfs_inode.c         | 19 ++++++++----
>  3 files changed, 55 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index d2d7378abf49..597629353d4d 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1570,7 +1570,7 @@ xfs_dialloc_ag_update_inobt(
>   * The caller selected an AG for us, and made sure that free inodes are
>   * available.
>   */
> -STATIC int
> +int
>  xfs_dialloc_ag(
>  	struct xfs_trans	*tp,
>  	struct xfs_buf		*agbp,
> @@ -1728,21 +1728,22 @@ xfs_dialloc_roll(
>  }
>  
>  /*
> - * Allocate an inode on disk.
> + * Select and prepare an AG for inode allocation.
>   *
> - * Mode is used to tell whether the new inode will need space, and whether it
> - * is a directory.
> + * Mode is used to tell whether the new inode is a directory and hence where to
> + * locate it.
>   *
> - * Once we successfully pick an inode its number is returned and the on-disk
> - * data structures are updated.  The inode itself is not read in, since doing so
> - * would break ordering constraints with xfs_reclaim.
> + * This function will ensure that the selected AG has free inodes available to
> + * allocate from. The selected AGI will be returned locked to the caller, and it
> + * will allocate more free inodes if required. If no free inodes are found or
> + * can be allocated, no AGI will be returned.
>   */
>  int
> -xfs_dialloc(
> +xfs_dialloc_select_ag(
>  	struct xfs_trans	**tpp,
>  	xfs_ino_t		parent,
>  	umode_t			mode,
> -	xfs_ino_t		*inop)
> +	struct xfs_buf		**IO_agbp)
>  {
>  	struct xfs_mount	*mp = (*tpp)->t_mountp;
>  	struct xfs_buf		*agbp;
> @@ -1755,15 +1756,15 @@ xfs_dialloc(
>  	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
>  	bool			okalloc = true;
>  
> +	*IO_agbp = NULL;
> +
>  	/*
>  	 * We do not have an agbp, so select an initial allocation
>  	 * group for inode allocation.
>  	 */
>  	start_agno = xfs_ialloc_ag_select(*tpp, parent, mode);
> -	if (start_agno == NULLAGNUMBER) {
> -		*inop = NULLFSINO;
> +	if (start_agno == NULLAGNUMBER)
>  		return 0;
> -	}
>  
>  	/*
>  	 * If we have already hit the ceiling of inode blocks then clear
> @@ -1796,7 +1797,7 @@ xfs_dialloc(
>  		if (!pag->pagi_init) {
>  			error = xfs_ialloc_pagi_init(mp, *tpp, agno);
>  			if (error)
> -				goto out_error;
> +				break;
>  		}
>  
>  		/*
> @@ -1811,11 +1812,12 @@ xfs_dialloc(
>  		 */
>  		error = xfs_ialloc_read_agi(mp, *tpp, agno, &agbp);
>  		if (error)
> -			goto out_error;
> +			break;
>  
>  		if (pag->pagi_freecount) {
>  			xfs_perag_put(pag);
> -			goto out_alloc;
> +			*IO_agbp = agbp;
> +			return 0;
>  		}
>  
>  		if (!okalloc)
> @@ -1826,19 +1828,17 @@ xfs_dialloc(
>  		if (error) {
>  			xfs_trans_brelse(*tpp, agbp);
>  
> -			if (error != -ENOSPC)
> -				goto out_error;
> -
> -			xfs_perag_put(pag);
> -			*inop = NULLFSINO;
> -			return 0;
> +			if (error == -ENOSPC)
> +				error = 0;
> +			break;
>  		}
>  
>  		if (ialloced) {
>  			/*
> -			 * We successfully allocated some inodes, roll the
> -			 * transaction so they can allocate one of the free
> -			 * inodes we just prepared for them.
> +			 * We successfully allocated some inodes, so roll the
> +			 * transaction and return the locked AGI buffer to the
> +			 * caller so they can allocate one of the free inodes we
> +			 * just prepared for them.
>  			 */
>  			ASSERT(pag->pagi_freecount > 0);
>  			xfs_perag_put(pag);
> @@ -1847,8 +1847,8 @@ xfs_dialloc(
>  			if (error)
>  				return error;
>  
> -			*inop = NULLFSINO;
> -			goto out_alloc;
> +			*IO_agbp = agbp;
> +			return 0;
>  		}
>  
>  nextag_relse_buffer:
> @@ -1857,15 +1857,10 @@ xfs_dialloc(
>  		xfs_perag_put(pag);
>  		if (++agno == mp->m_sb.sb_agcount)
>  			agno = 0;
> -		if (agno == start_agno) {
> -			*inop = NULLFSINO;
> +		if (agno == start_agno)
>  			return noroom ? -ENOSPC : 0;
> -		}
>  	}
>  
> -out_alloc:
> -	return xfs_dialloc_ag(*tpp, agbp, parent, inop);
> -out_error:
>  	xfs_perag_put(pag);
>  	return error;
>  }
> diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
> index 13810ffe4af9..3511086a7ae1 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.h
> +++ b/fs/xfs/libxfs/xfs_ialloc.h
> @@ -37,16 +37,26 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
>   * Mode is used to tell whether the new inode will need space, and whether
>   * it is a directory.
>   *
> - * Once we successfully pick an inode its number is returned and the
> - * on-disk data structures are updated.  The inode itself is not read
> - * in, since doing so would break ordering constraints with xfs_reclaim.
> + * There are two phases to inode allocation: selecting an AG and ensuring
> + * that it contains free inodes, followed by allocating one of the free
> + * inodes. xfs_dialloc_select_ag() does the former and returns a locked AGI
> + * to the caller, ensuring that followup call to xfs_dialloc_ag() will
> + * have free inodes to allocate from. xfs_dialloc_ag() will return the inode
> + * number of the free inode we allocated.
>   */
>  int					/* error */
> -xfs_dialloc(
> +xfs_dialloc_select_ag(
>  	struct xfs_trans **tpp,		/* double pointer of transaction */
>  	xfs_ino_t	parent,		/* parent inode (directory) */
>  	umode_t		mode,		/* mode bits for new inode */
> -	xfs_ino_t	*inop);		/* inode number allocated */
> +	struct xfs_buf	**IO_agbp);
> +
> +int
> +xfs_dialloc_ag(
> +	struct xfs_trans	*tp,
> +	struct xfs_buf		*agbp,
> +	xfs_ino_t		parent,
> +	xfs_ino_t		*inop);
>  
>  /*
>   * Free disk inode.  Carefully avoids touching the incore inode, all
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c039fc56b396..d0ae0d6ee892 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -908,10 +908,11 @@ xfs_dir_ialloc(
>  	xfs_inode_t	**ipp)		/* pointer to inode; it will be
>  					   locked. */
>  {
> -	xfs_inode_t	*ip;
> -	xfs_ino_t	pino = dp ? dp->i_ino : 0;
> -	xfs_ino_t	ino;
> -	int		error;
> +	struct xfs_buf		*agibp;
> +	struct xfs_inode	*ip;
> +	xfs_ino_t		pino = dp ? dp->i_ino : 0;
> +	xfs_ino_t		ino;
> +	int			error;
>  
>  	ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
>  	*ipp = NULL;
> @@ -927,13 +928,19 @@ xfs_dir_ialloc(
>  	 * commit so that no other process can steal the inode(s) that we've
>  	 * just allocated.
>  	 */
> -	error = xfs_dialloc(tpp, pino, mode, &ino);
> +	error = xfs_dialloc_select_ag(tpp, pino, mode, &agibp);
>  	if (error)
>  		return error;
>  
> -	if (ino == NULLFSINO)
> +	if (!agibp)
>  		return -ENOSPC;
>  
> +	/* Allocate an inode from the selected AG */
> +	error = xfs_dialloc_ag(*tpp, agibp, pino, &ino);
> +	if (error)
> +		return error;
> +	ASSERT(ino != NULLFSINO);
> +
>  	/* Initialise the newly allocated inode. */
>  	ip = xfs_ialloc(*tpp, dp, ino, mode, nlink, rdev, prid);
>  	if (IS_ERR(ip))
> -- 
> 2.18.4
>