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 <libaokun1@huawei.com>]

On 2024/1/24 0:24, Christian Brauner wrote:
> On Tue, Jan 23, 2024 at 03:07:50PM +0800, Baokun Li wrote:
>> On 2024/1/23 8:12, kernel test robot wrote:
>>> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.misc
>>> head:   297983dc9011461cba6278bfe03f4305c4a2caa0
>>> commit: 4bbd51d0f0ad709c0f02c100439d6c9ba6811d4b [12/13] fs: make the i_size_read/write helpers be smp_load_acquire/store_release()
>>> config: i386-randconfig-015-20240123 (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/config)
>>> compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240123/202401230837.TXro0PHi-lkp@intel.com/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202401230837.TXro0PHi-lkp@intel.com/
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>      In file included from fs/netfs/buffered_read.c:10:
>>>      In file included from fs/netfs/internal.h:9:
>>>      In file included from include/linux/seq_file.h:12:
>>>>> 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);
>>>            |                ^
>>>      include/asm-generic/barrier.h:206:2: note: expanded from macro 'smp_load_acquire'
>>>        206 |         compiletime_assert_atomic_type(*p);                             \
>>>            |         ^
>>>      include/linux/compiler_types.h:438:2: note: expanded from macro 'compiletime_assert_atomic_type'
>>>        438 |         compiletime_assert(__native_word(t),                            \
>>>            |         ^
>>>      include/linux/compiler_types.h:435:2: note: expanded from macro 'compiletime_assert'
>>>        435 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>>            |         ^
>>>      include/linux/compiler_types.h:423:2: note: expanded from macro '_compiletime_assert'
>>>        423 |         __compiletime_assert(condition, msg, prefix, suffix)
>>>            |         ^
>>>      include/linux/compiler_types.h:416:4: note: expanded from macro '__compiletime_assert'
>>>        416 |                         prefix ## suffix();                             \
>>>            |                         ^
>>>      <scratch space>:38:1: note: expanded from here
>>>         38 | __compiletime_assert_207
>>>            | ^
>>>      1 error generated.
>>>
>>>
>>> vim +911 include/linux/fs.h
>>>
>>>      874	
>>>      875	void filemap_invalidate_lock_two(struct address_space *mapping1,
>>>      876					 struct address_space *mapping2);
>>>      877	void filemap_invalidate_unlock_two(struct address_space *mapping1,
>>>      878					   struct address_space *mapping2);
>>>      879	
>>>      880	
>>>      881	/*
>>>      882	 * NOTE: in a 32bit arch with a preemptable kernel and
>>>      883	 * an UP compile the i_size_read/write must be atomic
>>>      884	 * with respect to the local cpu (unlike with preempt disabled),
>>>      885	 * but they don't need to be atomic with respect to other cpus like in
>>>      886	 * true SMP (so they need either to either locally disable irq around
>>>      887	 * the read or for example on x86 they can be still implemented as a
>>>      888	 * cmpxchg8b without the need of the lock prefix). For SMP compiles
>>>      889	 * and 64bit archs it makes no difference if preempt is enabled or not.
>>>      890	 */
>>>      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;
>>>      909	#else
>>>      910		/* Pairs with smp_store_release() in i_size_write() */
>>>    > 911		return smp_load_acquire(&inode->i_size);
>>>      912	#endif
>>>      913	}
>>>      914	
>> Sorry to cause this compilation error, I have not encountered this
>> before when testing compilation with gcc on x86 and arm64.
>> Is there a special config required for this, or is it just clang that
>> triggers this compile error?
>>
>> The compile error seems to be that some architectural implementations
>> of smp_load_acquire() do not support pointers to long long data
>> types. Can't think of a good way to avoid this problem, any ideas
>> from linus and christian?
> That code in i_size_{read,write}() is terrible to look at.
> Pragmatically, we can probably just do READ_ONCE() + smp_rmb() and
> smp_wmb() + WRITE_ONCE(). This guarantees the ordering and it works
> (compiles) on 32bit as well. I think it's still possible that on 32bit
> the READ_ONCE()/WRITE_ONCE() are compiled as two accesses. But I'm not
> sure that this matters because iiuc that problem would've already been
> there with the barrier in mm/filemap.c instead of here.
Thank you very much for your suggestion!

READ_ONCE()/WRITE_ONCE() now allows 64-bit accesses on 32-bit
architectures. Linus suggests smp_load_acquire()/smp_store_release()
to do the same in non-SMP case. I think this is better, what do you think?

Thanks again!
-- 
With Best Regards,
Baokun Li
.