Re: [PATCH 2/3] xfs: kill ialloced in xfs_dialloc()

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

On Tue, Nov 24, 2020 at 11:51:29PM +0800, Gao Xiang wrote:
> It's enough to just use return code, and get rid of an argument.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ialloc.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 45cf7e55f5ee..5c8b0210aad3 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -607,13 +607,14 @@ xfs_inobt_insert_sprec(
>  
>  /*
>   * Allocate new inodes in the allocation group specified by agbp.
> - * Return 0 for success, else error code.
> + * Return 0 for successfully allocating some inodes in this AG;
> + *        1 for skipping to allocating in the next AG;
> + *      < 0 for error code.
>   */
>  STATIC int
>  xfs_ialloc_ag_alloc(
>  	struct xfs_trans	*tp,
> -	struct xfs_buf		*agbp,
> -	int			*alloc)
> +	struct xfs_buf		*agbp)
>  {
>  	struct xfs_agi		*agi;
>  	struct xfs_alloc_arg	args;
> @@ -795,10 +796,9 @@ xfs_ialloc_ag_alloc(
>  		allocmask = (1 << (newlen / XFS_INODES_PER_HOLEMASK_BIT)) - 1;
>  	}
>  
> -	if (args.fsbno == NULLFSBLOCK) {
> -		*alloc = 0;
> -		return 0;
> -	}
> +	if (args.fsbno == NULLFSBLOCK)
> +		return 1;
> +
>  	ASSERT(args.len == args.minlen);
>  
>  	/*
> @@ -903,7 +903,6 @@ xfs_ialloc_ag_alloc(
>  	 */
>  	xfs_trans_mod_sb(tp, XFS_TRANS_SB_ICOUNT, (long)newlen);
>  	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, (long)newlen);
> -	*alloc = 1;
>  	return 0;
>  }
>  
> @@ -1715,7 +1714,6 @@ xfs_dialloc(
>  	struct xfs_buf		*agbp;
>  	xfs_agnumber_t		agno;
>  	int			error;
> -	int			ialloced;
>  	bool			noroom = false;
>  	xfs_agnumber_t		start_agno;
>  	struct xfs_perag	*pag;
> @@ -1799,8 +1797,8 @@ xfs_dialloc(
>  			goto nextag_relse_buffer;
>  
>  
> -		error = xfs_ialloc_ag_alloc(tp, agbp, &ialloced);
> -		if (error) {
> +		error = xfs_ialloc_ag_alloc(tp, agbp);
> +		if (error < 0) {
>  			xfs_trans_brelse(tp, agbp);
>  
>  			if (error != -ENOSPC)
> @@ -1811,7 +1809,7 @@ xfs_dialloc(
>  			return 0;
>  		}
>  
> -		if (ialloced) {
> +		if (!error) {

I wonder if this should be "if (error == 0)" because the comment
for _ialloc_ag_alloc says that 0 and 1 have specific meanings?

Otherwise looks fine to me.

--D

>  			/*
>  			 * We successfully allocated some inodes, return
>  			 * the current context to the caller so that it
> -- 
> 2.18.4
>