On Mon, 22 Jan 2024 at 23:08, Baokun Li <libaokun1@xxxxxxxxxx> wrote:
>
> >>> include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.
> > 911 | return smp_load_acquire(&inode->i_size);
Ahh, yes. We used to allow READ_ONCE() on non-atomic data structures
(we still do, but we used to too), but with truly atomic cases it's
wrong, since reading a 64-bit value with two 32-bit instructions is
clearly ever atomic.
So your patch is hitting the "these atomic and/or ordered accesses
need to be able to actually be done as *one* access" sanity check.
And the reason is that while we have special-cases 32-bit AMP and
preemption code to avoid this:
> > 891 static inline loff_t i_size_read(const struct inode *inode)
> > 892 {
> > 893 #if BITS_PER_LONG==32 && defined(CONFIG_SMP)
> > 894 loff_t i_size;
> > 895 unsigned int seq;
> > 896
> > 897 do {
> > 898 seq = read_seqcount_begin(&inode->i_size_seqcount);
> > 899 i_size = inode->i_size;
> > 900 } while (read_seqcount_retry(&inode->i_size_seqcount, seq));
> > 901 return i_size;
> > 902 #elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPTION)
> > 903 loff_t i_size;
> > 904
> > 905 preempt_disable();
> > 906 i_size = inode->i_size;
> > 907 preempt_enable();
> > 908 return i_size;
We have *not* special-cased the UP and non-preempt code, because in
that case doing a 64-bit load with two 32-bit accesses is obviously
fine (modulo interrupts, which *really* shouldn't be changing i_size.
So this last case:
> > 909 #else
> > 910 /* Pairs with smp_store_release() in i_size_write() */
> > > 911 return smp_load_acquire(&inode->i_size);
> > 912 #endif
used to be just a simple
return inode->i_size;
but now that you changed it to smp_load_acquire(), our "native size"
sanity checks kick in.
The solution is either to add a new case here (saying "if we're not
SMP, the smp_load_acquire() is pointless"), or to just remove the size
check from the non-SMP version of smp_load_acquire().
Honestly, that sounds like the right thing to do anyway. Our non-SMP
case looks like this:
#ifndef smp_load_acquire
#define smp_load_acquire(p) \
({ \
__unqual_scalar_typeof(*p) ___p1 = READ_ONCE(*p); \
compiletime_assert_atomic_type(*p); \
barrier(); \
(typeof(*p))___p1; \
})
#endif
and that compiletime_assert_atomic_type() might as well go away. Yes,
it removes some type-checking coverage, but honestly, the non-SMP case
simply isn't relevant. No developer uses a UP build for testing
anyway, so the "less coverage" is pretty much completely theoretical.
So I *think* the fix is as trivial as something like this:
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -193,7 +193,6 @@ do { \
#ifndef smp_store_release
#define smp_store_release(p, v) \
do { \
- compiletime_assert_atomic_type(*p); \
barrier(); \
WRITE_ONCE(*p, v); \
} while (0)
@@ -203,7 +202,6 @@ do { \
#define smp_load_acquire(p) \
({ \
__unqual_scalar_typeof(*p) ___p1 = READ_ONCE(*p); \
- compiletime_assert_atomic_type(*p); \
barrier(); \
(typeof(*p))___p1; \
})
because the extra type checking here only makes for extra work, not
for extra safety.
Hmm?
Linus
