svn commit: r280308 - head/sys/fs/devfs

[Bruce Evans]

On Sun, 22 Mar 2015, Jilles Tjoelker wrote:

> On Sat, Mar 21, 2015 at 08:49:00PM +1100, Bruce Evans wrote:
>> On Sat, 21 Mar 2015, Xin LI wrote:
>
>>> Log:
>>>  Disable timestamping on devfs read/write operations by default.
>
>> I don't like this.  It defaults to non-POSIX-conformant behaviour...
>
>> The slowness is mostly from no delayed update of times in devfs.
> ...
> Yes, I think implementing delayed updates is the right solution to this
> problem. This way, only stat and last close will need to read the clock.
> No configuration option will be needed.
>
> A subtle difference with most other file systems is that devfs nodes
> often stay open for very long, so the timestamps will usually come from
> stat() calls, which may be much later than the actual read or write.
> Still that is better than not updating timestamps at all.

It is probably the large important files that stay open a lot.

Another not so subtle difference is that atimes are actually useful
for some device files.  They are used by w(1).

My IN_LAZYMOD optimizations for ffs are relevant.  They should be
used for atimes for all file types, but the committed version only
used for device files, and this code is now dead since device files
moved to devfs.  This code was also dead (bypassed due to missing
details) before devfs in the soft updates case.  My version sets
IN_LAZMOD for regular files, but then kills it in another way:

X Index: ufs_vnops.c
X ===================================================================
X RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v
X retrieving revision 1.239
X diff -u -2 -r1.239 ufs_vnops.c
X --- ufs_vnops.c	7 Apr 2004 03:47:20 -0000	1.239
X +++ ufs_vnops.c	4 Dec 2007 15:41:26 -0000
X @@ -157,8 +157,22 @@
X  	if ((ip->i_flag & (IN_ACCESS | IN_CHANGE | IN_UPDATE)) == 0)
X  		return;
X +	/*
X +	 * XXX IN_LAZYMOD has still not been verified to work for the
X +	 * DOINGSOFTDEP() case.
X +	 * XXX devfs makes IN_LAZYMOD almost moot for the device case ...
X +	 */
X  	if ((vp->v_type == VBLK || vp->v_type == VCHR) && !DOINGSOFTDEP(vp))
X  		ip->i_flag |= IN_LAZYMOD;
X -	else
X -		ip->i_flag |= IN_MODIFIED;
X +	else {
X +		/*
X +		 * ... but IN_LAZYMOD should be used in more cases.  Start
X +		 * using it for atime-only updates.
X +		 */
X +		if (ip->i_flag & (IN_CHANGE | IN_UPDATE))
X +			ip->i_flag |= IN_MODIFIED;
X +		else
X +			ip->i_flag |= IN_LAZYMOD;

The previous line is executed for the IN_ATIME case.

X +		ip->i_flag |= IN_MODIFIED;	/* XXX change/mod confusion */

But then override the change.  Motivation for completing this is small
since I mount most file systems with -noatime, so the code is unreachable
for another reason.

X +	}
X  	if ((vp->v_mount->mnt_flag & MNT_RDONLY) == 0) {
X  		vfs_timestamp(&ts);

Before checking the above, I thought it would be a reasonable fix to
implement only mounting with noatime for devfs, and require users
to mount with noatime if they notice the slowness.  Most device
accesses are probably for reads.  Checking the MNT_NOATIME flag
takes about 1 line.

>>> ...
>>> Modified: head/sys/fs/devfs/devfs_vnops.c
>>> ==============================================================================
>>> --- head/sys/fs/devfs/devfs_vnops.c	Sat Mar 21 00:21:30 2015	(r280307)
>>> +++ head/sys/fs/devfs/devfs_vnops.c	Sat Mar 21 01:14:11 2015	(r280308)
>>> ...
>>> @@ -1700,7 +1708,8 @@ devfs_write_f(struct file *fp, struct ui
>>> 	resid = uio->uio_resid;
>>>
>>> 	error = dsw->d_write(dev, uio, ioflag);
>>> -	if (uio->uio_resid != resid || (error == 0 && resid != 0)) {
>>> +	if (devfs_dotimes &&
>>> +	    (uio->uio_resid != resid || (error == 0 && resid != 0))) {
>>> 		vfs_timestamp(&dev->si_ctime);
>>> 		dev->si_mtime = dev->si_ctime;
>>> 	}
>
>> An old bug is evident in the diff.  Writing shouldn't change the ctime.
>
> That is not a bug. POSIX unambiguously requires write() to update both
> mtime and ctime.

Oops.  I don't know how I forgot that.  ffs's IN_* flag names are mixed
up, but in ffs_write() IN_CHANGE is the one flag that is not affected
there by the confusion -- it actually used for exactly what it means
(that the file change time is marked for update, but the inode has not
been modified for this since the update has not occured yet).

Bruce