From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, chandanrlinux@gmail.com,
wen.gang.wang@oracle.com
Subject: Re: [PATCH 1/3] xfs: pass alloc flags through to xfs_extent_busy_flush()
Date: Wed, 14 Jun 2023 20:32:35 -0700 [thread overview]
Message-ID: <20230615033235.GL11441@frogsfrogsfrogs> (raw)
In-Reply-To: <20230615014201.3171380-2-david@fromorbit.com>
On Thu, Jun 15, 2023 at 11:41:59AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> To avoid blocking in xfs_extent_busy_flush() when freeing extents
> and the only busy extents are held by the current transaction, we
> need to pass the XFS_ALLOC_FLAG_FREEING flag context all the way
> into xfs_extent_busy_flush().
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/libxfs/xfs_alloc.c | 96 +++++++++++++++++++++------------------
> fs/xfs/libxfs/xfs_alloc.h | 2 +-
> fs/xfs/xfs_extent_busy.c | 3 +-
> fs/xfs/xfs_extent_busy.h | 2 +-
> 4 files changed, 56 insertions(+), 47 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index c20fe99405d8..11bd0a1756a1 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -1536,7 +1536,8 @@ xfs_alloc_ag_vextent_lastblock(
> */
> STATIC int
> xfs_alloc_ag_vextent_near(
> - struct xfs_alloc_arg *args)
> + struct xfs_alloc_arg *args,
> + uint32_t alloc_flags)
Is some bot going to complain about the uint32_t here vs the unsigned
int in xfs_extent_busy_flush?
Just to check my grokking here -- it's the ALLOC_FLAG_FREEING from
xfs_free_extent_fix_freelist that we need to pass all the way to the
bottom of the allocator?
If the answers are 'no' and 'yes', then
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> {
> struct xfs_alloc_cur acur = {};
> int error; /* error code */
> @@ -1612,7 +1613,7 @@ xfs_alloc_ag_vextent_near(
> if (acur.busy) {
> trace_xfs_alloc_near_busy(args);
> xfs_extent_busy_flush(args->mp, args->pag,
> - acur.busy_gen);
> + acur.busy_gen, alloc_flags);
> goto restart;
> }
> trace_xfs_alloc_size_neither(args);
> @@ -1635,21 +1636,22 @@ xfs_alloc_ag_vextent_near(
> * and of the form k * prod + mod unless there's nothing that large.
> * Return the starting a.g. block, or NULLAGBLOCK if we can't do it.
> */
> -STATIC int /* error */
> +static int
> xfs_alloc_ag_vextent_size(
> - xfs_alloc_arg_t *args) /* allocation argument structure */
> + struct xfs_alloc_arg *args,
> + uint32_t alloc_flags)
> {
> - struct xfs_agf *agf = args->agbp->b_addr;
> - struct xfs_btree_cur *bno_cur; /* cursor for bno btree */
> - struct xfs_btree_cur *cnt_cur; /* cursor for cnt btree */
> - int error; /* error result */
> - xfs_agblock_t fbno; /* start of found freespace */
> - xfs_extlen_t flen; /* length of found freespace */
> - int i; /* temp status variable */
> - xfs_agblock_t rbno; /* returned block number */
> - xfs_extlen_t rlen; /* length of returned extent */
> - bool busy;
> - unsigned busy_gen;
> + struct xfs_agf *agf = args->agbp->b_addr;
> + struct xfs_btree_cur *bno_cur;
> + struct xfs_btree_cur *cnt_cur;
> + xfs_agblock_t fbno; /* start of found freespace */
> + xfs_extlen_t flen; /* length of found freespace */
> + xfs_agblock_t rbno; /* returned block number */
> + xfs_extlen_t rlen; /* length of returned extent */
> + bool busy;
> + unsigned busy_gen;
> + int error;
> + int i;
>
> restart:
> /*
> @@ -1717,8 +1719,8 @@ xfs_alloc_ag_vextent_size(
> xfs_btree_del_cursor(cnt_cur,
> XFS_BTREE_NOERROR);
> trace_xfs_alloc_size_busy(args);
> - xfs_extent_busy_flush(args->mp,
> - args->pag, busy_gen);
> + xfs_extent_busy_flush(args->mp, args->pag,
> + busy_gen, alloc_flags);
> goto restart;
> }
> }
> @@ -1802,7 +1804,8 @@ xfs_alloc_ag_vextent_size(
> if (busy) {
> xfs_btree_del_cursor(cnt_cur, XFS_BTREE_NOERROR);
> trace_xfs_alloc_size_busy(args);
> - xfs_extent_busy_flush(args->mp, args->pag, busy_gen);
> + xfs_extent_busy_flush(args->mp, args->pag, busy_gen,
> + alloc_flags);
> goto restart;
> }
> goto out_nominleft;
> @@ -2568,7 +2571,7 @@ xfs_exact_minlen_extent_available(
> int /* error */
> xfs_alloc_fix_freelist(
> struct xfs_alloc_arg *args, /* allocation argument structure */
> - int flags) /* XFS_ALLOC_FLAG_... */
> + uint32_t alloc_flags)
> {
> struct xfs_mount *mp = args->mp;
> struct xfs_perag *pag = args->pag;
> @@ -2584,7 +2587,7 @@ xfs_alloc_fix_freelist(
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
>
> if (!xfs_perag_initialised_agf(pag)) {
> - error = xfs_alloc_read_agf(pag, tp, flags, &agbp);
> + error = xfs_alloc_read_agf(pag, tp, alloc_flags, &agbp);
> if (error) {
> /* Couldn't lock the AGF so skip this AG. */
> if (error == -EAGAIN)
> @@ -2600,13 +2603,13 @@ xfs_alloc_fix_freelist(
> */
> if (xfs_perag_prefers_metadata(pag) &&
> (args->datatype & XFS_ALLOC_USERDATA) &&
> - (flags & XFS_ALLOC_FLAG_TRYLOCK)) {
> - ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> + (alloc_flags & XFS_ALLOC_FLAG_TRYLOCK)) {
> + ASSERT(!(alloc_flags & XFS_ALLOC_FLAG_FREEING));
> goto out_agbp_relse;
> }
>
> need = xfs_alloc_min_freelist(mp, pag);
> - if (!xfs_alloc_space_available(args, need, flags |
> + if (!xfs_alloc_space_available(args, need, alloc_flags |
> XFS_ALLOC_FLAG_CHECK))
> goto out_agbp_relse;
>
> @@ -2615,7 +2618,7 @@ xfs_alloc_fix_freelist(
> * Can fail if we're not blocking on locks, and it's held.
> */
> if (!agbp) {
> - error = xfs_alloc_read_agf(pag, tp, flags, &agbp);
> + error = xfs_alloc_read_agf(pag, tp, alloc_flags, &agbp);
> if (error) {
> /* Couldn't lock the AGF so skip this AG. */
> if (error == -EAGAIN)
> @@ -2630,7 +2633,7 @@ xfs_alloc_fix_freelist(
>
> /* If there isn't enough total space or single-extent, reject it. */
> need = xfs_alloc_min_freelist(mp, pag);
> - if (!xfs_alloc_space_available(args, need, flags))
> + if (!xfs_alloc_space_available(args, need, alloc_flags))
> goto out_agbp_relse;
>
> #ifdef DEBUG
> @@ -2668,11 +2671,12 @@ xfs_alloc_fix_freelist(
> */
> memset(&targs, 0, sizeof(targs));
> /* struct copy below */
> - if (flags & XFS_ALLOC_FLAG_NORMAP)
> + if (alloc_flags & XFS_ALLOC_FLAG_NORMAP)
> targs.oinfo = XFS_RMAP_OINFO_SKIP_UPDATE;
> else
> targs.oinfo = XFS_RMAP_OINFO_AG;
> - while (!(flags & XFS_ALLOC_FLAG_NOSHRINK) && pag->pagf_flcount > need) {
> + while (!(alloc_flags & XFS_ALLOC_FLAG_NOSHRINK) &&
> + pag->pagf_flcount > need) {
> error = xfs_alloc_get_freelist(pag, tp, agbp, &bno, 0);
> if (error)
> goto out_agbp_relse;
> @@ -2700,7 +2704,7 @@ xfs_alloc_fix_freelist(
> targs.resv = XFS_AG_RESV_AGFL;
>
> /* Allocate as many blocks as possible at once. */
> - error = xfs_alloc_ag_vextent_size(&targs);
> + error = xfs_alloc_ag_vextent_size(&targs, alloc_flags);
> if (error)
> goto out_agflbp_relse;
>
> @@ -2710,7 +2714,7 @@ xfs_alloc_fix_freelist(
> * on a completely full ag.
> */
> if (targs.agbno == NULLAGBLOCK) {
> - if (flags & XFS_ALLOC_FLAG_FREEING)
> + if (alloc_flags & XFS_ALLOC_FLAG_FREEING)
> break;
> goto out_agflbp_relse;
> }
> @@ -3226,7 +3230,7 @@ xfs_alloc_vextent_check_args(
> static int
> xfs_alloc_vextent_prepare_ag(
> struct xfs_alloc_arg *args,
> - uint32_t flags)
> + uint32_t alloc_flags)
> {
> bool need_pag = !args->pag;
> int error;
> @@ -3235,7 +3239,7 @@ xfs_alloc_vextent_prepare_ag(
> args->pag = xfs_perag_get(args->mp, args->agno);
>
> args->agbp = NULL;
> - error = xfs_alloc_fix_freelist(args, flags);
> + error = xfs_alloc_fix_freelist(args, alloc_flags);
> if (error) {
> trace_xfs_alloc_vextent_nofix(args);
> if (need_pag)
> @@ -3357,6 +3361,7 @@ xfs_alloc_vextent_this_ag(
> {
> struct xfs_mount *mp = args->mp;
> xfs_agnumber_t minimum_agno;
> + uint32_t alloc_flags = 0;
> int error;
>
> ASSERT(args->pag != NULL);
> @@ -3375,9 +3380,9 @@ xfs_alloc_vextent_this_ag(
> return error;
> }
>
> - error = xfs_alloc_vextent_prepare_ag(args, 0);
> + error = xfs_alloc_vextent_prepare_ag(args, alloc_flags);
> if (!error && args->agbp)
> - error = xfs_alloc_ag_vextent_size(args);
> + error = xfs_alloc_ag_vextent_size(args, alloc_flags);
>
> return xfs_alloc_vextent_finish(args, minimum_agno, error, false);
> }
> @@ -3406,20 +3411,20 @@ xfs_alloc_vextent_iterate_ags(
> xfs_agnumber_t minimum_agno,
> xfs_agnumber_t start_agno,
> xfs_agblock_t target_agbno,
> - uint32_t flags)
> + uint32_t alloc_flags)
> {
> struct xfs_mount *mp = args->mp;
> xfs_agnumber_t restart_agno = minimum_agno;
> xfs_agnumber_t agno;
> int error = 0;
>
> - if (flags & XFS_ALLOC_FLAG_TRYLOCK)
> + if (alloc_flags & XFS_ALLOC_FLAG_TRYLOCK)
> restart_agno = 0;
> restart:
> for_each_perag_wrap_range(mp, start_agno, restart_agno,
> mp->m_sb.sb_agcount, agno, args->pag) {
> args->agno = agno;
> - error = xfs_alloc_vextent_prepare_ag(args, flags);
> + error = xfs_alloc_vextent_prepare_ag(args, alloc_flags);
> if (error)
> break;
> if (!args->agbp) {
> @@ -3433,10 +3438,10 @@ xfs_alloc_vextent_iterate_ags(
> */
> if (args->agno == start_agno && target_agbno) {
> args->agbno = target_agbno;
> - error = xfs_alloc_ag_vextent_near(args);
> + error = xfs_alloc_ag_vextent_near(args, alloc_flags);
> } else {
> args->agbno = 0;
> - error = xfs_alloc_ag_vextent_size(args);
> + error = xfs_alloc_ag_vextent_size(args, alloc_flags);
> }
> break;
> }
> @@ -3453,8 +3458,8 @@ xfs_alloc_vextent_iterate_ags(
> * constraining flags by the caller, drop them and retry the allocation
> * without any constraints being set.
> */
> - if (flags) {
> - flags = 0;
> + if (alloc_flags & XFS_ALLOC_FLAG_TRYLOCK) {
> + alloc_flags &= ~XFS_ALLOC_FLAG_TRYLOCK;
> restart_agno = minimum_agno;
> goto restart;
> }
> @@ -3482,6 +3487,7 @@ xfs_alloc_vextent_start_ag(
> xfs_agnumber_t start_agno;
> xfs_agnumber_t rotorstep = xfs_rotorstep;
> bool bump_rotor = false;
> + uint32_t alloc_flags = XFS_ALLOC_FLAG_TRYLOCK;
> int error;
>
> ASSERT(args->pag == NULL);
> @@ -3508,7 +3514,7 @@ xfs_alloc_vextent_start_ag(
>
> start_agno = max(minimum_agno, XFS_FSB_TO_AGNO(mp, target));
> error = xfs_alloc_vextent_iterate_ags(args, minimum_agno, start_agno,
> - XFS_FSB_TO_AGBNO(mp, target), XFS_ALLOC_FLAG_TRYLOCK);
> + XFS_FSB_TO_AGBNO(mp, target), alloc_flags);
>
> if (bump_rotor) {
> if (args->agno == start_agno)
> @@ -3535,6 +3541,7 @@ xfs_alloc_vextent_first_ag(
> struct xfs_mount *mp = args->mp;
> xfs_agnumber_t minimum_agno;
> xfs_agnumber_t start_agno;
> + uint32_t alloc_flags = XFS_ALLOC_FLAG_TRYLOCK;
> int error;
>
> ASSERT(args->pag == NULL);
> @@ -3553,7 +3560,7 @@ xfs_alloc_vextent_first_ag(
>
> start_agno = max(minimum_agno, XFS_FSB_TO_AGNO(mp, target));
> error = xfs_alloc_vextent_iterate_ags(args, minimum_agno, start_agno,
> - XFS_FSB_TO_AGBNO(mp, target), 0);
> + XFS_FSB_TO_AGBNO(mp, target), alloc_flags);
> return xfs_alloc_vextent_finish(args, minimum_agno, error, true);
> }
>
> @@ -3606,6 +3613,7 @@ xfs_alloc_vextent_near_bno(
> struct xfs_mount *mp = args->mp;
> xfs_agnumber_t minimum_agno;
> bool needs_perag = args->pag == NULL;
> + uint32_t alloc_flags = 0;
> int error;
>
> if (!needs_perag)
> @@ -3626,9 +3634,9 @@ xfs_alloc_vextent_near_bno(
> if (needs_perag)
> args->pag = xfs_perag_grab(mp, args->agno);
>
> - error = xfs_alloc_vextent_prepare_ag(args, 0);
> + error = xfs_alloc_vextent_prepare_ag(args, alloc_flags);
> if (!error && args->agbp)
> - error = xfs_alloc_ag_vextent_near(args);
> + error = xfs_alloc_ag_vextent_near(args, alloc_flags);
>
> return xfs_alloc_vextent_finish(args, minimum_agno, error, needs_perag);
> }
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index 85ac470be0da..d1aa7c63eafe 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -195,7 +195,7 @@ int xfs_alloc_read_agfl(struct xfs_perag *pag, struct xfs_trans *tp,
> struct xfs_buf **bpp);
> int xfs_free_agfl_block(struct xfs_trans *, xfs_agnumber_t, xfs_agblock_t,
> struct xfs_buf *, struct xfs_owner_info *);
> -int xfs_alloc_fix_freelist(struct xfs_alloc_arg *args, int flags);
> +int xfs_alloc_fix_freelist(struct xfs_alloc_arg *args, uint32_t alloc_flags);
> int xfs_free_extent_fix_freelist(struct xfs_trans *tp, struct xfs_perag *pag,
> struct xfs_buf **agbp);
>
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index f3d328e4a440..874798949625 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -571,7 +571,8 @@ void
> xfs_extent_busy_flush(
> struct xfs_mount *mp,
> struct xfs_perag *pag,
> - unsigned busy_gen)
> + unsigned busy_gen,
> + unsigned int flags)
> {
> DEFINE_WAIT (wait);
> int error;
> diff --git a/fs/xfs/xfs_extent_busy.h b/fs/xfs/xfs_extent_busy.h
> index 4a118131059f..79ef5a2d7758 100644
> --- a/fs/xfs/xfs_extent_busy.h
> +++ b/fs/xfs/xfs_extent_busy.h
> @@ -53,7 +53,7 @@ xfs_extent_busy_trim(struct xfs_alloc_arg *args, xfs_agblock_t *bno,
>
> void
> xfs_extent_busy_flush(struct xfs_mount *mp, struct xfs_perag *pag,
> - unsigned busy_gen);
> + unsigned busy_gen, unsigned int flags);
>
> void
> xfs_extent_busy_wait_all(struct xfs_mount *mp);
> --
> 2.40.1
>
next prev parent reply other threads:[~2023-06-15 3:32 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-15 1:41 [PATCH 0/3] xfs: fix xfs_extent_busy_flush() deadlock in EFI processing Dave Chinner
2023-06-15 1:41 ` [PATCH 1/3] xfs: pass alloc flags through to xfs_extent_busy_flush() Dave Chinner
2023-06-15 3:32 ` Darrick J. Wong [this message]
2023-06-15 3:48 ` Dave Chinner
2023-06-15 21:57 ` Wengang Wang
2023-06-15 22:14 ` Dave Chinner
2023-06-15 22:31 ` Wengang Wang
2023-06-15 23:09 ` Wengang Wang
2023-06-15 23:33 ` Dave Chinner
2023-06-15 23:51 ` Wengang Wang
2023-06-16 0:17 ` Dave Chinner
2023-06-16 0:42 ` Wengang Wang
2023-06-16 4:27 ` Wengang Wang
2023-06-16 5:04 ` Wengang Wang
2023-06-16 7:36 ` Dave Chinner
2023-06-16 17:43 ` Wengang Wang
2023-06-16 22:29 ` Dave Chinner
2023-06-16 22:53 ` Wengang Wang
2023-06-16 23:14 ` Wengang Wang
2023-06-17 0:47 ` Dave Chinner
2023-06-20 16:56 ` Wengang Wang
2023-06-22 1:15 ` Wengang Wang
2023-06-15 1:42 ` [PATCH 2/3] xfs: allow extent free intents to be retried Dave Chinner
2023-06-15 3:38 ` Darrick J. Wong
2023-06-15 3:57 ` Dave Chinner
2023-06-15 14:41 ` Darrick J. Wong
2023-06-15 22:21 ` Dave Chinner
2023-06-15 1:42 ` [PATCH 3/3] xfs: don't block in busy flushing when freeing extents Dave Chinner
2023-06-15 3:40 ` Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230615033235.GL11441@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=chandanrlinux@gmail.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
--cc=wen.gang.wang@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox