Re: [PATCH 17/22] xfs: simplify xfs_dialloc_select_ag() return values

[Brian Foster <bfoster@redhat.com>]

On Thu, May 06, 2021 at 05:20:49PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The only caller of xfs_dialloc_select_ag() will always return
> -ENOSPC to it's caller if the agbp returned from
> xfs_dialloc_select_ag() is NULL. IOWs, failure to find a candidate
> AGI we can allocate inodes from is always an ENOSPC condition, so
> move this logic up into xfs_dialloc_select_ag() so we can simplify
> the return logic in this function.
> 
> xfs_dialloc_select_ag() now only ever returns 0 with a locked
> agbp, or an error with no agbp.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_ialloc.c | 23 ++++++++---------------
>  fs/xfs/xfs_inode.c         |  3 ---
>  2 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 4540fbcd68a3..872591e8f5cb 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1717,7 +1717,7 @@ xfs_dialloc_roll(
>   * 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.
> + * can be allocated, -ENOSPC be returned.
>   */
>  int
>  xfs_dialloc_select_ag(
> @@ -1730,7 +1730,6 @@ xfs_dialloc_select_ag(
>  	struct xfs_buf		*agbp;
>  	xfs_agnumber_t		agno;
>  	int			error;
> -	bool			noroom = false;
>  	xfs_agnumber_t		start_agno;
>  	struct xfs_perag	*pag;
>  	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> @@ -1744,7 +1743,7 @@ xfs_dialloc_select_ag(
>  	 */
>  	start_agno = xfs_ialloc_ag_select(*tpp, parent, mode);
>  	if (start_agno == NULLAGNUMBER)
> -		return 0;
> +		return -ENOSPC;
>  
>  	/*
>  	 * If we have already hit the ceiling of inode blocks then clear
> @@ -1757,7 +1756,6 @@ xfs_dialloc_select_ag(
>  	if (igeo->maxicount &&
>  	    percpu_counter_read_positive(&mp->m_icount) + igeo->ialloc_inos
>  							> igeo->maxicount) {
> -		noroom = true;
>  		okalloc = false;
>  	}
>  
> @@ -1794,10 +1792,8 @@ xfs_dialloc_select_ag(
>  		if (error)
>  			break;
>  
> -		if (pag->pagi_freecount) {
> -			xfs_perag_put(pag);
> +		if (pag->pagi_freecount)
>  			goto found_ag;
> -		}
>  
>  		if (!okalloc)
>  			goto nextag_relse_buffer;
> @@ -1805,9 +1801,6 @@ xfs_dialloc_select_ag(
>  		error = xfs_ialloc_ag_alloc(*tpp, agbp, pag);
>  		if (error < 0) {
>  			xfs_trans_brelse(*tpp, agbp);
> -
> -			if (error == -ENOSPC)
> -				error = 0;
>  			break;
>  		}
>  
> @@ -1818,12 +1811,11 @@ xfs_dialloc_select_ag(
>  			 * allocate one of the new inodes.
>  			 */
>  			ASSERT(pag->pagi_freecount > 0);
> -			xfs_perag_put(pag);
>  
>  			error = xfs_dialloc_roll(tpp, agbp);
>  			if (error) {
>  				xfs_buf_relse(agbp);
> -				return error;
> +				break;
>  			}
>  			goto found_ag;
>  		}
> @@ -1831,16 +1823,17 @@ xfs_dialloc_select_ag(
>  nextag_relse_buffer:
>  		xfs_trans_brelse(*tpp, agbp);
>  nextag:
> -		xfs_perag_put(pag);
>  		if (++agno == mp->m_sb.sb_agcount)
>  			agno = 0;
>  		if (agno == start_agno)
> -			return noroom ? -ENOSPC : 0;
> +			break;
> +		xfs_perag_put(pag);
>  	}
>  
>  	xfs_perag_put(pag);
> -	return error;
> +	return error ? error : -ENOSPC;
>  found_ag:
> +	xfs_perag_put(pag);
>  	*IO_agbp = agbp;
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 25910b145d70..3918c99fa95b 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -923,9 +923,6 @@ xfs_dir_ialloc(
>  	if (error)
>  		return error;
>  
> -	if (!agibp)
> -		return -ENOSPC;
> -
>  	/* Allocate an inode from the selected AG */
>  	error = xfs_dialloc_ag(*tpp, agibp, parent_ino, &ino);
>  	if (error)
> -- 
> 2.31.1
>