Re: [brauner-vfs:vfs.misc 12/13] include/linux/fs.h:911:9: error: call to '__compiletime_assert_207' declared with 'error' attribute: Need native word sized stores/loads for atomicity.

Baokun Li
2024-01-24T07:31:23Z

On 2024/1/24 1:46, Linus Torvalds wrote:
> On Mon, 22 Jan 2024 at 23:08, Baokun Li <libaokun1@huawei.com> 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!