Multigrain timestamps do not work on RISC-V

Multigrain timestamps do not work on RISC-V

[Andreas Schwab]

The statx06 test in the LTP testsuite fails since the multigrain
timestamp feature was merged:

https://openqa.opensuse.org/tests/4800409#step/statx06/7

The issue is that the nsec part of ctime does not change from the time
the file is created:

$ touch xx
$ stat -c $'mtime %y\nctime %z' xx
mtime 2025-01-29 09:43:44.677442605 +0100
ctime 2025-01-29 09:43:44.677442605 +0100
$ touch xx
$ stat -c $'mtime %y\nctime %z' xx
mtime 2025-01-29 09:43:51.641581658 +0100
ctime 2025-01-29 09:43:51.677442605 +0100

My guess would be that something in inode_set_ctime_current is going
wrong.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Multigrain timestamps do not work on RISC-V

[Andreas Schwab]

On Jan 29 2025, Andreas Schwab wrote:

> My guess would be that something in inode_set_ctime_current is going
> wrong.

The bug is in the arch_cmpxchg macros in <asm/cmpxchg.h>, they mishandle
atomic exchange of u32 values:

    # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
    #NO_APP
            ld	a5,-96(s0)		# _33, now.tv_nsec
    # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
            slli	a4,s2,32	#, _49, cns
            srli	a4,a4,32	#, _49, _49
    # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
            beq	a4,a5,.L1248	#, _49, _33,
    .L1205:
            addi	a3,s1,120	#, __ai_ptr, inode
    .L1221:
    # fs/inode.c:2809: 	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now.tv_nsec)) {
    #APP
    # 2809 "fs/inode.c" 1
            0:	lr.w a2, 0(a3)	# __ret, *__ai_ptr_20
            bne  a2, a4, 1f	# __ret, _49

Here the unsigned extended value of cur (a4) is compared with the sign
extended value of inode->i_ctime_nsec (a2).  They cannot match if cur
has I_CTIME_QUERIED set (the sign bit).  The lr/sc loop is exited before
the store conditional is executed.

            sc.w.rl a1, a5, 0(a3)	# __rc, _33, *__ai_ptr_20
            bnez a1, 0b	# __rc
            fence rw,rw
    1:

    # 0 "" 2
    #NO_APP
            sext.w	a5,a2	# __ret, __ret

A redundant sign extension of the current contents of inode->i_ctime_nsec.

    # fs/inode.c:2809: 	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now.tv_nsec)) {
            bne	a5,s2,.L1211	#, __ret, cns,

Here the sign extended value of inode->i_ctime_nsec (a5) is compared
with the sign extended expected value (s2).  They match, and try_cmpxchg
returns true, but the store never happend.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Multigrain timestamps do not work on RISC-V

[Jeff Layton]

On Wed, 2025-01-29 at 18:20 +0100, Andreas Schwab wrote:
> On Jan 29 2025, Andreas Schwab wrote:
> 
> > My guess would be that something in inode_set_ctime_current is going
> > wrong.
> 
> The bug is in the arch_cmpxchg macros in <asm/cmpxchg.h>, they mishandle
> atomic exchange of u32 values:
> 
>     # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
>     #NO_APP
>             ld	a5,-96(s0)		# _33, now.tv_nsec
>     # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
>             slli	a4,s2,32	#, _49, cns
>             srli	a4,a4,32	#, _49, _49
>     # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
>             beq	a4,a5,.L1248	#, _49, _33,
>     .L1205:
>             addi	a3,s1,120	#, __ai_ptr, inode
>     .L1221:
>     # fs/inode.c:2809: 	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now.tv_nsec)) {
>     #APP
>     # 2809 "fs/inode.c" 1
>             0:	lr.w a2, 0(a3)	# __ret, *__ai_ptr_20
>             bne  a2, a4, 1f	# __ret, _49
> 
> Here the unsigned extended value of cur (a4) is compared with the sign
> extended value of inode->i_ctime_nsec (a2).  They cannot match if cur
> has I_CTIME_QUERIED set (the sign bit).  The lr/sc loop is exited before
> the store conditional is executed.
> 
>             sc.w.rl a1, a5, 0(a3)	# __rc, _33, *__ai_ptr_20
>             bnez a1, 0b	# __rc
>             fence rw,rw
>     1:
> 
>     # 0 "" 2
>     #NO_APP
>             sext.w	a5,a2	# __ret, __ret
> 
> A redundant sign extension of the current contents of inode->i_ctime_nsec.
> 
>     # fs/inode.c:2809: 	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now.tv_nsec)) {
>             bne	a5,s2,.L1211	#, __ret, cns,
> 
> Here the sign extended value of inode->i_ctime_nsec (a5) is compared
> with the sign extended expected value (s2).  They match, and try_cmpxchg
> returns true, but the store never happend.
> 

Wow, nice catch! Had me worried there for a bit!

-- 
Jeff Layton <jlayton@kernel.org>

Re: Multigrain timestamps do not work on RISC-V

[Jeff Layton]

On Wed, 2025-01-29 at 18:06 -0500, Jeff Layton wrote:
> On Wed, 2025-01-29 at 18:20 +0100, Andreas Schwab wrote:
> > On Jan 29 2025, Andreas Schwab wrote:
> > 
> > > My guess would be that something in inode_set_ctime_current is going
> > > wrong.
> > 
> > The bug is in the arch_cmpxchg macros in <asm/cmpxchg.h>, they mishandle
> > atomic exchange of u32 values:
> > 
> >     # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
> >     #NO_APP
> >             ld	a5,-96(s0)		# _33, now.tv_nsec
> >     # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
> >             slli	a4,s2,32	#, _49, cns
> >             srli	a4,a4,32	#, _49, _49
> >     # fs/inode.c:2802: 	if (cns == now.tv_nsec && inode->i_ctime_sec == now.tv_sec) {
> >             beq	a4,a5,.L1248	#, _49, _33,
> >     .L1205:
> >             addi	a3,s1,120	#, __ai_ptr, inode
> >     .L1221:
> >     # fs/inode.c:2809: 	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now.tv_nsec)) {
> >     #APP
> >     # 2809 "fs/inode.c" 1
> >             0:	lr.w a2, 0(a3)	# __ret, *__ai_ptr_20
> >             bne  a2, a4, 1f	# __ret, _49
> > 
> > Here the unsigned extended value of cur (a4) is compared with the sign
> > extended value of inode->i_ctime_nsec (a2).  They cannot match if cur
> > has I_CTIME_QUERIED set (the sign bit).  The lr/sc loop is exited before
> > the store conditional is executed.
> > 
> >             sc.w.rl a1, a5, 0(a3)	# __rc, _33, *__ai_ptr_20
> >             bnez a1, 0b	# __rc
> >             fence rw,rw
> >     1:
> > 
> >     # 0 "" 2
> >     #NO_APP
> >             sext.w	a5,a2	# __ret, __ret
> > 
> > A redundant sign extension of the current contents of inode->i_ctime_nsec.
> > 
> >     # fs/inode.c:2809: 	if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now.tv_nsec)) {
> >             bne	a5,s2,.L1211	#, __ret, cns,
> > 
> > Here the sign extended value of inode->i_ctime_nsec (a5) is compared
> > with the sign extended expected value (s2).  They match, and try_cmpxchg
> > returns true, but the store never happend.
> > 
> 
> Wow, nice catch! Had me worried there for a bit!
> 

For the record: 

Bit 30 is also unused. When I wrote these patches, I considered using
that instead of the sign bit since I wasn't sure whether I might run
into problems using the sign bit.

Obviously, fixing the macro seems like the better solution, but if
fixing this efficiently is a problem, then moving I_CTIME_QUERIED to
bit 30 is also an option.

Cheers,
-- 
Jeff Layton <jlayton@kernel.org>

Re: Multigrain timestamps do not work on RISC-V

[Andreas Schwab]

On Jan 29 2025, Jeff Layton wrote:

> Obviously, fixing the macro seems like the better solution, but if
> fixing this efficiently is a problem, then moving I_CTIME_QUERIED to
> bit 30 is also an option.

There is no need to change it, the fix for arch_cmpxchg is pretty
straight forward, see
<https://lore.kernel.org/all/mvmed0k4prh.fsf@suse.de/>.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Multigrain timestamps do not work on RISC-V

[Jeff Layton]

On Wed, 2025-01-29 at 11:07 +0100, Andreas Schwab wrote:
> The statx06 test in the LTP testsuite fails since the multigrain
> timestamp feature was merged:
> 
> https://openqa.opensuse.org/tests/4800409#step/statx06/7
> 
> The issue is that the nsec part of ctime does not change from the time
> the file is created:
> 
> $ touch xx
> $ stat -c $'mtime %y\nctime %z' xx
> mtime 2025-01-29 09:43:44.677442605 +0100
> ctime 2025-01-29 09:43:44.677442605 +0100
> $ touch xx
> $ stat -c $'mtime %y\nctime %z' xx
> mtime 2025-01-29 09:43:51.641581658 +0100
> ctime 2025-01-29 09:43:51.677442605 +0100
>
> My guess would be that something in inode_set_ctime_current is going
> wrong.
> 

Thanks for the bug report, Andreas.

I assume you're seeing this across different filesystems (i.e. tmpfs,
ext4, etc.)? It almost looks like this try_cmpxchg() is returning true
without actually doing the swap:

        if (try_cmpxchg(&inode->i_ctime_nsec, &cur, now.tv_nsec)) {
                /* If swap occurred, then we're (mostly) done */
                inode->i_ctime_sec = now.tv_sec;
                trace_ctime_ns_xchg(inode, cns, now.tv_nsec, cur);
                mgtime_counter_inc(mg_ctime_swaps);
        } else {

It might also be interesting to see the output of that tracepoint over
this test, if you're able.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>