Re: [PATCH 02/11] xfs: start creating infrastructure for health monitoring

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Feb 06, 2026 at 07:54:51PM +0100, Pankaj Raghav wrote:
> 
> 
> On 2/6/26 18:47, Darrick J. Wong wrote:
> > On Fri, Feb 06, 2026 at 02:07:56PM +0100, Pankaj Raghav (Samsung) wrote:
> >>> +static DEFINE_SPINLOCK(xfs_healthmon_lock);
> >>> +
> >>> +/* Grab a reference to the healthmon object for a given mount, if any. */
> >>> +static struct xfs_healthmon *
> >>> +xfs_healthmon_get(
> >>> +	struct xfs_mount		*mp)
> >>> +{
> >>> +	struct xfs_healthmon		*hm;
> >>> +
> >>> +	rcu_read_lock();
> >>> +	hm = mp->m_healthmon;
> >>
> >> Nit: Should we do a READ_ONCE(mp->m_healthmon) here to avoid any
> >> compiler tricks that can result in an undefined behaviour? I am not sure
> >> if I am being paranoid here.
> > 
> > Compiler tricks?  We've taken the rcu read lock, which adds an
> > optimization barrier so that the mp->m_healthmon access can't be
> > reordered before the rcu_read_lock.  I'm not sure if that answers your
> > question.
> > 
> 
> This answers. So this is my understanding: RCU guarantees that we get
> a valid object (actual data of m_healthmon) but does not guarantee the
> compiler will not reread the pointer between checking if hm is !NULL
> and accessing the pointer as we are doing it lockless.

Oh, now I see what you're concerned about.  You're worried that the
compiler could turn this:

	if (hm && !refcount_inc_not_zero(&hm->ref))

into this:

	if (mp->m_healthmon && !refcount_inc_not_zero(&mp->m_healthmon->ref))

which then gives xfs_healthmon_detach the opening it needs to slip in
between the two dereferences of mp and turn m_healthmon into NULL,
leading the "mp->m_healthmon->ref" expression to become a NULL pointer
dereference.

> So just a barrier() call in rcu_read_lock is enough to make sure this
> doesn't happen and probably adding a READ_ONCE() is not needed?

Nope.  You're right, we do need READ_ONCE here.

--D