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

On Wed, Oct 4, 2023 at 8:31 AM Mark Johnston <markj@freebsd.org> wrote:
>
> 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;
>

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.