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
