From owner-svn-src-head@FreeBSD.ORG Mon Mar 30 04:14:13 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id EAAD981B; Mon, 30 Mar 2015 04:14:13 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id AA288B55; Mon, 30 Mar 2015 04:14:13 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 9ECBA782F0B; Mon, 30 Mar 2015 15:14:11 +1100 (AEDT) Date: Mon, 30 Mar 2015 15:14:10 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov Subject: Re: svn commit: r280308 - head/sys/fs/devfs In-Reply-To: <20150329184238.GB2379@kib.kiev.ua> Message-ID: <20150330145148.C1660@besplex.bde.org> References: <20150322162507.GD2379@kib.kiev.ua> <201503221825.t2MIP7jv096531@gw.catspoiler.org> <20150329175137.GD95224@stack.nl> <20150329184238.GB2379@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=A5NVYcmG c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=Ie7hrarWT0n9zBAm1v4A:9 a=CjuIK1q_8ugA:10 Cc: src-committers@freebsd.org, Jilles Tjoelker , svn-src-all@freebsd.org, delphij@freebsd.org, brde@optusnet.com.au, svn-src-head@freebsd.org, Don Lewis X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Mar 2015 04:14:14 -0000 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