On Tue, Apr 11, 2023 at 01:20:35PM +1000, Dave Chinner wrote: > > ASSERT(ir.loaded == xfs_iext_count(ifp)); > > + smp_mb(); > > + ifp->if_needextents = 0; > > Hmmm - if this is to ensure that everything above is completed > before the clearing of this flag is visible everywhere else, then we > should be able to use load_acquire/store_release semantics? i.e. the > above is > > smp_store_release(ifp->if_needextents, 0); > > and we use > > smp_load_acquire(ifp->if_needextents) Yeah, that's probably better than my READ_ONCE/WRITE_ONCE suggestions as it also orders vs the previous assignments. > > ifp = xfs_ifork_ptr(ip, whichfork); > > + ifp->if_needextents = 1; > > Hmmm - what's the guarantee that the reader will see ifp->if_format > set correctly if they if_needextents = 1? > > Wouldn't it be better to set this at the same time we set the > ifp->if_format value? We clear it unconditionally above in > xfs_iread_extents(), so why not set it unconditionally there, too, > before we start. i.e. > > /* > * Set the format before we set needsextents with release > * semantics. This ensures that we can use acquire semantics > * on needextents in xfs_need_iread_extents() and be > * guaranteed to see a valid format value after that load. > */ > ifp->if_format = dip->di_format; > smp_store_release(ifp->if_needextents, 1); > > That then means xfs_need_iread_extents() is guaranteed to see a > valid ifp->if_format if ifp->if_needextents is set if we do: I'd just drop the if_format check in xfs_need_iread_extents, which together with the memory barriers should fix all this.
