Date: Thu, 5 Oct 2023 18:53:04 -0700 From: Rick Macklem <rick.macklem@gmail.com> To: Alan Somers <asomers@freebsd.org> Cc: Mark Johnston <markj@freebsd.org>, freebsd-hackers@freebsd.org, rmacklem@freebsd.org Subject: Re: copy_file_range() doesn't update the atime of an empty file Message-ID: <CAM5tNy70AmgrsLmYPqgiDkPAP10msv77o8E_DzCmiO0LycEnfg@mail.gmail.com> In-Reply-To: <CAM5tNy4k%2BfZYC8MVJO5gGf9%2Bo=Fi0sL8ER_kckrwZmi6Fwt9ow@mail.gmail.com> References: <ZR2FUeIhO7DIQIpj@nuc> <CAOtMX2h7QLqLHPm-gUMDJKeR8oyAXssn2vxkJ8xNgBBT6Cc3bw@mail.gmail.com> <CAM5tNy72tPBLHM8mkhqkUu64GuLUiZuKFJ%2B2JFsOzVgA1hm0eA@mail.gmail.com> <CAM5tNy5nLWf9c%2BnsdxJsU-M9Q3p_VVc%2BnpuY6uwbZPwM6EwhKg@mail.gmail.com> <CAM5tNy4%2BZZTYQ4QuD_sapx3q%2BQ%2Bwz9uNu6CGL17JFsjN13i0Sg@mail.gmail.com> <CAOtMX2jSXLnhjN1JDxk9N_NCjjjKWxguhsb05F4ww9mKwcbSsg@mail.gmail.com> <CAM5tNy4k%2BfZYC8MVJO5gGf9%2Bo=Fi0sL8ER_kckrwZmi6Fwt9ow@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Oct 5, 2023 at 3:30=E2=80=AFPM Rick Macklem <rick.macklem@gmail.com=
> wrote:
>
> On Thu, Oct 5, 2023 at 8:41=E2=80=AFAM Alan Somers <asomers@freebsd.org> =
wrote:
> >
> > 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).
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.
>
> ZFS..I have no idea. Someone else will need to test it (with block clonin=
g
> enabled).
>
> rick
> >
> > On Thu, Oct 5, 2023 at 7:53=E2=80=AFAM Rick Macklem <rick.macklem@gmail=
.com> wrote:
> > >
> > > Note that, although i'd prefer to keep copy_file_range(2) Linux compa=
tible,
> > > I would like to hear others chime in w.r.t. their preference.
> > >
> > > rick
> > >
> > > On Wed, Oct 4, 2023 at 4:39=E2=80=AFPM Rick Macklem <rick.macklem@gma=
il.com> wrote:
> > > >
> > > > Resent now that I am subscribed to freebsd-hackers@,
> > > >
> > > > On Wed, Oct 4, 2023 at 4:25=E2=80=AFPM Rick Macklem <rick.macklem@g=
mail.com> wrote:
> > > > >
> > > > > On Wed, Oct 4, 2023 at 8:40=E2=80=AFAM Alan Somers <asomers@freeb=
sd.org> wrote:
> > > > > >
> > > > > > CAUTION: This email originated from outside of the University o=
f Guelph. Do not click links or open attachments unless you recognize the s=
ender and know the content is safe. If in doubt, forward suspicious emails =
to IThelp@uoguelph.ca.
> > > > > >
> > > > > >
> > > > > > On Wed, Oct 4, 2023 at 8:31=E2=80=AFAM Mark Johnston <markj@fre=
ebsd.org> wrote:
> > > > > > >
> > > > > > > For a while, Jenkins has been complaining that one of the tmp=
fs tests is
> > > > > > > failing:
> > > > > > > https://ci.freebsd.org/job/FreeBSD-main-amd64-test/23814/test=
Report/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 fi=
le, waits
> > > > > > > for a second, then cat(1)s it and checks that the file's atim=
e was
> > > > > > > updated. After the aforementioned commit, the atime is not u=
pdated.
> > > > > > >
> > > > > > > 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 ev=
en 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 wonde=
r if we
> > > > > > > could at least change it to call VOP_READ in that scenario, a=
s 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 -=3D (*inoffp % blksize);
> > > > > > > }
> > > > > > > /* Loop copying the data block. */
> > > > > > > - while (copylen > 0 && error =3D=3D 0 && !eof =
&& interrupted =3D=3D 0) {
> > > > > > > + while (error =3D=3D 0 && !eof && interrupted =
=3D=3D 0) {
> > > > > > > if (copylen < xfer)
> > > > > > > xfer =3D copylen;
> > > > > > > error =3D 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 =3D false;
> > > > > > > - if (error =3D=3D 0 && aresid > 0) {
> > > > > > > + if (error =3D=3D 0 && (xfer =3D=3D 0 =
|| aresid > 0)) {
> > > > > > > /* Stop the copy at EOF on th=
e input file. */
> > > > > > > xfer -=3D aresid;
> > > > > > > eof =3D true;
> > > > > > >
> > > > > >
> > > > > > From POSIX: "Note that a read() of zero bytes does not modify t=
he last
> > > > > > data access timestamp. A read() that requests more than zero by=
tes,
> > > > > > 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 y=
ou
> > > > > > suggest.
> > > > > Well, I'd like to maintain the syscall as "Linux compatible", whi=
ch 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 f=
iles for
> > > > > this reason. (I eventually intend to patch the man page to clari=
fy 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 w=
hen a
> > > > > copy_file_range() returns 0 bytes. I just did a test on Linux (ke=
rnel
> > > > > 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 t=
he 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 ma=
ke
> > > > > it look close to POSIX compliant). As such, expecting a copy_fil=
e_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 compat=
ible.
> > > > > Does cat(1) really need to exhibit this behaviour or is it just r=
ead(2)
> > > > > that specifies this?
> > > > >
> > > > > rick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAM5tNy70AmgrsLmYPqgiDkPAP10msv77o8E_DzCmiO0LycEnfg>