From owner-freebsd-fs@FreeBSD.ORG Tue Jan 15 01:22:34 2013 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id D0F9F5C3; Tue, 15 Jan 2013 01:22:34 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-annu.net.uoguelph.ca (esa-annu.mail.uoguelph.ca [131.104.91.36]) by mx1.freebsd.org (Postfix) with ESMTP id 7672D9CB; Tue, 15 Jan 2013 01:22:33 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqAEAOCu9FCDaFvO/2dsb2JhbABEhjqzZYN0c4IeAQEFIwRSGw4KAgINGQJZBogspS+QW4EjjniBEwOIYY0qkEmDE4IG X-IronPort-AV: E=Sophos;i="4.84,469,1355115600"; d="scan'208";a="9060144" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-annu.net.uoguelph.ca with ESMTP; 14 Jan 2013 20:20:55 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id F246FB3F26; Mon, 14 Jan 2013 20:20:54 -0500 (EST) Date: Mon, 14 Jan 2013 20:20:54 -0500 (EST) From: Rick Macklem To: John Baldwin Message-ID: <162405990.1985479.1358212854967.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <201301141445.29260.jhb@freebsd.org> Subject: Re: [PATCH] Better handle NULL utimes() in the NFS client MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.201] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - FF3.0 (Win)/6.0.10_GA_2692) 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: Tue, 15 Jan 2013 01:22:34 -0000 John Baldwin wrote: > The NFS client tries to infer when an application has passed NULL to > utimes() > so that it can let the server set the timestamp rather than using a > client- > supplied timestamp. It does this by checking to see if the desired > timestamp's second matches the current second. However, this breaks > applications that are intentionally trying to set a specific timestamp > within > the current second. In addition, utimes() sets a flag to indicate if > NULL was > passed to utimes(). The patch below changes the NFS client to check > this flag > and only use the server-supplied time in that case: > > Index: fs/nfsclient/nfs_clport.c > =================================================================== > --- fs/nfsclient/nfs_clport.c (revision 225511) > +++ fs/nfsclient/nfs_clport.c (working copy) > @@ -762,7 +762,7 @@ > *tl = newnfs_false; > } > if (vap->va_atime.tv_sec != VNOVAL) { > - if (vap->va_atime.tv_sec != curtime.tv_sec) { > + if (!(vap->va_vaflags & VA_UTIMES_NULL)) { > NFSM_BUILD(tl, u_int32_t *, 3 * NFSX_UNSIGNED); > *tl++ = txdr_unsigned(NFSV3SATTRTIME_TOCLIENT); > txdr_nfsv3time(&vap->va_atime, tl); > @@ -775,7 +775,7 @@ > *tl = txdr_unsigned(NFSV3SATTRTIME_DONTCHANGE); > } > if (vap->va_mtime.tv_sec != VNOVAL) { > - if (vap->va_mtime.tv_sec != curtime.tv_sec) { > + if (!(vap->va_vaflags & VA_UTIMES_NULL)) { > NFSM_BUILD(tl, u_int32_t *, 3 * NFSX_UNSIGNED); > *tl++ = txdr_unsigned(NFSV3SATTRTIME_TOCLIENT); > txdr_nfsv3time(&vap->va_mtime, tl); > Index: nfsclient/nfs_subs.c > =================================================================== > --- nfsclient/nfs_subs.c (revision 225511) > +++ nfsclient/nfs_subs.c (working copy) > @@ -1119,7 +1119,7 @@ > *tl = nfs_false; > } > if (va->va_atime.tv_sec != VNOVAL) { > - if (va->va_atime.tv_sec != time_second) { > + if (!(vattr.va_vaflags & VA_UTIMES_NULL)) { > tl = nfsm_build_xx(3 * NFSX_UNSIGNED, mb, bpos); > *tl++ = txdr_unsigned(NFSV3SATTRTIME_TOCLIENT); > txdr_nfsv3time(&va->va_atime, tl); > @@ -1132,7 +1132,7 @@ > *tl = txdr_unsigned(NFSV3SATTRTIME_DONTCHANGE); > } > if (va->va_mtime.tv_sec != VNOVAL) { > - if (va->va_mtime.tv_sec != time_second) { > + if (!(vattr.va_vaflags & VA_UTIMES_NULL)) { > tl = nfsm_build_xx(3 * NFSX_UNSIGNED, mb, bpos); > *tl++ = txdr_unsigned(NFSV3SATTRTIME_TOCLIENT); > txdr_nfsv3time(&va->va_mtime, tl); > > -- > John Baldwin I think this patch is ok, too. In the old days, a lot of NFS servers only stored times at a resolution of 1sec, which I think is why the code had the habit of comparing "seconds equal". If there is some app. out there that sets "current time" via utimes(2) with a curent time argument instead of a NULL argument would seem to be broken to me. (It is conceivable that some app. did this to avoid clock skew between the client and server, but I doubt it.) Have fun with it, rick ps: If you were concerned that the change might break something that depended on the old behaviour, you could apply the patch to the new client only. Then switching to an "oldnfs" mount would provide the old "same sec->set time to current time on the server" behaviour.