From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Carlos Maiolino <cem@kernel.org>,
Brian Foster <bfoster@redhat.com>,
linux-xfs@vger.kernel.org
Subject: Re: [PATCH 07/12] xfs: use WRITE_ONCE to update b_flags
Date: Tue, 28 Jul 2026 08:43:21 -0700 [thread overview]
Message-ID: <20260728154321.GR2901224@frogsfrogsfrogs> (raw)
In-Reply-To: <20260728081152.1778841-8-hch@lst.de>
On Tue, Jul 28, 2026 at 10:11:15AM +0200, Christoph Hellwig wrote:
> Prepare for limited lockless reading of flags by using WRITE_ONCE to
> prevent the compiler from doing non-standard read-modify-write
> operations.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks good to me,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
> ---
> fs/xfs/xfs_buf.c | 86 ++++++++++++++++++++++++++++--------------------
> 1 file changed, 51 insertions(+), 35 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index bc660c8ab9dd..69ace78f9e7f 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -55,11 +55,27 @@ static inline bool xfs_buf_is_uncached(struct xfs_buf *bp)
> return bp->b_rhash_key == XFS_BUF_DADDR_NULL;
> }
>
> +static inline void
> +xfs_buf_set_flags(
> + struct xfs_buf *bp,
> + unsigned int flags)
> +{
> + WRITE_ONCE(bp->b_flags, bp->b_flags | flags);
> +}
> +
> +static inline void
> +xfs_buf_clear_flags(
> + struct xfs_buf *bp,
> + unsigned int flags)
> +{
> + WRITE_ONCE(bp->b_flags, bp->b_flags & ~flags);
> +}
> +
> void
> xfs_buf_set_uptodate(
> struct xfs_buf *bp)
> {
> - bp->b_flags |= XBF_DONE;
> + xfs_buf_set_flags(bp, XBF_DONE);
> }
>
> /*
> @@ -76,14 +92,14 @@ xfs_buf_stale(
> {
> ASSERT(xfs_buf_islocked(bp));
>
> - bp->b_flags |= XBF_STALE;
> + xfs_buf_set_flags(bp, XBF_STALE);
>
> /*
> * Clear the delwri status so that a delwri queue walker will not
> * flush this buffer to disk now that it is stale. The delwri queue has
> * a reference to the buffer, so this is safe to do.
> */
> - bp->b_flags &= ~_XBF_DELWRI_Q;
> + xfs_buf_clear_flags(bp, _XBF_DELWRI_Q);
>
> spin_lock(&bp->b_lockref.lock);
> atomic_set(&bp->b_lru_ref, 0);
> @@ -97,8 +113,7 @@ xfs_buf_clear_stale(
> struct xfs_buf *bp)
> {
> ASSERT(bp->b_flags & XBF_STALE);
> -
> - bp->b_flags &= ~XBF_STALE;
> + xfs_buf_clear_flags(bp, XBF_STALE);
> }
>
> static void
> @@ -175,7 +190,7 @@ xfs_buf_alloc_kmem(
> bp->b_addr = NULL;
> return -ENOMEM;
> }
> - bp->b_flags |= _XBF_KMEM;
> + xfs_buf_set_flags(bp, _XBF_KMEM);
> trace_xfs_buf_backing_kmem(bp, _RET_IP_);
> return 0;
> }
> @@ -320,7 +335,7 @@ xfs_buf_alloc(
> INIT_LIST_HEAD(&bp->b_li_list);
> bp->b_target = target;
> bp->b_mount = target->bt_mount;
> - bp->b_flags = flags;
> + WRITE_ONCE(bp->b_flags, flags);
> bp->b_rhash_key = map[0].bm_bn;
> bp->b_length = 0;
> bp->b_map_count = nmaps;
> @@ -449,7 +464,7 @@ xfs_buf_find_lock(
> return -ENOENT;
> }
> ASSERT((bp->b_flags & _XBF_DELWRI_Q) == 0);
> - bp->b_flags &= _XBF_KMEM;
> + xfs_buf_clear_flags(bp, ~_XBF_KMEM);
> bp->b_ops = NULL;
> }
> return 0;
> @@ -621,8 +636,9 @@ _xfs_buf_read(
> {
> ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
>
> - bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD | XBF_DONE);
> - bp->b_flags |= XBF_READ;
> + xfs_buf_clear_flags(bp, XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD |
> + XBF_DONE);
> + xfs_buf_set_flags(bp, XBF_READ);
> xfs_buf_submit(bp);
> return xfs_buf_iowait(bp);
> }
> @@ -658,7 +674,7 @@ xfs_buf_reverify(
> bp->b_ops = ops;
> bp->b_ops->verify_read(bp);
> if (bp->b_error)
> - bp->b_flags &= ~XBF_DONE;
> + xfs_buf_clear_flags(bp, XBF_DONE);
> return bp->b_error;
> }
>
> @@ -696,7 +712,7 @@ xfs_buf_read_map(
> error = xfs_buf_reverify(bp, ops);
>
> /* We do not want read in the flags */
> - bp->b_flags &= ~XBF_READ;
> + xfs_buf_clear_flags(bp, XBF_READ);
> ASSERT(bp->b_ops != NULL || ops == NULL);
> }
>
> @@ -721,7 +737,7 @@ xfs_buf_read_map(
> if (!xlog_is_shutdown(target->bt_mount->m_log))
> xfs_buf_ioerror_alert(bp, fa);
>
> - bp->b_flags &= ~XBF_DONE;
> + xfs_buf_clear_flags(bp, XBF_DONE);
> xfs_buf_stale(bp);
> xfs_buf_relse(bp);
>
> @@ -767,8 +783,8 @@ xfs_buf_readahead_map(
> }
> XFS_STATS_INC(target->bt_mount, xb_get_read);
> bp->b_ops = ops;
> - bp->b_flags &= ~(XBF_WRITE | XBF_DONE);
> - bp->b_flags |= flags;
> + xfs_buf_clear_flags(bp, XBF_WRITE | XBF_DONE);
> + xfs_buf_set_flags(bp, flags);
> percpu_counter_inc(&target->bt_readahead_count);
> xfs_buf_submit(bp);
> }
> @@ -800,7 +816,7 @@ xfs_buf_read_uncached(
> ASSERT(bp->b_map_count == 1);
> bp->b_rhash_key = XFS_BUF_DADDR_NULL;
> bp->b_maps[0].bm_bn = daddr;
> - bp->b_flags |= XBF_READ;
> + xfs_buf_set_flags(bp, XBF_READ);
> bp->b_ops = ops;
>
> xfs_buf_submit(bp);
> @@ -1094,14 +1110,14 @@ xfs_buf_ioend_handle_error(
>
> resubmit:
> xfs_buf_ioerror(bp, 0);
> - bp->b_flags |= (XBF_DONE | XBF_WRITE_FAIL);
> + xfs_buf_set_flags(bp, XBF_DONE | XBF_WRITE_FAIL);
> reinit_completion(&bp->b_iowait);
> xfs_buf_submit(bp);
> return true;
> out_stale:
> xfs_buf_stale(bp);
> - bp->b_flags |= XBF_DONE;
> - bp->b_flags &= ~XBF_WRITE;
> + xfs_buf_set_flags(bp, XBF_DONE);
> + xfs_buf_clear_flags(bp, XBF_WRITE);
> trace_xfs_buf_error_relse(bp, _RET_IP_);
> return false;
> }
> @@ -1126,7 +1142,7 @@ xfs_buf_ioend(
> if (!bp->b_error && bp->b_ops)
> bp->b_ops->verify_read(bp);
> if (!bp->b_error)
> - bp->b_flags |= XBF_DONE;
> + xfs_buf_set_flags(bp, XBF_DONE);
> if (bp->b_flags & XBF_READ_AHEAD)
> percpu_counter_dec(&bp->b_target->bt_readahead_count);
> } else {
> @@ -1136,8 +1152,8 @@ xfs_buf_ioend(
> return;
> }
> } else {
> - bp->b_flags &= ~XBF_WRITE_FAIL;
> - bp->b_flags |= XBF_DONE;
> + xfs_buf_clear_flags(bp, XBF_WRITE_FAIL);
> + xfs_buf_set_flags(bp, XBF_DONE);
> }
>
> /* clear the retry state */
> @@ -1157,7 +1173,7 @@ xfs_buf_ioend(
> bp->b_iodone(bp);
> }
>
> - bp->b_flags &= ~(XBF_READ | XBF_WRITE | XBF_READ_AHEAD);
> + xfs_buf_clear_flags(bp, XBF_READ | XBF_WRITE | XBF_READ_AHEAD);
> if (async)
> xfs_buf_relse(bp);
> }
> @@ -1203,8 +1219,8 @@ xfs_buf_fail(
> {
> ASSERT(xfs_buf_islocked(bp));
>
> - bp->b_flags |= XBF_ASYNC;
> - bp->b_flags &= ~XBF_DONE;
> + xfs_buf_set_flags(bp, XBF_ASYNC);
> + xfs_buf_clear_flags(bp, XBF_DONE);
> xfs_buf_stale(bp);
> xfs_buf_ioerror(bp, -EIO);
> xfs_buf_ioend(bp);
> @@ -1218,9 +1234,9 @@ xfs_bwrite(
>
> ASSERT(xfs_buf_islocked(bp));
>
> - bp->b_flags |= XBF_WRITE;
> - bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
> - XBF_DONE);
> + xfs_buf_set_flags(bp, XBF_WRITE);
> + xfs_buf_clear_flags(bp, XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
> + XBF_DONE);
>
> xfs_buf_submit(bp);
> error = xfs_buf_iowait(bp);
> @@ -1411,7 +1427,7 @@ xfs_buf_submit(
> return;
>
> ioerror:
> - bp->b_flags &= ~XBF_DONE;
> + xfs_buf_clear_flags(bp, XBF_DONE);
> xfs_buf_stale(bp);
> end_io:
> if (bp->b_flags & XBF_ASYNC)
> @@ -1822,7 +1838,7 @@ xfs_buf_delwri_cancel(
> bp = list_first_entry(list, struct xfs_buf, b_list);
>
> xfs_buf_lock(bp);
> - bp->b_flags &= ~_XBF_DELWRI_Q;
> + xfs_buf_clear_flags(bp, _XBF_DELWRI_Q);
> xfs_buf_list_del(bp);
> xfs_buf_relse(bp);
> }
> @@ -1867,7 +1883,7 @@ xfs_buf_delwri_queue(
> * might get readded to a delwri list after the synchronous writeout, in
> * which case we need just need to re-add the flag here.
> */
> - bp->b_flags |= _XBF_DELWRI_Q;
> + xfs_buf_set_flags(bp, _XBF_DELWRI_Q);
> if (list_empty(&bp->b_list)) {
> xfs_buf_hold(bp);
> list_add_tail(&bp->b_list, list);
> @@ -1944,8 +1960,8 @@ xfs_buf_delwri_submit_prep(
> }
>
> trace_xfs_buf_delwri_split(bp, _RET_IP_);
> - bp->b_flags &= ~_XBF_DELWRI_Q;
> - bp->b_flags |= XBF_WRITE;
> + xfs_buf_clear_flags(bp, _XBF_DELWRI_Q);
> + xfs_buf_set_flags(bp, XBF_WRITE);
> return true;
> }
>
> @@ -1986,7 +2002,7 @@ xfs_buf_delwri_submit_nowait(
> }
> if (!xfs_buf_delwri_submit_prep(bp))
> continue;
> - bp->b_flags |= XBF_ASYNC;
> + xfs_buf_set_flags(bp, XBF_ASYNC);
> xfs_buf_list_del(bp);
> xfs_buf_submit(bp);
> }
> @@ -2019,7 +2035,7 @@ xfs_buf_delwri_submit(
> xfs_buf_lock(bp);
> if (!xfs_buf_delwri_submit_prep(bp))
> continue;
> - bp->b_flags &= ~XBF_ASYNC;
> + xfs_buf_clear_flags(bp, XBF_ASYNC);
> list_move_tail(&bp->b_list, &wait_list);
> xfs_buf_submit(bp);
> }
> --
> 2.53.0
>
>
next prev parent reply other threads:[~2026-07-28 15:43 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-28 8:11 misc buffer cache improvements v2 Christoph Hellwig
2026-07-28 8:11 ` [PATCH 01/12] xfs: don't get a pag reference in xfs_buf_get_map Christoph Hellwig
2026-07-28 8:11 ` [PATCH 02/12] xfs: consolidate buffer locking " Christoph Hellwig
2026-07-28 15:42 ` Darrick J. Wong
2026-07-28 8:11 ` [PATCH 03/12] xfs: split out a lower-level xfs_buf_get_map helper from xfs_find_get_buf Christoph Hellwig
2026-07-28 8:11 ` [PATCH 04/12] xfs: remove spurious XBF_DONE clearing on readahead validation failure Christoph Hellwig
2026-07-28 8:11 ` [PATCH 05/12] xfs: remove _XBF_LOGRECOVERY Christoph Hellwig
2026-07-28 8:11 ` [PATCH 06/12] xfs: hide b_flags manipulation from code outside of xfs_buf.c Christoph Hellwig
2026-07-28 8:11 ` [PATCH 07/12] xfs: use WRITE_ONCE to update b_flags Christoph Hellwig
2026-07-28 15:43 ` Darrick J. Wong [this message]
2026-07-28 8:11 ` [PATCH 08/12] xfs: don't reverify buffers in xfs_buf_readahead_map Christoph Hellwig
2026-07-28 8:11 ` [PATCH 09/12] xfs: use goto based error unwinding in xfs_buf_read_map Christoph Hellwig
2026-07-28 8:11 ` [PATCH 10/12] xfs: merge xfs_buf_reverify into xfs_buf_read_map Christoph Hellwig
2026-07-28 8:11 ` [PATCH 11/12] xfs: move buffer locking out of xfs_find_get_buf Christoph Hellwig
2026-07-28 8:11 ` [PATCH 12/12] xfs: add lockless xfs_buf_readahead_map fast path Christoph Hellwig
-- strict thread matches above, loose matches on Subject: below --
2026-07-15 14:50 misc buffer cache improvements Christoph Hellwig
2026-07-15 14:51 ` [PATCH 07/12] xfs: use WRITE_ONCE to update b_flags Christoph Hellwig
2026-07-24 16:53 ` Darrick J. Wong
2026-07-28 4:40 ` Christoph Hellwig
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=20260728154321.GR2901224@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=bfoster@redhat.com \
--cc=cem@kernel.org \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/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