Skip site navigation (1)Skip section navigation (2)
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>