On 2024/1/24 1:46, Linus Torvalds wrote:
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
Thank you very much for the detailed explanation! 😊
Now I understand that the cause of this problem is that on 32-bit
architectures the size of the long type and the long long type are
not equal, so this compilation error is triggered when both
CONFIG_SMP and CONFIG_PREEMPTION are not enabled, which
is also why I don't have it triggered on x86 and arm64. I will
include the patch in the next version of the patch set.
Thanks again!
--
With Best Regards,
Baokun Li
.
