Date: Sun, 29 Mar 2015 21:42:38 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Jilles Tjoelker <jilles@stack.nl> Cc: src-committers@FreeBSD.org, Don Lewis <truckman@FreeBSD.org>, delphij@FreeBSD.org, brde@optusnet.com.au, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org Subject: Re: svn commit: r280308 - head/sys/fs/devfs Message-ID: <20150329184238.GB2379@kib.kiev.ua> In-Reply-To: <20150329175137.GD95224@stack.nl> References: <20150322162507.GD2379@kib.kiev.ua> <201503221825.t2MIP7jv096531@gw.catspoiler.org> <20150329175137.GD95224@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Mar 29, 2015 at 07:51:37PM +0200, Jilles Tjoelker wrote:
> On Sun, Mar 22, 2015 at 11:25:07AM -0700, Don Lewis wrote:
> > It's not totally worthless. I think the mtime on tty devices is used to
> > calculate the idle time that is printed by the w command. We just don't
> > need nanosecond accuracy for that.
>
> Hmm. The idle time on tty devices breaking is a clear POLA violation,
> but I'm not sure what's the best way to fix it. By the way, w uses atime
> and who -u uses mtime.
>
> Some options are:
>
> * Bypass vfs_timestamp() and hard-code second accuracy in devfs;
> futimens() will still set timestamps to the nanosecond, even with
> UTIME_NOW. A timestamp update after UTIME_NOW setting may set back the
> timestamp to an older value.
>
> * Make vfs.timestamp_precision filesystem-specific. Since this does not
> affect futimens() with explicit timestamps, it will not cause strange
> situations with cp -p.
>
> * Restrict file times on devfs to seconds.
>
> * Somehow add a special case for TTY devices so they will always keep
> timestamps.
>
> * Somehow add a special case for "performance-critical" devices so they
> will not keep timestamps.
>
> * Change the default for vfs.timestamp_precision to 1, which still
> provides non-zero nanoseconds (so much of the same bugs) but is not so
> slow. The file server people won't like this though.
>
> My proposal for delayed updates as in UFS clearly does not work for TTY
> idle times, so there is no point in that.
>
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.
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).
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");
+
+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;
+ }
+ }
+}
static int
devfs_fp_check(struct file *fp, struct cdev **devp, struct cdevsw **dswp,
@@ -1228,9 +1244,8 @@ devfs_read_f(struct file *fp, struct uio *uio, struct ucred *cred,
foffset_lock_uio(fp, uio, flags | FOF_NOLOCK);
error = dsw->d_read(dev, uio, ioflag);
- if (devfs_dotimes &&
- (uio->uio_resid != resid || (error == 0 && resid != 0)))
- vfs_timestamp(&dev->si_atime);
+ if (uio->uio_resid != resid || (error == 0 && resid != 0))
+ devfs_timestamp(&dev->si_atime);
td->td_fpop = fpop;
dev_relthread(dev, ref);
@@ -1708,9 +1723,8 @@ devfs_write_f(struct file *fp, struct uio *uio, struct ucred *cred,
resid = uio->uio_resid;
error = dsw->d_write(dev, uio, ioflag);
- if (devfs_dotimes &&
- (uio->uio_resid != resid || (error == 0 && resid != 0))) {
- vfs_timestamp(&dev->si_ctime);
+ if (uio->uio_resid != resid || (error == 0 && resid != 0)) {
+ devfs_timestamp(&dev->si_ctime);
dev->si_mtime = dev->si_ctime;
}
td->td_fpop = fpop;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150329184238.GB2379>