Fix truncate(2) mtime and ctime handling

[loli10K]

Description

On Linux, ftruncate(2) always changes the file timestamps, even if the
file size is not changed. However, in case of a successfull
truncate(2), the timestamps are updated only if the file size changes.
This translates to the VFS calling the ZFS Posix Layer "setattr"
function (zpl_setattr) with ATTR_MTIME and ATTR_CTIME unconditionally
set on the iattr mask only when doing a ftruncate(2), while the
truncate(2) is left to the filesystem implementation to be dealt with.

This behaviour is consistent with POSIX:2004/SUSv3 specifications
where there's no explicit requirement for file size changes to update
the timestamps only for ftruncate(2):

http://pubs.opengroup.org/onlinepubs/009695399/functions/truncate.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/ftruncate.html

This has been later updated in POSIX:2008/SUSv4 where, for both
truncate(2)/ftruncate(2), there's no mention of this size change
requirement:

http://austingroupbugs.net/view.php?id=489
http://pubs.opengroup.org/onlinepubs/9699919799/functions/truncate.html
http://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html

Unfortunately the Linux VFS is still calling into the ZPL without
ATTR_MTIME/ATTR_CTIME set in the truncate(2) case: we fix this by
explicitly updating the timestamps when detecting the ATTR_SIZE bit,
which is always set in do_truncate(), on the iattr mask.

Motivation and Context

Fix #6811

How Has This Been Tested?

SystemTap and https://github.com/zfsonlinux/fstest.
Will add new code to the ZTS if/when this doesn't break any old test case.

root@linux:~# cd /testpool/ && git clone https://github.com/zfsonlinux/fstest.git --depth 1 && cd fstest/ && make && prove -r tests/truncate/00.t
Cloning into 'fstest'...
remote: Counting objects: 205, done.
remote: Compressing objects: 100% (138/138), done.
remote: Total 205 (delta 69), reused 200 (delta 67), pack-reused 0
Receiving objects: 100% (205/205), 67.68 KiB | 0 bytes/s, done.
Resolving deltas: 100% (69/69), done.
Checking connectivity... done.
gcc -Wall  fstest.c -o fstest
tests/truncate/00.t .. ok     
All tests successful.
Files=1, Tests=21,  3 wallclock secs ( 0.03 usr  0.01 sys +  0.00 cusr  0.14 csys =  0.18 CPU)
Result: PASS
root@linux:/testpool/fstest# 

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)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.
• Change has been approved by a ZFS on Linux member.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@5277f20). Click here to learn what that means.
The diff coverage is 54.05%.

@@            Coverage Diff            @@
##             master    #6819   +/-   ##
=========================================
  Coverage          ?   75.26%           
=========================================
  Files             ?      298           
  Lines             ?    94476           
  Branches          ?        0           
=========================================
  Hits              ?    71109           
  Misses            ?    23367           
  Partials          ?        0

Flag	Coverage Δ
#kernel	74.52% <100%> (?)
#user	67.6% <51.42%> (?)

Impacted Files	Coverage Δ
module/zfs/zfs_vnops.c	62.59% <100%> (ø)
...sts/zfs-tests/tests/functional/truncate/truncate.c	51.42% <51.42%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5277f20...8cf5b75. Read the comment docs.

[dinatale2]

Thanks! Just a couple of comments.

[dinatale2]

Would it be worth moving log_must_eq and log_must_neq to libtest.shlib? They seem generic enough to be placed there and I can think of at least a handful of test cases that could benefit from having such functions.

[behlendorf]

I'd suggest logapi.shlib if you want to stick with the log_must prefix, otherwise math.shlib and an alternate name. math.shlib already contains a vaguely similar helper function called, within_percent.

[dinatale2]

nit: Should make this message consistent with the log_must_eq version. Compared $name should not be equal:...

[loli10K]

That's what i did originally (copy/pasted the message from the other function), but then i noticed it doesn't fit in a single 80-cols line, and i don't like multiline strings.

I'll try changing variable names to fit 80 cols (with the appropriate message) when i refresh this PR.

[behlendorf]

Rather than introduce a new binary how about extending the existing tests/zfs-tests/cmd/file_trunc/file_trunc.c command.

[loli10K]

I thought about it, but:

1. file_trunc opens the target file with O_TRUNC unconditionally, my fear was this would invalidate the test case (i actually just checked, it doesn't).

2. file_trunc unconditionally does a write() and then a truncate(), i would have to add an additional flag to the CLI to avoid that first write() (again, this could invalidate the test case), and optionally update any consumer.

3. do_trunc() takes a int fd param but we need a char *path to test truncate(), so this may require some more code refactoring to work.

I'll see what i can do.

[loli10K]

A few more considerations:

• Most of file_trunc consumers (online_offline_001_pos, online_offline_003_neg, replacement_001_pos, replacement_002_pos and replacement_003_pos) are exploiting the fact file_trunc -c 0 will generate a stream of writes to test zpool operations during IO: they should probably use file_write instead, which currently doesn't support -c 0.

• The only other two consumers (sparse_001_pos and truncate_001_pos) are basically the same test, they both use file_trunc with the same parameters (-r just reads the data just written and verify it) and then use verify_filesys (from libtest.shlib) to "verify" the filesystem; to be honest i don't understand what these tests are really doing.

root@linux:~# sysdig proc.name = 'file_trunc' and evt.type = execve -p%proc.cmdline
file_trunc -f 67108864 -b 512 -c 16384 -r /var/tmp/testdir/testfile.sparse
file_trunc -f 67108864 -b 512 -c 16384 /var/tmp/testdir/testfile.2802

All things considered the ZTS could really use some cleaning up here: once we do that we can probably drop do_write() from file_trunc.c and use it to properly test truncate(2)/ftruncate(2) without the need to introduce a new truncate binary.

Most of the work need is very much out of scope here though, and to avoid issues with OpenZFS patches should be also ported to Illumos.

[behlendorf]

In the interests of getting this fix merged and limiting the scope of the change let's go ahead with the PR pretty much as-is. The one change I'd like to see is to rename this new binary to avoid any confusion with the standard truncate(1) command. Then we can follow up with any cleanup we decide is worthwhile.

[behlendorf]

I'd suggest logapi.shlib if you want to stick with the log_must prefix, otherwise math.shlib and an alternate name. math.shlib already contains a vaguely similar helper function called, within_percent.

[behlendorf]

In the interests of getting this fix merged and limiting the scope of the change let's go ahead with the PR pretty much as-is. The one change I'd like to see is to rename this new binary to avoid any confusion with the standard truncate(1) command. Then we can follow up with any cleanup we decide is worthwhile.