On Wed, 2023-11-08 at 13:29 -0800, Linus Torvalds wrote:
> On Wed, 8 Nov 2023 at 02:19, Chandan Babu R <chandanbabu@xxxxxxxxxx> wrote:
> >
> > I had performed a test merge with latest contents of torvalds/linux.git.
> >
> > This resulted in merge conflicts. The following diff should resolve the merge
> > conflicts.
>
> Well, your merge conflict resolution is the same as my initial
> mindless one, but then when I look closer at it, it turns out that
> it's wrong.
>
> It's wrong not because the merge itself would be wrong, but because
> the conflict made me look at the original, and it turns out that
> commit 75d1e312bbbd ("xfs: convert to new timestamp accessors") was
> buggy.
>
> I'm actually surprised the compilers don't complain about it, because
> the bug means that the new
>
> struct timespec64 ts;
>
> temporary isn't actually initialized for the !XFS_DIFLAG_NEWRTBM case.
>
> The code does
>
> xfs_rtpick_extent(..)
> ...
> struct timespec64 ts;
> ..
> if (!(mp->m_rbmip->i_diflags & XFS_DIFLAG_NEWRTBM)) {
> mp->m_rbmip->i_diflags |= XFS_DIFLAG_NEWRTBM;
> seq = 0;
> } else {
> ...
> ts.tv_sec = (time64_t)seq + 1;
> inode_set_atime_to_ts(VFS_I(mp->m_rbmip), ts);
>
> and notice how 'ts.tv_nsec' is never initialized. So we'll set the
> nsec part of the atime to random garbage.
>
> Oh, I'm sure it doesn't really *matter*, but it's most certainly wrong.
>
> I am not very happy about the whole crazy XFS model where people cast
> the 'struct timespec64' pointer to an 'uint64_t' pointer, and then say
> 'now it's a sequence number'. This is not the only place that
> happened, ie we have similar disgusting code in at least
> xfs_rtfree_extent() too.
>
> That other place in xfs_rtfree_extent() didn't have this bug - it does
> inode_get_atime() unconditionally and this keeps the nsec field as-is,
> but that other place has the same really ugly code.
>
> Doing that "cast struct timespec64 to an uint64_t' is not only ugly
> and wrong, it's _stupid_. The only reason it works in the first place
> is that 'struct timespec64' is
>
> struct timespec64 {
> time64_t tv_sec; /* seconds */
> long tv_nsec; /* nanoseconds */
> };
>
> so the first field is 'tv_sec', which is a 64-bit (signed) value.
>
> So the cast is disgusting - and it's pointless. I don't know why it's
> done that way. It would have been much cleaner to just use tv_sec, and
> have a big comment about it being used as a sequence number here.
>
> I _assume_ there's just a simple 32-bit history to this all, where at
> one point it was a 32-bit tv_sec, and the cast basically used both
> 32-bit fields as a 64-bit sequence number. I get it. But it's most
> definitely wrong now.
>
> End result: I ended up fixing that bug and removing the bogus casts in
> my merge. I *think* I got it right, but apologies in advance if I
> screwed up. I only did visual inspection and build testing, no actual
> real testing.
>
> Also, xfs people may obviously have other preferences for how to deal
> with the whole "now using tv_sec in the VFS inode as a 64-bit sequence
> number" thing, and maybe you prefer to then update my fix to this all.
> But that horrid casts certainly wasn't the right way to do it.
>
> Put another way: please do give my merge a closer look, and decide
> amongst yourself if you then want to deal with this some other way.
>
> Linus
I think when I was looking at that code, I had convinced myself that the
tv_nsec field didn't matter at all, since it wasn't being used, but I
should have done a better job of preserving the existing value. Mea
culpa.
Your fixup looks right to me. Thanks for fixing it.
Cheers,
--
Jeff Layton <jlayton@xxxxxxxxxx>
