[14/42] xfs: introduce xfs_for_each_perag_wrap()

Message ID	20230118224505.1964941-15-david@fromorbit.com (mailing list archive)
State	Superseded, archived
Headers	show Return-Path: <linux-xfs-owner@vger.kernel.org> From: Dave Chinner <david@fromorbit.com> To: linux-xfs@vger.kernel.org Subject: [PATCH 14/42] xfs: introduce xfs_for_each_perag_wrap() Date: Thu, 19 Jan 2023 09:44:37 +1100 Message-Id: <20230118224505.1964941-15-david@fromorbit.com> In-Reply-To: <20230118224505.1964941-1-david@fromorbit.com> References: <20230118224505.1964941-1-david@fromorbit.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	xfs: per-ag centric allocation alogrithms \| expand [00/42] xfs: per-ag centric allocation alogrithms [01/42] xfs: fix low space alloc deadlock [02/42] xfs: prefer free inodes at ENOSPC over chunk allocation [03/42] xfs: block reservation too large for minleft allocation [04/42] xfs: drop firstblock constraints from allocation setup [05/42] xfs: t_firstblock is tracking AGs not blocks [06/42] xfs: don't assert fail on transaction cancel with deferred ops [07/42] xfs: active perag reference counting [08/42] xfs: rework the perag trace points to be perag centric [09/42] xfs: convert xfs_imap() to take a perag [10/42] xfs: use active perag references for inode allocation [11/42] xfs: inobt can use perags in many more places than it does [12/42] xfs: convert xfs_ialloc_next_ag() to an atomic [13/42] xfs: perags need atomic operational state [14/42] xfs: introduce xfs_for_each_perag_wrap() [15/42] xfs: rework xfs_alloc_vextent() [16/42] xfs: factor xfs_alloc_vextent_this_ag() for _iterate_ags() [17/42] xfs: combine __xfs_alloc_vextent_this_ag and xfs_alloc_ag_vextent [18/42] xfs: use xfs_alloc_vextent_this_ag() where appropriate [19/42] xfs: factor xfs_bmap_btalloc() [20/42] xfs: use xfs_alloc_vextent_first_ag() where appropriate [21/42] xfs: use xfs_alloc_vextent_start_bno() where appropriate [22/42] xfs: introduce xfs_alloc_vextent_near_bno() [23/42] xfs: introduce xfs_alloc_vextent_exact_bno() [24/42] xfs: introduce xfs_alloc_vextent_prepare() [25/42] xfs: move allocation accounting to xfs_alloc_vextent_set_fsbno() [26/42] xfs: fold xfs_alloc_ag_vextent() into callers [27/42] xfs: move the minimum agno checks into xfs_alloc_vextent_check_args [28/42] xfs: convert xfs_alloc_vextent_iterate_ags() to use perag walker [29/42] xfs: convert trim to use for_each_perag_range [30/42] xfs: factor out filestreams from xfs_bmap_btalloc_nullfb [31/42] xfs: get rid of notinit from xfs_bmap_longest_free_extent [32/42] xfs: use xfs_bmap_longest_free_extent() in filestreams [33/42] xfs: move xfs_bmap_btalloc_filestreams() to xfs_filestreams.c [34/42] xfs: merge filestream AG lookup into xfs_filestream_select_ag() [35/42] xfs: merge new filestream AG selection into xfs_filestream_select_ag() [36/42] xfs: remove xfs_filestream_select_ag() longest extent check [37/42] xfs: factor out MRU hit case in xfs_filestream_select_ag [38/42] xfs: track an active perag reference in filestreams [39/42] xfs: use for_each_perag_wrap in xfs_filestream_pick_ag [40/42] xfs: pass perag to filestreams tracing [41/42] xfs: return a referenced perag from filestreams allocator [42/42] xfs: refactor the filestreams allocator pick functions

Message ID

20230118224505.1964941-15-david@fromorbit.com (mailing list archive)

State

Superseded, archived

Headers

From: Dave Chinner <david@fromorbit.com>
To: linux-xfs@vger.kernel.org
Subject: [PATCH 14/42] xfs: introduce xfs_for_each_perag_wrap()
Date: Thu, 19 Jan 2023 09:44:37 +1100
Message-Id: <20230118224505.1964941-15-david@fromorbit.com>
In-Reply-To: <20230118224505.1964941-1-david@fromorbit.com>
References: <20230118224505.1964941-1-david@fromorbit.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

xfs: per-ag centric allocation alogrithms | expand

Commit Message

Dave Chinner Jan. 18, 2023, 10:44 p.m. UTC

From: Dave Chinner <dchinner@redhat.com>

In several places we iterate every AG from a specific start agno and
wrap back to the first AG when we reach the end of the filesystem to
continue searching. We don't have a primitive for this iteration
yet, so add one for conversion of these algorithms to per-ag based
iteration.

The filestream AG select code is a mess, and this initially makes it
worse. The per-ag selection needs to be driven completely into the
filestream code to clean this up and it will be done in a future
patch that makes the filestream allocator use active per-ag
references correctly.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.h     | 45 +++++++++++++++++++++-
 fs/xfs/libxfs/xfs_bmap.c   | 76 ++++++++++++++++++++++----------------
 fs/xfs/libxfs/xfs_ialloc.c | 32 ++++++++--------
 3 files changed, 104 insertions(+), 49 deletions(-)

Comments

Allison Henderson Jan. 23, 2023, 5:41 a.m. UTC | #1

On Thu, 2023-01-19 at 09:44 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> In several places we iterate every AG from a specific start agno and
> wrap back to the first AG when we reach the end of the filesystem to
> continue searching. We don't have a primitive for this iteration
> yet, so add one for conversion of these algorithms to per-ag based
> iteration.
> 
> The filestream AG select code is a mess, and this initially makes it
> worse. The per-ag selection needs to be driven completely into the
> filestream code to clean this up and it will be done in a future
> patch that makes the filestream allocator use active per-ag
> references correctly.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag.h     | 45 +++++++++++++++++++++-
>  fs/xfs/libxfs/xfs_bmap.c   | 76 ++++++++++++++++++++++--------------
> --
>  fs/xfs/libxfs/xfs_ialloc.c | 32 ++++++++--------
>  3 files changed, 104 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 187d30d9bb13..8f43b91d4cf3 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -237,7 +237,6 @@ xfs_perag_next(
>  #define for_each_perag_from(mp, agno, pag) \
>         for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount - 1,
> (pag))
>  
> -
>  #define for_each_perag(mp, agno, pag) \
>         (agno) = 0; \
>         for_each_perag_from((mp), (agno), (pag))
> @@ -249,6 +248,50 @@ xfs_perag_next(
>                 xfs_perag_rele(pag), \
>                 (pag) = xfs_perag_grab_tag((mp), (agno), (tag)))
>  
> +static inline struct xfs_perag *
> +xfs_perag_next_wrap(
> +       struct xfs_perag        *pag,
> +       xfs_agnumber_t          *agno,
> +       xfs_agnumber_t          stop_agno,
> +       xfs_agnumber_t          wrap_agno)
> +{
> +       struct xfs_mount        *mp = pag->pag_mount;
> +
> +       *agno = pag->pag_agno + 1;
> +       xfs_perag_rele(pag);
> +       while (*agno != stop_agno) {
> +               if (*agno >= wrap_agno)
> +                       *agno = 0;
> +               if (*agno == stop_agno)
> +                       break;
> +
> +               pag = xfs_perag_grab(mp, *agno);
> +               if (pag)
> +                       return pag;
> +               (*agno)++;
> +       }
> +       return NULL;
> +}
> +
> +/*
> + * Iterate all AGs from start_agno through wrap_agno, then 0 through
> + * (start_agno - 1).
> + */
> +#define for_each_perag_wrap_at(mp, start_agno, wrap_agno, agno, pag)
> \
> +       for ((agno) = (start_agno), (pag) = xfs_perag_grab((mp),
> (agno)); \
> +               (pag) != NULL; \
> +               (pag) = xfs_perag_next_wrap((pag), &(agno),
> (start_agno), \
> +                               (wrap_agno)))
> +
> +/*
> + * Iterate all AGs from start_agno through to the end of the
> filesystem, then 0
> + * through (start_agno - 1).
> + */
> +#define for_each_perag_wrap(mp, start_agno, agno, pag) \
> +       for_each_perag_wrap_at((mp), (start_agno), (mp)-
> >m_sb.sb_agcount, \
> +                               (agno), (pag))
> +
> +
>  struct aghdr_init_data {
>         /* per ag data */
>         xfs_agblock_t           agno;           /* ag to init */
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6aad0ea5e606..e5519abbfa0d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3136,17 +3136,14 @@ xfs_bmap_adjacent(
>  
>  static int
>  xfs_bmap_longest_free_extent(
> +       struct xfs_perag        *pag,
>         struct xfs_trans        *tp,
> -       xfs_agnumber_t          ag,
>         xfs_extlen_t            *blen,
>         int                     *notinit)
>  {
> -       struct xfs_mount        *mp = tp->t_mountp;
> -       struct xfs_perag        *pag;
>         xfs_extlen_t            longest;
>         int                     error = 0;
>  
> -       pag = xfs_perag_get(mp, ag);
>         if (!xfs_perag_initialised_agf(pag)) {
>                 error = xfs_alloc_read_agf(pag, tp,
> XFS_ALLOC_FLAG_TRYLOCK,
>                                 NULL);
> @@ -3156,19 +3153,17 @@ xfs_bmap_longest_free_extent(
>                                 *notinit = 1;
>                                 error = 0;
>                         }
> -                       goto out;
> +                       return error;
>                 }
>         }
>  
>         longest = xfs_alloc_longest_free_extent(pag,
> -                               xfs_alloc_min_freelist(mp, pag),
> +                               xfs_alloc_min_freelist(pag-
> >pag_mount, pag),
>                                 xfs_ag_resv_needed(pag,
> XFS_AG_RESV_NONE));
>         if (*blen < longest)
>                 *blen = longest;
>  
> -out:
> -       xfs_perag_put(pag);
> -       return error;
> +       return 0;
>  }
>  
>  static void
> @@ -3206,9 +3201,10 @@ xfs_bmap_btalloc_select_lengths(
>         xfs_extlen_t            *blen)
>  {
>         struct xfs_mount        *mp = ap->ip->i_mount;
> -       xfs_agnumber_t          ag, startag;
> +       struct xfs_perag        *pag;
> +       xfs_agnumber_t          agno, startag;
>         int                     notinit = 0;
> -       int                     error;
> +       int                     error = 0;
>  
>         args->type = XFS_ALLOCTYPE_START_BNO;
>         if (ap->tp->t_flags & XFS_TRANS_LOWMODE) {
> @@ -3218,21 +3214,21 @@ xfs_bmap_btalloc_select_lengths(
>         }
>  
>         args->total = ap->total;
> -       startag = ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
> +       startag = XFS_FSB_TO_AGNO(mp, args->fsbno);
>         if (startag == NULLAGNUMBER)
> -               startag = ag = 0;
> +               startag = 0;
>  
> -       while (*blen < args->maxlen) {
> -               error = xfs_bmap_longest_free_extent(args->tp, ag,
> blen,
> +       *blen = 0;
> +       for_each_perag_wrap(mp, startag, agno, pag) {
> +               error = xfs_bmap_longest_free_extent(pag, args->tp,
> blen,
>                                                      &notinit);
>                 if (error)
> -                       return error;
> -
> -               if (++ag == mp->m_sb.sb_agcount)
> -                       ag = 0;
> -               if (ag == startag)
> +                       break;
> +               if (*blen >= args->maxlen)
>                         break;
>         }
> +       if (pag)
> +               xfs_perag_rele(pag);
>  
>         xfs_bmap_select_minlen(ap, args, blen, notinit);
>         return 0;
Hmm, did you want to return error here?  Since now we only break on
error in the loop body above?

Otherwise looks good.
Allison

> @@ -3245,7 +3241,8 @@ xfs_bmap_btalloc_filestreams(
>         xfs_extlen_t            *blen)
>  {
>         struct xfs_mount        *mp = ap->ip->i_mount;
> -       xfs_agnumber_t          ag;
> +       struct xfs_perag        *pag;
> +       xfs_agnumber_t          start_agno;
>         int                     notinit = 0;
>         int                     error;
>  
> @@ -3259,33 +3256,50 @@ xfs_bmap_btalloc_filestreams(
>         args->type = XFS_ALLOCTYPE_NEAR_BNO;
>         args->total = ap->total;
>  
> -       ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
> -       if (ag == NULLAGNUMBER)
> -               ag = 0;
> +       start_agno = XFS_FSB_TO_AGNO(mp, args->fsbno);
> +       if (start_agno == NULLAGNUMBER)
> +               start_agno = 0;
>  
> -       error = xfs_bmap_longest_free_extent(args->tp, ag, blen,
> &notinit);
> -       if (error)
> -               return error;
> +       pag = xfs_perag_grab(mp, start_agno);
> +       if (pag) {
> +               error = xfs_bmap_longest_free_extent(pag, args->tp,
> blen,
> +                               &notinit);
> +               xfs_perag_rele(pag);
> +               if (error)
> +                       return error;
> +       }
>  
>         if (*blen < args->maxlen) {
> -               error = xfs_filestream_new_ag(ap, &ag);
> +               xfs_agnumber_t  agno = start_agno;
> +
> +               error = xfs_filestream_new_ag(ap, &agno);
>                 if (error)
>                         return error;
> +               if (agno == NULLAGNUMBER)
> +                       goto out_select;
>  
> -               error = xfs_bmap_longest_free_extent(args->tp, ag,
> blen,
> -                                                    &notinit);
> +               pag = xfs_perag_grab(mp, agno);
> +               if (!pag)
> +                       goto out_select;
> +
> +               error = xfs_bmap_longest_free_extent(pag, args->tp,
> +                               blen, &notinit);
> +               xfs_perag_rele(pag);
>                 if (error)
>                         return error;
>  
> +               start_agno = agno;
> +
>         }
>  
> +out_select:
>         xfs_bmap_select_minlen(ap, args, blen, notinit);
>  
>         /*
>          * Set the failure fallback case to look in the selected AG
> as stream
>          * may have moved.
>          */
> -       ap->blkno = args->fsbno = XFS_AGB_TO_FSB(mp, ag, 0);
> +       ap->blkno = args->fsbno = XFS_AGB_TO_FSB(mp, start_agno, 0);
>         return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 2a323ffa5ba9..50fef3f5af51 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1725,7 +1725,7 @@ xfs_dialloc(
>         bool                    ok_alloc = true;
>         bool                    low_space = false;
>         int                     flags;
> -       xfs_ino_t               ino;
> +       xfs_ino_t               ino = NULLFSINO;
>  
>         /*
>          * Directories, symlinks, and regular files frequently
> allocate at least
> @@ -1773,39 +1773,37 @@ xfs_dialloc(
>          * or in which we can allocate some inodes.  Iterate through
> the
>          * allocation groups upward, wrapping at the end.
>          */
> -       agno = start_agno;
>         flags = XFS_ALLOC_FLAG_TRYLOCK;
> -       for (;;) {
> -               pag = xfs_perag_grab(mp, agno);
> +retry:
> +       for_each_perag_wrap_at(mp, start_agno, mp->m_maxagi, agno,
> pag) {
>                 if (xfs_dialloc_good_ag(pag, *tpp, mode, flags,
> ok_alloc)) {
>                         error = xfs_dialloc_try_ag(pag, tpp, parent,
>                                         &ino, ok_alloc);
>                         if (error != -EAGAIN)
>                                 break;
> +                       error = 0;
>                 }
>  
>                 if (xfs_is_shutdown(mp)) {
>                         error = -EFSCORRUPTED;
>                         break;
>                 }
> -               if (++agno == mp->m_maxagi)
> -                       agno = 0;
> -               if (agno == start_agno) {
> -                       if (!flags) {
> -                               error = -ENOSPC;
> -                               break;
> -                       }
> +       }
> +       if (pag)
> +               xfs_perag_rele(pag);
> +       if (error)
> +               return error;
> +       if (ino == NULLFSINO) {
> +               if (flags) {
>                         flags = 0;
>                         if (low_space)
>                                 ok_alloc = true;
> +                       goto retry;
>                 }
> -               xfs_perag_rele(pag);
> +               return -ENOSPC;
>         }
> -
> -       if (!error)
> -               *new_ino = ino;
> -       xfs_perag_rele(pag);
> -       return error;
> +       *new_ino = ino;
> +       return 0;
>  }
>  
>  /*

Darrick J. Wong Feb. 1, 2023, 7:28 p.m. UTC | #2

On Thu, Jan 19, 2023 at 09:44:37AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> In several places we iterate every AG from a specific start agno and
> wrap back to the first AG when we reach the end of the filesystem to
> continue searching. We don't have a primitive for this iteration
> yet, so add one for conversion of these algorithms to per-ag based
> iteration.
> 
> The filestream AG select code is a mess, and this initially makes it
> worse. The per-ag selection needs to be driven completely into the
> filestream code to clean this up and it will be done in a future
> patch that makes the filestream allocator use active per-ag
> references correctly.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag.h     | 45 +++++++++++++++++++++-
>  fs/xfs/libxfs/xfs_bmap.c   | 76 ++++++++++++++++++++++----------------
>  fs/xfs/libxfs/xfs_ialloc.c | 32 ++++++++--------
>  3 files changed, 104 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 187d30d9bb13..8f43b91d4cf3 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -237,7 +237,6 @@ xfs_perag_next(
>  #define for_each_perag_from(mp, agno, pag) \
>  	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount - 1, (pag))
>  
> -
>  #define for_each_perag(mp, agno, pag) \
>  	(agno) = 0; \
>  	for_each_perag_from((mp), (agno), (pag))
> @@ -249,6 +248,50 @@ xfs_perag_next(
>  		xfs_perag_rele(pag), \
>  		(pag) = xfs_perag_grab_tag((mp), (agno), (tag)))
>  
> +static inline struct xfs_perag *
> +xfs_perag_next_wrap(
> +	struct xfs_perag	*pag,
> +	xfs_agnumber_t		*agno,
> +	xfs_agnumber_t		stop_agno,
> +	xfs_agnumber_t		wrap_agno)
> +{
> +	struct xfs_mount	*mp = pag->pag_mount;
> +
> +	*agno = pag->pag_agno + 1;
> +	xfs_perag_rele(pag);
> +	while (*agno != stop_agno) {
> +		if (*agno >= wrap_agno)
> +			*agno = 0;
> +		if (*agno == stop_agno)
> +			break;
> +
> +		pag = xfs_perag_grab(mp, *agno);
> +		if (pag)
> +			return pag;
> +		(*agno)++;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * Iterate all AGs from start_agno through wrap_agno, then 0 through
> + * (start_agno - 1).
> + */
> +#define for_each_perag_wrap_at(mp, start_agno, wrap_agno, agno, pag) \
> +	for ((agno) = (start_agno), (pag) = xfs_perag_grab((mp), (agno)); \
> +		(pag) != NULL; \
> +		(pag) = xfs_perag_next_wrap((pag), &(agno), (start_agno), \
> +				(wrap_agno)))
> +
> +/*
> + * Iterate all AGs from start_agno through to the end of the filesystem, then 0
> + * through (start_agno - 1).
> + */
> +#define for_each_perag_wrap(mp, start_agno, agno, pag) \
> +	for_each_perag_wrap_at((mp), (start_agno), (mp)->m_sb.sb_agcount, \
> +				(agno), (pag))

This seems like a useful new iterator.  I like that the opencoded loops
finally got cleaned up.

> +
> +
>  struct aghdr_init_data {
>  	/* per ag data */
>  	xfs_agblock_t		agno;		/* ag to init */
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6aad0ea5e606..e5519abbfa0d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c

<snip>

> @@ -3218,21 +3214,21 @@ xfs_bmap_btalloc_select_lengths(
>  	}
>  
>  	args->total = ap->total;
> -	startag = ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
> +	startag = XFS_FSB_TO_AGNO(mp, args->fsbno);
>  	if (startag == NULLAGNUMBER)
> -		startag = ag = 0;
> +		startag = 0;
>  
> -	while (*blen < args->maxlen) {
> -		error = xfs_bmap_longest_free_extent(args->tp, ag, blen,
> +	*blen = 0;
> +	for_each_perag_wrap(mp, startag, agno, pag) {
> +		error = xfs_bmap_longest_free_extent(pag, args->tp, blen,
>  						     &notinit);
>  		if (error)
> -			return error;
> -
> -		if (++ag == mp->m_sb.sb_agcount)
> -			ag = 0;
> -		if (ag == startag)
> +			break;
> +		if (*blen >= args->maxlen)
>  			break;
>  	}
> +	if (pag)
> +		xfs_perag_rele(pag);
>  
>  	xfs_bmap_select_minlen(ap, args, blen, notinit);
>  	return 0;

Same question as Allison -- if xfs_bmap_longest_free_extent returned a
non-EAGAIN error code, don't we want to return that to the caller?

--D

> @@ -3245,7 +3241,8 @@ xfs_bmap_btalloc_filestreams(
>  	xfs_extlen_t		*blen)
>  {
>  	struct xfs_mount	*mp = ap->ip->i_mount;
> -	xfs_agnumber_t		ag;
> +	struct xfs_perag	*pag;
> +	xfs_agnumber_t		start_agno;
>  	int			notinit = 0;
>  	int			error;
>  
> @@ -3259,33 +3256,50 @@ xfs_bmap_btalloc_filestreams(
>  	args->type = XFS_ALLOCTYPE_NEAR_BNO;
>  	args->total = ap->total;
>  
> -	ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
> -	if (ag == NULLAGNUMBER)
> -		ag = 0;
> +	start_agno = XFS_FSB_TO_AGNO(mp, args->fsbno);
> +	if (start_agno == NULLAGNUMBER)
> +		start_agno = 0;
>  
> -	error = xfs_bmap_longest_free_extent(args->tp, ag, blen, &notinit);
> -	if (error)
> -		return error;
> +	pag = xfs_perag_grab(mp, start_agno);
> +	if (pag) {
> +		error = xfs_bmap_longest_free_extent(pag, args->tp, blen,
> +				&notinit);
> +		xfs_perag_rele(pag);
> +		if (error)
> +			return error;
> +	}
>  
>  	if (*blen < args->maxlen) {
> -		error = xfs_filestream_new_ag(ap, &ag);
> +		xfs_agnumber_t	agno = start_agno;
> +
> +		error = xfs_filestream_new_ag(ap, &agno);
>  		if (error)
>  			return error;
> +		if (agno == NULLAGNUMBER)
> +			goto out_select;
>  
> -		error = xfs_bmap_longest_free_extent(args->tp, ag, blen,
> -						     &notinit);
> +		pag = xfs_perag_grab(mp, agno);
> +		if (!pag)
> +			goto out_select;
> +
> +		error = xfs_bmap_longest_free_extent(pag, args->tp,
> +				blen, &notinit);
> +		xfs_perag_rele(pag);
>  		if (error)
>  			return error;
>  
> +		start_agno = agno;
> +
>  	}
>  
> +out_select:
>  	xfs_bmap_select_minlen(ap, args, blen, notinit);
>  
>  	/*
>  	 * Set the failure fallback case to look in the selected AG as stream
>  	 * may have moved.
>  	 */
> -	ap->blkno = args->fsbno = XFS_AGB_TO_FSB(mp, ag, 0);
> +	ap->blkno = args->fsbno = XFS_AGB_TO_FSB(mp, start_agno, 0);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 2a323ffa5ba9..50fef3f5af51 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1725,7 +1725,7 @@ xfs_dialloc(
>  	bool			ok_alloc = true;
>  	bool			low_space = false;
>  	int			flags;
> -	xfs_ino_t		ino;
> +	xfs_ino_t		ino = NULLFSINO;
>  
>  	/*
>  	 * Directories, symlinks, and regular files frequently allocate at least
> @@ -1773,39 +1773,37 @@ xfs_dialloc(
>  	 * or in which we can allocate some inodes.  Iterate through the
>  	 * allocation groups upward, wrapping at the end.
>  	 */
> -	agno = start_agno;
>  	flags = XFS_ALLOC_FLAG_TRYLOCK;
> -	for (;;) {
> -		pag = xfs_perag_grab(mp, agno);
> +retry:
> +	for_each_perag_wrap_at(mp, start_agno, mp->m_maxagi, agno, pag) {
>  		if (xfs_dialloc_good_ag(pag, *tpp, mode, flags, ok_alloc)) {
>  			error = xfs_dialloc_try_ag(pag, tpp, parent,
>  					&ino, ok_alloc);
>  			if (error != -EAGAIN)
>  				break;
> +			error = 0;
>  		}
>  
>  		if (xfs_is_shutdown(mp)) {
>  			error = -EFSCORRUPTED;
>  			break;
>  		}
> -		if (++agno == mp->m_maxagi)
> -			agno = 0;
> -		if (agno == start_agno) {
> -			if (!flags) {
> -				error = -ENOSPC;
> -				break;
> -			}
> +	}
> +	if (pag)
> +		xfs_perag_rele(pag);
> +	if (error)
> +		return error;
> +	if (ino == NULLFSINO) {
> +		if (flags) {
>  			flags = 0;
>  			if (low_space)
>  				ok_alloc = true;
> +			goto retry;
>  		}
> -		xfs_perag_rele(pag);
> +		return -ENOSPC;
>  	}
> -
> -	if (!error)
> -		*new_ino = ino;
> -	xfs_perag_rele(pag);
> -	return error;
> +	*new_ino = ino;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.39.0
>

Dave Chinner Feb. 6, 2023, 11:14 p.m. UTC | #3

On Mon, Jan 23, 2023 at 05:41:09AM +0000, Allison Henderson wrote:
> On Thu, 2023-01-19 at 09:44 +1100, Dave Chinner wrote:
> > @@ -3218,21 +3214,21 @@ xfs_bmap_btalloc_select_lengths(
> >         }
> >  
> >         args->total = ap->total;
> > -       startag = ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
> > +       startag = XFS_FSB_TO_AGNO(mp, args->fsbno);
> >         if (startag == NULLAGNUMBER)
> > -               startag = ag = 0;
> > +               startag = 0;
> >  
> > -       while (*blen < args->maxlen) {
> > -               error = xfs_bmap_longest_free_extent(args->tp, ag,
> > blen,
> > +       *blen = 0;
> > +       for_each_perag_wrap(mp, startag, agno, pag) {
> > +               error = xfs_bmap_longest_free_extent(pag, args->tp,
> > blen,
> >                                                      &notinit);
> >                 if (error)
> > -                       return error;
> > -
> > -               if (++ag == mp->m_sb.sb_agcount)
> > -                       ag = 0;
> > -               if (ag == startag)
> > +                       break;
> > +               if (*blen >= args->maxlen)
> >                         break;
> >         }
> > +       if (pag)
> > +               xfs_perag_rele(pag);
> >  
> >         xfs_bmap_select_minlen(ap, args, blen, notinit);
> >         return 0;
> Hmm, did you want to return error here?  Since now we only break on
> error in the loop body above?

Yup, good catch Allison, that needs fixing.

-Dave.

diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 187d30d9bb13..8f43b91d4cf3 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -237,7 +237,6 @@  xfs_perag_next(
 #define for_each_perag_from(mp, agno, pag) \
 	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount - 1, (pag))
 
-
 #define for_each_perag(mp, agno, pag) \
 	(agno) = 0; \
 	for_each_perag_from((mp), (agno), (pag))
@@ -249,6 +248,50 @@  xfs_perag_next(
 		xfs_perag_rele(pag), \
 		(pag) = xfs_perag_grab_tag((mp), (agno), (tag)))
 
+static inline struct xfs_perag *
+xfs_perag_next_wrap(
+	struct xfs_perag	*pag,
+	xfs_agnumber_t		*agno,
+	xfs_agnumber_t		stop_agno,
+	xfs_agnumber_t		wrap_agno)
+{
+	struct xfs_mount	*mp = pag->pag_mount;
+
+	*agno = pag->pag_agno + 1;
+	xfs_perag_rele(pag);
+	while (*agno != stop_agno) {
+		if (*agno >= wrap_agno)
+			*agno = 0;
+		if (*agno == stop_agno)
+			break;
+
+		pag = xfs_perag_grab(mp, *agno);
+		if (pag)
+			return pag;
+		(*agno)++;
+	}
+	return NULL;
+}
+
+/*
+ * Iterate all AGs from start_agno through wrap_agno, then 0 through
+ * (start_agno - 1).
+ */
+#define for_each_perag_wrap_at(mp, start_agno, wrap_agno, agno, pag) \
+	for ((agno) = (start_agno), (pag) = xfs_perag_grab((mp), (agno)); \
+		(pag) != NULL; \
+		(pag) = xfs_perag_next_wrap((pag), &(agno), (start_agno), \
+				(wrap_agno)))
+
+/*
+ * Iterate all AGs from start_agno through to the end of the filesystem, then 0
+ * through (start_agno - 1).
+ */
+#define for_each_perag_wrap(mp, start_agno, agno, pag) \
+	for_each_perag_wrap_at((mp), (start_agno), (mp)->m_sb.sb_agcount, \
+				(agno), (pag))
+
+
 struct aghdr_init_data {
 	/* per ag data */
 	xfs_agblock_t		agno;		/* ag to init */
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 6aad0ea5e606..e5519abbfa0d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3136,17 +3136,14 @@  xfs_bmap_adjacent(
 
 static int
 xfs_bmap_longest_free_extent(
+	struct xfs_perag	*pag,
 	struct xfs_trans	*tp,
-	xfs_agnumber_t		ag,
 	xfs_extlen_t		*blen,
 	int			*notinit)
 {
-	struct xfs_mount	*mp = tp->t_mountp;
-	struct xfs_perag	*pag;
 	xfs_extlen_t		longest;
 	int			error = 0;
 
-	pag = xfs_perag_get(mp, ag);
 	if (!xfs_perag_initialised_agf(pag)) {
 		error = xfs_alloc_read_agf(pag, tp, XFS_ALLOC_FLAG_TRYLOCK,
 				NULL);
@@ -3156,19 +3153,17 @@  xfs_bmap_longest_free_extent(
 				*notinit = 1;
 				error = 0;
 			}
-			goto out;
+			return error;
 		}
 	}
 
 	longest = xfs_alloc_longest_free_extent(pag,
-				xfs_alloc_min_freelist(mp, pag),
+				xfs_alloc_min_freelist(pag->pag_mount, pag),
 				xfs_ag_resv_needed(pag, XFS_AG_RESV_NONE));
 	if (*blen < longest)
 		*blen = longest;
 
-out:
-	xfs_perag_put(pag);
-	return error;
+	return 0;
 }
 
 static void
@@ -3206,9 +3201,10 @@  xfs_bmap_btalloc_select_lengths(
 	xfs_extlen_t		*blen)
 {
 	struct xfs_mount	*mp = ap->ip->i_mount;
-	xfs_agnumber_t		ag, startag;
+	struct xfs_perag	*pag;
+	xfs_agnumber_t		agno, startag;
 	int			notinit = 0;
-	int			error;
+	int			error = 0;
 
 	args->type = XFS_ALLOCTYPE_START_BNO;
 	if (ap->tp->t_flags & XFS_TRANS_LOWMODE) {
@@ -3218,21 +3214,21 @@  xfs_bmap_btalloc_select_lengths(
 	}
 
 	args->total = ap->total;
-	startag = ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
+	startag = XFS_FSB_TO_AGNO(mp, args->fsbno);
 	if (startag == NULLAGNUMBER)
-		startag = ag = 0;
+		startag = 0;
 
-	while (*blen < args->maxlen) {
-		error = xfs_bmap_longest_free_extent(args->tp, ag, blen,
+	*blen = 0;
+	for_each_perag_wrap(mp, startag, agno, pag) {
+		error = xfs_bmap_longest_free_extent(pag, args->tp, blen,
 						     &notinit);
 		if (error)
-			return error;
-
-		if (++ag == mp->m_sb.sb_agcount)
-			ag = 0;
-		if (ag == startag)
+			break;
+		if (*blen >= args->maxlen)
 			break;
 	}
+	if (pag)
+		xfs_perag_rele(pag);
 
 	xfs_bmap_select_minlen(ap, args, blen, notinit);
 	return 0;
@@ -3245,7 +3241,8 @@  xfs_bmap_btalloc_filestreams(
 	xfs_extlen_t		*blen)
 {
 	struct xfs_mount	*mp = ap->ip->i_mount;
-	xfs_agnumber_t		ag;
+	struct xfs_perag	*pag;
+	xfs_agnumber_t		start_agno;
 	int			notinit = 0;
 	int			error;
 
@@ -3259,33 +3256,50 @@  xfs_bmap_btalloc_filestreams(
 	args->type = XFS_ALLOCTYPE_NEAR_BNO;
 	args->total = ap->total;
 
-	ag = XFS_FSB_TO_AGNO(mp, args->fsbno);
-	if (ag == NULLAGNUMBER)
-		ag = 0;
+	start_agno = XFS_FSB_TO_AGNO(mp, args->fsbno);
+	if (start_agno == NULLAGNUMBER)
+		start_agno = 0;
 
-	error = xfs_bmap_longest_free_extent(args->tp, ag, blen, &notinit);
-	if (error)
-		return error;
+	pag = xfs_perag_grab(mp, start_agno);
+	if (pag) {
+		error = xfs_bmap_longest_free_extent(pag, args->tp, blen,
+				&notinit);
+		xfs_perag_rele(pag);
+		if (error)
+			return error;
+	}
 
 	if (*blen < args->maxlen) {
-		error = xfs_filestream_new_ag(ap, &ag);
+		xfs_agnumber_t	agno = start_agno;
+
+		error = xfs_filestream_new_ag(ap, &agno);
 		if (error)
 			return error;
+		if (agno == NULLAGNUMBER)
+			goto out_select;
 
-		error = xfs_bmap_longest_free_extent(args->tp, ag, blen,
-						     &notinit);
+		pag = xfs_perag_grab(mp, agno);
+		if (!pag)
+			goto out_select;
+
+		error = xfs_bmap_longest_free_extent(pag, args->tp,
+				blen, &notinit);
+		xfs_perag_rele(pag);
 		if (error)
 			return error;
 
+		start_agno = agno;
+
 	}
 
+out_select:
 	xfs_bmap_select_minlen(ap, args, blen, notinit);
 
 	/*
 	 * Set the failure fallback case to look in the selected AG as stream
 	 * may have moved.
 	 */
-	ap->blkno = args->fsbno = XFS_AGB_TO_FSB(mp, ag, 0);
+	ap->blkno = args->fsbno = XFS_AGB_TO_FSB(mp, start_agno, 0);
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 2a323ffa5ba9..50fef3f5af51 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1725,7 +1725,7 @@  xfs_dialloc(
 	bool			ok_alloc = true;
 	bool			low_space = false;
 	int			flags;
-	xfs_ino_t		ino;
+	xfs_ino_t		ino = NULLFSINO;
 
 	/*
 	 * Directories, symlinks, and regular files frequently allocate at least
@@ -1773,39 +1773,37 @@  xfs_dialloc(
 	 * or in which we can allocate some inodes.  Iterate through the
 	 * allocation groups upward, wrapping at the end.
 	 */
-	agno = start_agno;
 	flags = XFS_ALLOC_FLAG_TRYLOCK;
-	for (;;) {
-		pag = xfs_perag_grab(mp, agno);
+retry:
+	for_each_perag_wrap_at(mp, start_agno, mp->m_maxagi, agno, pag) {
 		if (xfs_dialloc_good_ag(pag, *tpp, mode, flags, ok_alloc)) {
 			error = xfs_dialloc_try_ag(pag, tpp, parent,
 					&ino, ok_alloc);
 			if (error != -EAGAIN)
 				break;
+			error = 0;
 		}
 
 		if (xfs_is_shutdown(mp)) {
 			error = -EFSCORRUPTED;
 			break;
 		}
-		if (++agno == mp->m_maxagi)
-			agno = 0;
-		if (agno == start_agno) {
-			if (!flags) {
-				error = -ENOSPC;
-				break;
-			}
+	}
+	if (pag)
+		xfs_perag_rele(pag);
+	if (error)
+		return error;
+	if (ino == NULLFSINO) {
+		if (flags) {
 			flags = 0;
 			if (low_space)
 				ok_alloc = true;
+			goto retry;
 		}
-		xfs_perag_rele(pag);
+		return -ENOSPC;
 	}
-
-	if (!error)
-		*new_ino = ino;
-	xfs_perag_rele(pag);
-	return error;
+	*new_ino = ino;
+	return 0;
 }
 
 /*

[14/42] xfs: introduce xfs_for_each_perag_wrap()

Commit Message

Comments

Patch