zfs_setattr: fix atime update

[robn]

Description

In db4fc55 I messed up and changed this bit of code to set the inode atime to an uninitialised value, when actually it was just supposed to loading the atime from the inode to be stored in the SA. This changes it to what it should have been.

Fixes: #15762

How Has This Been Tested?

By hand, on kernel 5.10.x and 6.4.x. At 00f4096 (ie db4fc559c^), correct behaviour:

$ touch /tank/file
$ stat -c%x /tank/file
2024-01-14 02:17:44.508877399 +0000

At db4fc55, and on master a1771d2 on 5.10.x, 6.4.x and 6.7.x:

$ touch /tank/file
$ stat -c%x /tank/file
1970-01-01 00:00:00.000000000 +0000

With this patch:

$ touch /tank/file
$ stat -c%x /tank/file
2024-01-14 02:51:13.589359644 +0000

[update]: existing ctime/mtime/atime modification test has been updated to check the update makes sense, not just that it was updated.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[snajpa]

Can you, pretty please, add a test (to this PR or later) so this doesn't happen again? I'd love to buy you a beer if you do, btw :D (PM me how can I do that if you're up for it ;))

[robn]

Can you, pretty please, add a test (to this PR or later) so this doesn't happen again?

@snajpa good call! Turns out there was a test checking the ctime/mtime/atime changes worked, but it was only checking that the times changed at all, not that they changed appropriately. So I've updated that to check for the expected value, and sure enough, it fails before this patch, succeeds after. Nice!

I'd love to buy you a beer if you do, btw :D (PM me how can I do that if you're up for it ;))

Well I'm not sure I deserve one after two rounds of stuff ups, but if you insist I won't say no! 😆 There's links in my Github bio, and/or let me know if you're ever in Melbourne. Cheers! 🍻

[snajpa]

So I've updated that to check

Thank you :)

There's links in my Github

Completely forgot GH added that feature, so didn't check before asking :D

also about this:

after two rounds of stuff ups

I think you're doing good work, mishaps happen.

FWIW, I think it perhaps might be a good idea to change stable policy to avoid leaking such problems to the wider userbase - to put a bit of a delay before backporting functionality enhancing patches to the stable; I think new Linux version compat is that, it isn't a fix, not compiling on newer Linux is a missing feature. New features sometimes break stuff :)

[robn]

FWIW, I think it perhaps might be a good idea to change stable policy to avoid leaking such problems to the wider userbase - to put a bit of a delay before backporting functionality enhancing patches to the stable; I think new Linux version compat is that, it isn't a fix, not compiling on newer Linux is a missing feature. New features sometimes break stuff :)

I don't disagree in principle, but I do think there's a bit of a grey area and updates for newer kernels may fall into it. There are competing interests: people running bleeding-edge kernels are also the people more likely to experiment and test and offer feedback, so helping them out might be a bit of a force multiplier for us. There's also optics; keeping up with Linux helps to make sure we don't appear to be stagnating.

And to be fair, this bug also beat two reviewers and the test suite, which is rare.

I guess I'm saying that I can at least make an argument either way. Not that its my decision in the end, but I do support it.

Appreciate the feedback, thank you :)

[behlendorf]

@snajpa thanks for catching this and letting us know. Sorry it slipped by! Some good news here is that neither of the stable releases (2.1.14 or 2.2.2) include these Linux 6.7 compatibility changes yet.