From owner-freebsd-fs@FreeBSD.ORG Wed Jan 16 03:49:20 2013 Return-Path: Delivered-To: fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id D681726E; Wed, 16 Jan 2013 03:49:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id 5BC7B344; Wed, 16 Jan 2013 03:49:19 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r0G3nBT5001550 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 16 Jan 2013 14:49:13 +1100 Date: Wed, 16 Jan 2013 14:49:11 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin Subject: Re: [PATCH] Better handle NULL utimes() in the NFS client In-Reply-To: <201301151458.42874.jhb@freebsd.org> Message-ID: <20130116134627.S1060@besplex.bde.org> References: <162405990.1985479.1358212854967.JavaMail.root@erie.cs.uoguelph.ca> <20130115141019.H1444@besplex.bde.org> <201301151458.42874.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=P/xiHV8u c=1 sm=1 a=S8Qr1IbAvFsA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=U1Z5fgpPGSMA:10 a=kNFpKb6NvubOC5D93twA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: Rick Macklem , fs@FreeBSD.org X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jan 2013 03:49:20 -0000 On Tue, 15 Jan 2013, John Baldwin wrote: > On Monday, January 14, 2013 11:51:23 pm Bruce Evans wrote: >> >> I can't see anything that does the different permissions check for >> the VA_UTIMES_NULL case, and testing shows that this case is just broken, >> at least for an old version of the old nfs client -- the same permissions >> are required for all cases, but write permission is supposed to be >> enough for the VA_UTIMES_NULL case (since write permission is sufficient >> for setting the mtime to the current time (plus epsilon) using write(2) >> and truncate(2). Setting the atime to the current time should require >> no more and no less than read permission, since it can be done using >> read(2), but utimes(NULL) requires write permission for that too). > > Correct. All the other uses of VA_UTIMES_NULL in the tree are to > provide the permissions check you describe and there is a large > comment about it in ufs_setattr(). Other filesystems have comments > that reference ufs_setattr(). I think these checks should be done > in nfs_setattr() rather than in the routine to build an NFS attribute > object however. Perhaps it can be done in vfs. There are some technical problems with this, but perhaps they are small. One is that file systems might not even have any timestamps. (I forgot to mention a related problem with the error handling. The permissions checks for utimes() are usually too strict for file systems that only have fake ownerships, like msdosfs. OTOH, msdosfs also doesn't have atimes for most variants of the file system. Since utimes() is supposed to set both the mtime and the atime (especially in the non-NULL case), it strictly cannot work on msdosfs. But msdosfs is not strict about this. It silently ignores the atimes when it can't set them.) > Fixing NFS to properly use vfs_timestamp() seems to be a larger > project. I think it is smaller. For the new nfs code, it is not as simple as changing the NFSGETTIME() macro, since nfs wants extra precision in most cases (for things like comparing cache times), and very rarely wants the semantics of vfs_timestamp(). I somehow missed seeing seeing even more confusion in this area: - the new nfs code also has a macro NFSGETNANOTIME() which reduces to getnanotime(). - monotonic times should be used if possible, but the new nfs code only uses them for NFSD_MONOSEC (which is used a lot) and in 2 places in nfs_commonkrpc.c where a hard-coded getmicrouptime() is used. - NFSGETNANOTIME() is used 4 times. But there are 4 hard-coded uses of getnanotime(). The latter are mostly in places where vfs_timestamp() is correct, for things like n_atim for fifos. nfs almost never needs to set file timestamps, since most file timestamps are set by leaf (non-nfs) file systems on the server. The only exceptions that I know about are the ones already noted (utimes(NULL) on the server, something in create() (?), and n_atim for fifos on the client (?). n_atim for special files on the client were an exception when special files were supported. In the old nfs code: - there are no NFSGET*TIME() macros, and there don't seem to be any get*time() calls instead either. The get*time() calls used are almost exactly the same ones as in the nfs nfs code, with the only exception that I noticed being the ones in the server for utimes(NULL). - monotonic times are only used in the same 2 places in nfs_commonkrpc.c. The non-monotonic time_second is used a lot (hard-coded), I think in much the same places where the new nfs server code time_uptime via NFSD_MONOSEC. I don't like obfuscating standard time calls using macros. Others that I don't like: - both the old and the new nfs client use NFS_TIMESPEC_COMPARE() instead of the standard and better timespeccmp(). NFS_TIMESSPEC_COMPARE() is more verbose and only compares for equality. Its implementation is home made and not based on timespeccmp(). - the new nfs client also has a macro NFS_CMPTIME(). This gives the same result as NFS_TIMESPEC_COMPARE() (or timespeccmp(..., =).. Iti works accidentally for both timespecs and timevals, due to the POSIX bug that the struct members for timespecs abuse the prefix for timevals in their spelling. Code derived from the old nfs client has not been translated to use the new macro (the "new" macro is probably actually older and may even be older or more likely just from a different code base than FreeBSD's timespeccmp()). - the new nfs client also has a macro NFS_SETTIME(). This doesn't actually set the time, but converts from the global timeval `time' to a timespec in the same way as the standard macro TIMEVAL_TO_TIMESPEC() would if it is passed a pointer to the global timeval. Accessing the global `time' like this would give races. Fortunately, the global `time' doesn't exist in FreeBSD, so of course this macro is never used. I like NFSD_MONOSEC, however. Global variables give a much more fragile and harder to translate API than function calls and function-like macros. Bruce