From patchwork Mon Jan 23 15:36:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Christoph Hellwig X-Patchwork-Id: 9532843 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C2CD16042D for ; Mon, 23 Jan 2017 15:36:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B1840283F3 for ; Mon, 23 Jan 2017 15:36:20 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A40A428422; Mon, 23 Jan 2017 15:36:20 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EA86D283F3 for ; Mon, 23 Jan 2017 15:36:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750732AbdAWPgS (ORCPT ); Mon, 23 Jan 2017 10:36:18 -0500 Received: from verein.lst.de ([213.95.11.211]:42949 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750705AbdAWPgS (ORCPT ); Mon, 23 Jan 2017 10:36:18 -0500 Received: by newverein.lst.de (Postfix, from userid 2407) id A701268DE0; Mon, 23 Jan 2017 16:36:15 +0100 (CET) Date: Mon, 23 Jan 2017 16:36:15 +0100 From: Christoph Hellwig To: Jeff Layton Cc: Christoph Hellwig , bfields@redhat.com, linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH] nfsd: special case truncates some more Message-ID: <20170123153615.GA32201@lst.de> References: <1485104060-15209-1-git-send-email-hch@lst.de> <1485104060-15209-2-git-send-email-hch@lst.de> <1485174116.2786.7.camel@poochiereds.net> <20170123123348.GA28102@lst.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170123123348.GA28102@lst.de> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Jan 23, 2017 at 01:33:48PM +0100, Christoph Hellwig wrote: > I'll need to look at the exact NFS semantics in that area, but after > a bit of research I can probably come up with something that will work. Here is my first attempt. As vfs_truncate will add the ctime and mtime updates when needed it just leaves handling that quirk to vfs_truncate and then exits early if no other attributes are set. Unfortunately at least the Linux client always seems to also request a mtime update with a size update. We could keep the if (iap->ia_size != i_size_read(inode)) check from the old code and remove ATTR_MTIME, but these racy checks outside i_rwsem make me feel a bit uneasy. Jeff, Bruce - any opinion if we should add something like this: /* vfs_truncate will update ctime and mtime if the size changes */ if (iap->ia_size != i_size_read(inode)) iap->ia_valid &= ATTR_MTIME; back to nfsd_setattr? This would avoid the additional setattr call, but make me feel dirty :) --- From 0e06e2fc6157bb97692ed47c21e36120efb9f15c Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Sun, 22 Jan 2017 17:17:48 +0100 Subject: nfsd: special case truncates some more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the NFS protocols and the Linux VFS use a setattr operation with a bitmap of attributs to set to set various file attributes including the file size and the uid/gid. The Linux syscalls never mixe size updates with unrelated updates like the uid/gid, and some file systems like XFS and GFS2 rely on the fact that truncates might not update random other attributes, and many other file systems handle the case but do not update the different attributes in the same transaction. NFSD on the other hand passes the attributes it gets on the wire more or less directly through to the VFS, leading to updates the file systems don't expect. XFS at least has an assert on the allowed attributes, which cought an NFS client sets the size and group ІD at the same time. To handles this issue properly this switches nfsd to call vfs_truncate for size changes, and then handling all other attributes through notify_change. As a side effect this also means less boilerplace code around the size change as we can now reuse the VFS code. Signed-off-by: Christoph Hellwig --- fs/nfsd/vfs.c | 92 +++++++++++++++++++---------------------------------------- 1 file changed, 30 insertions(+), 62 deletions(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 26c6fdb..4ca5b92 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -332,37 +332,6 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap) } } -static __be32 -nfsd_get_write_access(struct svc_rqst *rqstp, struct svc_fh *fhp, - struct iattr *iap) -{ - struct inode *inode = d_inode(fhp->fh_dentry); - int host_err; - - if (iap->ia_size < inode->i_size) { - __be32 err; - - err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry, - NFSD_MAY_TRUNC | NFSD_MAY_OWNER_OVERRIDE); - if (err) - return err; - } - - host_err = get_write_access(inode); - if (host_err) - goto out_nfserrno; - - host_err = locks_verify_truncate(inode, NULL, iap->ia_size); - if (host_err) - goto out_put_write_access; - return 0; - -out_put_write_access: - put_write_access(inode); -out_nfserrno: - return nfserrno(host_err); -} - /* * Set various file attributes. After this call fhp needs an fh_put. */ @@ -377,7 +346,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, __be32 err; int host_err; bool get_write_count; - int size_change = 0; if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE)) accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE; @@ -390,11 +358,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, /* Get inode */ err = fh_verify(rqstp, fhp, ftype, accmode); if (err) - goto out; + return err; if (get_write_count) { host_err = fh_want_write(fhp); if (host_err) - return nfserrno(host_err); + goto out_host_err; } dentry = fhp->fh_dentry; @@ -405,50 +373,50 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, iap->ia_valid &= ~ATTR_MODE; if (!iap->ia_valid) - goto out; + return 0; nfsd_sanitize_attrs(inode, iap); + if (check_guard && guardtime != inode->i_ctime.tv_sec) + return nfserr_notsync; + /* * The size case is special, it changes the file in addition to the - * attributes. + * attributes, and file systems don't expect it to be mixed with + * "random" attribute changes. We thus split out the size change + * into a separate calo for vfs_truncate, and do the rest as a + * a separate setattr call. + * + * Note that vfs_truncate will also update ctime and mtime if + * the file size changes. */ if (iap->ia_valid & ATTR_SIZE) { - err = nfsd_get_write_access(rqstp, fhp, iap); - if (err) - goto out; - size_change = 1; + struct path path = { + .mnt = fhp->fh_export->ex_path.mnt, + .dentry = dentry, + }; - /* - * RFC5661, Section 18.30.4: - * Changing the size of a file with SETATTR indirectly - * changes the time_modify and change attributes. - * - * (and similar for the older RFCs) - */ - if (iap->ia_size != i_size_read(inode)) - iap->ia_valid |= ATTR_MTIME; + host_err = vfs_truncate(&path, iap->ia_size); + if (host_err) + goto out_host_err; + + iap->ia_valid &= ~ATTR_SIZE; + if (!iap->ia_valid) + goto done; } iap->ia_valid |= ATTR_CTIME; - if (check_guard && guardtime != inode->i_ctime.tv_sec) { - err = nfserr_notsync; - goto out_put_write_access; - } - fh_lock(fhp); host_err = notify_change(dentry, iap, NULL); fh_unlock(fhp); - err = nfserrno(host_err); + if (host_err) + goto out_host_err; -out_put_write_access: - if (size_change) - put_write_access(inode); - if (!err) - err = nfserrno(commit_metadata(fhp)); -out: - return err; +done: + host_err = commit_metadata(fhp); +out_host_err: + return nfserrno(host_err); } #if defined(CONFIG_NFSD_V4)