Date: Mon, 30 Mar 2015 15:14:10 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, Jilles Tjoelker <jilles@stack.nl>, svn-src-all@freebsd.org, delphij@freebsd.org, brde@optusnet.com.au, svn-src-head@freebsd.org, Don Lewis <truckman@freebsd.org> Subject: Re: svn commit: r280308 - head/sys/fs/devfs Message-ID: <20150330145148.C1660@besplex.bde.org> In-Reply-To: <20150329184238.GB2379@kib.kiev.ua> References: <20150322162507.GD2379@kib.kiev.ua> <201503221825.t2MIP7jv096531@gw.catspoiler.org> <20150329175137.GD95224@stack.nl> <20150329184238.GB2379@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 29 Mar 2015, Konstantin Belousov wrote:
> Interesting complication with the devfs timestamp update is that
> devfs_read_f() and devfs_write_f() do not lock the vnode. So whatever
> update method is used, stat(2) on devfs might return inconsistent value,
> since tv_src/tv_nsec cannot be updated or read by single op, without
> locking.
Urk.
> That said, we could either:
> - ignore the race. Then the patch below might be good enough.
> - Trim the precision of futimens() for devfs to always set tv_nsec to 0.
> Then, the race can be handled satisfactory with atomics.
> - Add a lock (IMO this does not make sense).
Locks aren't that expensive, and are easier to use than atomics.
> diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
> index 941f92c..e199d52 100644
> --- a/sys/fs/devfs/devfs_vnops.c
> +++ b/sys/fs/devfs/devfs_vnops.c
> @@ -83,8 +83,24 @@ MTX_SYSINIT(cdevpriv_mtx, &cdevpriv_mtx, "cdevpriv lock", MTX_DEF);
> SYSCTL_DECL(_vfs_devfs);
>
> static int devfs_dotimes;
> -SYSCTL_INT(_vfs_devfs, OID_AUTO, dotimes, CTLFLAG_RW,
> - &devfs_dotimes, 0, "Update timestamps on DEVFS");
> +SYSCTL_INT(_vfs_devfs, OID_AUTO, dotimes, CTLFLAG_RW, &devfs_dotimes, 0,
> + "Update timestamps on DEVFS with default precision");
This fixes 1 style bug (the tab) but adds another. The normal formatting
is to split the declaration after CTLFLAG*. Then preferably use a less
verbose discription so as to not need to split the line. The old
description was a bit cryptic (but better than the sysctl name).
> +
> +static void
> +devfs_timestamp(struct timespec *tsp)
> +{
> + time_t ts;
> +
> + if (devfs_dotimes) {
> + vfs_timestamp(tsp);
> + } else {
> + ts = time_second;
> + if (tsp->tv_sec < ts) {
> + tsp->tv_sec = ts;
> + tsp->tv_nsec = 0;
> + }
File timestamps use CLOCK_REALTIME, so they are supposed to go backwards
sometimes. More importantly, if the time is set to a future time (by
utimes(), etc., not due to a clock step), this prevents it being correted.
I think you only want to do a null update if tv_nsec is nonzero due to a
previous setting with vfs_timestamp(), and the new second hasn't arrived
yet. Something like:
if (tsp->tv_sec != ts) ...
If '<', then as above. If '>', it means a correction by >= 1 second
(strictly greater if tv_nsec != 0). The initial value tv_nsec is
irrelevant in both cases.
> + }
> +}
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150330145148.C1660>