[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <edd2d831320fb14333e605e77d4b284b1123eb86.camel@kernel.org>
Date: Tue, 09 Jul 2024 11:27:40 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Arnd Bergmann <arnd@...db.de>, Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: linux-m68k@...ts.linux-m68k.org, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org
Subject: Re: [jlayton:mgtime 5/13] inode.c:undefined reference to
`__invalid_cmpxchg_size'
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@...nel.org>
Powered by blists - more mailing lists