Re: [GIT PULL] xfs: new code for 6.7

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2023-11-08 at 13:29 -0800, Linus Torvalds wrote:
> On Wed, 8 Nov 2023 at 02:19, Chandan Babu R <chandanbabu@kernel.org> 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@kernel.org>