On Tue, 2024-07-09 at 17:07 +0200, Arnd Bergmann wrote: > On Tue, Jul 9, 2024, at 16:23, Jeff Layton wrote: > > On Tue, 2024-07-09 at 16:16 +0200, Arnd Bergmann wrote: > > > On Tue, Jul 9, 2024, at 15:45, Geert Uytterhoeven wrote: > > > > I think the simplest solution is to make the floor value I'm > > tracking > > be an atomic64_t. That looks like it should smooth over the > > differences > > between arches. I'm testing a patch to do that now. > > Yes, atomic64_t should work, but be careful about using this > in a fast path since it can turn into a global spinlock > in lib/atomic64.c on architectures that don't support it > natively. > > I'm still reading through the rest of your series, but > it appears that you pass the time value into > ktime_to_timespec64() directly afterwards, so I guess > that is already a fairly large overhead on 32-bit > architectures and an extra spinlock doesn't hurt too > much. > Thanks Arnd. The context for this is generally a write or other change to an inode, so I too am hoping the overhead won't be too bad. It does take great pains to avoid changing the ctime_floor value whenever possible. > Two more things I noticed in your patch: > > - smp_load_acquire() on a 64-bit variable seems problematic > as well, maybe this needs a spinlock on 32-bit > architectures? > That should go away with the conversion of ctime_floor to atomic64_t. AFAICT, arches that don't have native a 64-bit cmpxchg op usually emulate that with hashed spinlocks. > - for the coarse_ctime function, I think you should be > able to avoid the conversion to timespec by just calling > ktime_get_coarse_real_ts64() again instead of converting > monotonic to real and then to timespec. > Note that we might get different values for the coarse timestamps, but if we do then the second fetch will just be a little later (which is OK). I'll plan to make this change. > - inode_set_ctime_current() seems to now store a fine-grained > timespec in the inode even for the !is_mgtime case, skipping > the timestamp_truncate() step. This appears to potentially > leak a non-truncated value to userspace, which would be > inconsistent with the value read back from disk. Oof, you're right. I'll fix that up for the next version. Thanks for the review! -- Jeff Layton <jlayton@xxxxxxxxxx>
