Re: struct vnode::v_mount access rules

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Thu, 3 Oct 2024 10:13:17 +0200
> From: "J. Hannken-Illjes" <hannken%mailbox.org@localhost>
> 
> > On 1. Oct 2024, at 18:57, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
> > 
> > I think the answer is no, nothing here precludes a concurrent
> > vcache_reclaim from writing to fp->f_vnode->v_mount at the same time
> > do_sys_fstatvfs is reading from it.  Although both the before and
> > after states are OK for dostatvfs, if this concurrent read/write is
> > supposed to be allowed, it should be done with atomic_load/store_* --
> > and the same needs to apply to all other use of v_mount that isn't,
> > e.g., serialized by the vnode lock.
> 
> Always thought while writing a pointer to aligned storage a
> concurrent reader will get either the old or the new value but
> never gets garbage.  Is this assumption wrong?

We have long made that assumption, and on all the architectures we
support that I'm aware of, it is true of a load or store instruction
executed by the CPU.

But the compiler might play other shenanigans, if you don't use
atomic_load/store_*.  It might, in principle, fuse or tear loads or
stores (mainly relevant for larger-size or misaligned load/stores).
It might, in practice, issue multiple load or store instructions on
the same location, and assume they produce the same results -- e.g.,
under register pressure, it might just load the same location twice
instead of spilling a register to the stack.

And sanitizers like tsan are apt to interpret this as a data race,
leading to false alarms even if it is otherwise harmless.

And, for the reader's sake in understanding the code, it is clearer to
use atomic_load/store_*.

So we should always use atomic_load/store_* if the intent is that the
location can be concurrently written to while we're reading from it.