copy_file_range() doesn't update the atime of an empty file

Discussion:

(too old to reply)

Mark Johnston

2023-10-04 15:31:29 UTC

For a while, Jenkins has been complaining that one of the tmpfs tests is
failing:
https://ci.freebsd.org/job/FreeBSD-main-amd64-test/23814/testReport/junit/sys.fs.tmpfs/times_test/empty/

This has been happening since commit
8113cc827611a88540736c92ced7d3a7020a1723, which converted cat(1) to use
copy_file_range(2). The test in question creates an empty file, waits
for a second, then cat(1)s it and checks that the file's atime was
updated. After the aforementioned commit, the atime is not updated.

I believe the essential difference is that a zero-length read(2) results
in a call to VOP_READ(), which results in an updated atime even if no
bytes were read. For instance, ffs_read() sets IN_ACCESS so long as the
routine doesn't return an error. (I'm not sure if the mtime is
correspondingly updated upon a zero-length write.)

copy_file_range() on the other hand elides calls to VOP_READ/VOP_WRITE
when copylen is 0, so the atime doesn't get updated. I wonder if we
could at least change it to call VOP_READ in that scenario, as in the
untested patch below. Any thoughts?

diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
index 4e4161ef1a7f..d60608a6d3b9 100644
--- a/sys/kern/vfs_vnops.c
+++ b/sys/kern/vfs_vnops.c
@@ -3499,7 +3499,7 @@ vn_generic_copy_file_range(struct vnode *invp, off_t *inoffp,
xfer -= (*inoffp % blksize);
}
/* Loop copying the data block. */
- while (copylen > 0 && error == 0 && !eof && interrupted == 0) {
+ while (error == 0 && !eof && interrupted == 0) {
if (copylen < xfer)
xfer = copylen;
error = vn_lock(invp, LK_SHARED);
@@ -3511,7 +3511,7 @@ vn_generic_copy_file_range(struct vnode *invp, off_t *inoffp,
curthread);
VOP_UNLOCK(invp);
lastblock = false;
- if (error == 0 && aresid > 0) {
+ if (error == 0 && (xfer == 0 || aresid > 0)) {
/* Stop the copy at EOF on the input file. */
xfer -= aresid;
eof = true;

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

Alan Somers

2023-10-04 15:40:10 UTC

Permalink

Post by Mark Johnston
For a while, Jenkins has been complaining that one of the tmpfs tests is
https://ci.freebsd.org/job/FreeBSD-main-amd64-test/23814/testReport/junit/sys.fs.tmpfs/times_test/empty/
This has been happening since commit
8113cc827611a88540736c92ced7d3a7020a1723, which converted cat(1) to use
copy_file_range(2). The test in question creates an empty file, waits
for a second, then cat(1)s it and checks that the file's atime was
updated. After the aforementioned commit, the atime is not updated.
I believe the essential difference is that a zero-length read(2) results
in a call to VOP_READ(), which results in an updated atime even if no
bytes were read. For instance, ffs_read() sets IN_ACCESS so long as the
routine doesn't return an error. (I'm not sure if the mtime is
correspondingly updated upon a zero-length write.)
copy_file_range() on the other hand elides calls to VOP_READ/VOP_WRITE
when copylen is 0, so the atime doesn't get updated. I wonder if we
could at least change it to call VOP_READ in that scenario, as in the
untested patch below. Any thoughts?
diff --git a/sys/kern/vfs_vnops.c b/sys/kern/vfs_vnops.c
index 4e4161ef1a7f..d60608a6d3b9 100644
--- a/sys/kern/vfs_vnops.c
+++ b/sys/kern/vfs_vnops.c
@@ -3499,7 +3499,7 @@ vn_generic_copy_file_range(struct vnode *invp, off_t *inoffp,
xfer -= (*inoffp % blksize);
}
/* Loop copying the data block. */
- while (copylen > 0 && error == 0 && !eof && interrupted == 0) {
+ while (error == 0 && !eof && interrupted == 0) {
if (copylen < xfer)
xfer = copylen;
error = vn_lock(invp, LK_SHARED);
@@ -3511,7 +3511,7 @@ vn_generic_copy_file_range(struct vnode *invp, off_t *inoffp,
curthread);
VOP_UNLOCK(invp);
lastblock = false;
- if (error == 0 && aresid > 0) {
+ if (error == 0 && (xfer == 0 || aresid > 0)) {
/* Stop the copy at EOF on the input file. */
xfer -= aresid;
eof = true;

From POSIX: "Note that a read() of zero bytes does not modify the last
data access timestamp. A read() that requests more than zero bytes,
but returns zero, is required to modify the last data access
timestamp."

While copy_file_range is not standardized, it ought to comport to
POSIX as closely as possible. I think we should change it as you
suggest.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

Rick Macklem

2023-10-04 23:39:22 UTC

Permalink

Post by Alan Somers

From POSIX: "Note that a read() of zero bytes does not modify the last
data access timestamp. A read() that requests more than zero bytes,
but returns zero, is required to modify the last data access
timestamp."
While copy_file_range is not standardized, it ought to comport to
POSIX as closely as possible. I think we should change it as you
suggest.

Well, I'd like to maintain the syscall as "Linux compatible", which was
my original intent. (I consider Linux as the defacto standard for *nix* like
operating systems).
I've been ignoring a recent request for support for non-regular files for
this reason. (I eventually intend to patch the man page to clarify that
it only works for regular files, which is what Linux does.)
As such, the first step is to figure out if Linux updates atime when a
copy_file_range() returns 0 bytes. I just did a test on Linux (kernel
version 6.3)
using a ext4 fs mounted "relatime" and doing a copy_file_range(2) on it
(using a trivial file copy program suing copy_file_range(2)) did not update
atime. (I did modify the file via "cat /dev/null > file" so that the atime would
be updated for "relatime". A similar test using "cp" did update the atime.)
Also, the above changes the "generic" copy loop, but changes will
also be required (or at least tested) for ZFS when block cloning is
enabled and NFSv4.2. The NFSv4.2 RFC does not specify whether
or not a "Copy" operation that returns 0 bytes updates atime
(called TimeAccess in NFSv4.2).
Oh, and the NFS protocol (up to and including NFSv4.2) cannot
provide a POSIX compliant file system (the NFS client tries to make
it look close to POSIX compliant). As such, expecting a copy_file_range(2)
over NFSv4.2 to behave in a POSIX-like way may not make sense?
Personally, I'd rather see copy_file_range(2) remain Linux compatible.
Does cat(1) really need to exhibit this behaviour or is it just read(2)
that specifies this?
rick

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

Alan Somers

2023-10-05 15:41:16 UTC

Permalink

I don't think that Linux is a good model to copy from, where atime is
concerned. It long ago gave up on POSIX-compliance for atime by
default. In this case, I think it's better to stick as closely as we
can to read(2). Preserving the existing behavior of tools like cat,
too, is worthwhile I think.

Post by Rick Macklem
Note that, although i'd prefer to keep copy_file_range(2) Linux compatible,
I would like to hear others chime in w.r.t. their preference.
rick

Post by Alan Somers

From POSIX: "Note that a read() of zero bytes does not modify the last
data access timestamp. A read() that requests more than zero bytes,
but returns zero, is required to modify the last data access
timestamp."
While copy_file_range is not standardized, it ought to comport to
POSIX as closely as possible. I think we should change it as you
suggest.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

Rick Macklem

2023-10-05 14:52:55 UTC

Permalink

Note that, although i'd prefer to keep copy_file_range(2) Linux compatible,
I would like to hear others chime in w.r.t. their preference.

rick

Post by Alan Somers

From POSIX: "Note that a read() of zero bytes does not modify the last
data access timestamp. A read() that requests more than zero bytes,
but returns zero, is required to modify the last data access
timestamp."
While copy_file_range is not standardized, it ought to comport to
POSIX as closely as possible. I think we should change it as you
suggest.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

Rick Macklem

2023-10-05 22:30:25 UTC

Permalink

Post by Alan Somers
I don't think that Linux is a good model to copy from, where atime is
concerned. It long ago gave up on POSIX-compliance for atime by
default. In this case, I think it's better to stick as closely as we
can to read(2). Preserving the existing behavior of tools like cat,
too, is worthwhile I think.

I have no problem with Mark's patch being applied for the default
local fs case. NFSv4.2 will not be able to comply with this unless
(as will be the case for the FreeBSD server) the NFSv4.2 server
happens to change atime after Mark's patch is applied to the
FreeBSD NFSv4.2 server (the Linux NFSv4.2 server will not).

ZFS..I have no idea. Someone else will need to test it (with block cloning
enabled).

rick

Post by Alan Somers

Post by Rick Macklem
Note that, although i'd prefer to keep copy_file_range(2) Linux compatible,
I would like to hear others chime in w.r.t. their preference.
rick

Post by Alan Somers

From POSIX: "Note that a read() of zero bytes does not modify the last
data access timestamp. A read() that requests more than zero bytes,
but returns zero, is required to modify the last data access
timestamp."
While copy_file_range is not standardized, it ought to comport to
POSIX as closely as possible. I think we should change it as you
suggest.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

Rick Macklem

2023-10-06 01:53:04 UTC

Permalink

Post by Rick Macklem

I have come up with a NFSv4.2 client patch that explicitly sets atime
for the input file in the same compound RPC as the Copy. It works for
a FreeBSD server without Mark's patch. If a NFSv4.2 server does not
do it, we can argue that the server ignores the Setattr of atime.

So, with this patch (which I will be testing against assorted servers next
week (an ietf bakeathon testing event) and Mark's patch, the only case
that may need more work is ZFS?

rick
ps: I'll admit I still doubt anyone cares about atime being set, but the
collective opinion seems to be that it should be set.

Post by Rick Macklem
ZFS..I have no idea. Someone else will need to test it (with block cloning
enabled).
rick

Post by Alan Somers

Post by Rick Macklem
Note that, although i'd prefer to keep copy_file_range(2) Linux compatible,
I would like to hear others chime in w.r.t. their preference.
rick

Post by Alan Somers

From POSIX: "Note that a read() of zero bytes does not modify the last
data access timestamp. A read() that requests more than zero bytes,
but returns zero, is required to modify the last data access
timestamp."
While copy_file_range is not standardized, it ought to comport to
POSIX as closely as possible. I think we should change it as you
suggest.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

Alan Somers

2023-10-06 02:48:41 UTC

Permalink

Post by Rick Macklem

I have come up with a NFSv4.2 client patch that explicitly sets atime
for the input file in the same compound RPC as the Copy. It works for
a FreeBSD server without Mark's patch. If a NFSv4.2 server does not
do it, we can argue that the server ignores the Setattr of atime.
So, with this patch (which I will be testing against assorted servers next
week (an ietf bakeathon testing event) and Mark's patch, the only case
that may need more work is ZFS?
rick
ps: I'll admit I still doubt anyone cares about atime being set, but the
collective opinion seems to be that it should be set.

I think the fusefs copy_file_range will already do this, but I should
at least add a test for it.
-Alan

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de