Change default VFS timestamp precision?

[Jilles Tjoelker]

On Sat, Feb 07, 2015 at 09:15:30PM +1100, Bruce Evans wrote:
> On Sat, 7 Feb 2015, Jilles Tjoelker wrote:

> > On Wed, Dec 17, 2014 at 11:39:37AM +1100, Bruce Evans wrote:
> >> This reminds me of related bugs in touch(1):
> >> - touch also tries first with a non-NULL timep.  So it uses its own idea
> >>    of the current time.  This is normally the one returned a little in
> >>    the past by gettimeofday().  It has microseconds resolution.  This
> >>    normally succeeds, giving a result that is inconsistent with the
> >>    one that would result from using a NULL timep, since the kernel now
> >>    honors vfs.timestamp_precision and that is not usually 2.

> > Yes, this is wrong the same way as in "make -t".

> >> - sometimes the first try fails.  Then touch retries with a NULL timep,
> >>    if the semantics allow this.  Sometimes this succeeds.

> > Another bug: it retries with a NULL timep even if -a or -m is given,
> > requesting only one of the timestamps be updated.

> Perhaps that is the best that it can do, or it can be treated as an error
> since the application/user shouldn't try to set only one of the timestamps
> on a file system that doesn't support both.  However, it might support
> just the one asked for, and asking for both when only 1 is supported is
> not treated as an error.

Perhaps I did not explain this clearly. My point was that, as a non-root
user, "touch -a /dev/null" behaves as "touch /dev/null" instead of
failing or using read().

> > The new functionality in futimens/utimensat allows doing "touch -a" and
> > "touch -m" with the current time properly (setting one timestamp to
> > vfs_timestamp and leaving the other timestamp unchanged). As with the
> > old methods, only the owner of the file and root can do this.

> When this gets down to individual file systems, the timestamp to be left
> unchanged becomes VNOVAL and is always (?) ignored.  However, to make
> the usual case when changes to both timestamps are requested work,
> requests to change nonexistent attributes are usually ignored too.  I
> don't see any better way to control this than a mount flag for every
> nonexistent attribute.  msdosfs currently has hard-coded policies that
> differ for each nonexistent attribute.  They are mostly too strict except
> for atimes.

The code in sys/kern/vfs_syscalls.c sets vattr.va_vaflags |=
VA_UTIMES_NULL if the timestamp was null or two times UTIME_NOW. This is
what file systems use for permission checks (via vn_utimes_perm()).

Although msdosfs supports file systems without Windows 95 extensions
that do not have atimes, this is rather rare and not worth much time for
bug fixing.

> > The below patch fixes these problems.

> Looks good.

Thanks for the review.

> > ...
> >> The non-NULL timep in make gives the following bug since make doesn't
> >> retry with a NULL timep: make -t fails if the files ar writeable but
> >> not owned.

> > This is best fixed by always using a NULL timep.

> I think this is OK for msdosfs.  The problems with setting timestamps
> in msdosfs are more in cp -p and mv.  There the NULL timep cannot be
> used.  msdosfs is very strict in this case.  cp and mv are also strict,
> except mv was broken recently to hide the corresponding problems for
> unsettable flags (e.g., zfs sets the archive flag a lot, but nfs doesn't
> support preserving it and hard-codes strict failures for attempts to set
> it).  The strictness is mostly just to fail to copy uncopyable attributes
> and just warn about this.  Even the warnings are too much when they occur
> for every file.  Even mv is mostly not required by POSIX to duplicate
> attributes.  But it is required to warn.  So nfs's strictness for
> chflags() is the correct default and msdosfs's non-strictness for
> atimes is an incorrect default, but more practical.

> > Index: usr.bin/touch/touch.c
> > ===================================================================
> > --- usr.bin/touch/touch.c	(revision 277645)
> > +++ usr.bin/touch/touch.c	(working copy)
> > ...
> > @@ -239,7 +222,7 @@
> > 	int yearset;
> > 	char *p;
> > 					/* Start with the current time. */
> > -	now = tvp[0].tv_sec;
> > +	now = time(NULL);
> > 	if ((t = localtime(&now)) == NULL)
> > 		err(1, "localtime");
> > 					/* [[CC]YY]MMDDhhmm[.SS] */

> This seems to be broken now, since time() is broken now.  I think tvp
> was obtained using clock_gettime(CLOCK_REALTIME, ... ) (previously
> gettimeofday).  These give the correct time.  time() used to use
> gettimeofday() to get the same correct time.  But someone broke time().
> It now uses clock_gettime(TIME_SECOND, ... ).  TIME_SECOND is getbintime()
> rounded down.  getbintime() lags the current time by an average of
> tc_tick * 1/hz / 2 seconds.  After rounding down, TIME_SECOND lags the
> current time by a full seconds for a duration of an  average of
> tc_tick * 1/hz / 2 seconds every second.

> The bug is compatible with the one given by vfs.timestamp_precision = 0
> and other low settings of the sysctl.  The latter also uses the lagging
> clock.  Previously, the incoherent clocks showed up in file times tests
> like:

>  	time(&t1);
>  	write to a file
>  	stat(file, &sb);
>  	assert(t1 <= sb.st_mtime);  /* can fail due to incoherent clocks */

> Add delays as necessary to compensate for the speed of light but not to
> compensate for buggy clocks.

> time() now uses the same buggy clock when vfs.timestamp_precision = 0, so
> the clocks are coherent because they are the same (unless the real time is
> stepped).  OTOH, if CLOCK_GETTIME is used to read t1, then the bug is
> detected as before.

> When vfs.timestamp_precision is higher so that the time is up to date,
> then the incoherencies go in the other direction:

>  	write to a file
>  	stat(file, &sb);
>  	time(&t1);
>  	assert(t1 >= sb.st_mtime);  /* can fail due to incoherent clocks */

> This looks like a file times test, but it actually finds the buggy time().

> > @@ -301,7 +284,7 @@
> > 	time_t now;
> > 	struct tm *t;
> > 					/* Start with the current time. */
> > -	now = tvp[0].tv_sec;
> > +	now = time(NULL);
> > 	if ((t = localtime(&now)) == NULL)
> > 		err(1, "localtime");

> Similarly.

> THe comment should be attached to the or just omitted now that the code
> obiously fetches the (buggy) current time.

> For a quick fix, keep using clock_gettime(CLOCK_REALTIME, ...).  I
> don't see a good way to be bug for bug compatible with the file system.
> A bad way is to write to a file on the same file system and stat it,
> as in the above tests.  This gives a time slightly in the past according
> to the file system's clock.  nfs gives interesting complications.  The
> server's clock may be much more incoherent with CLOCK_REALTIME than
> the local clock.

The code that now uses time() is for "touch -t" and the obsolete
timestamp operand. It serves to supply the year if it is not given, and
the seconds in the obsolete timestamp operand. It does not supply a full
tv_sec.

-- 
Jilles Tjoelker