Message ID | 20230518114742.128950-5-jlayton@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp447675vqo; Thu, 18 May 2023 05:20:11 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5EfI65lcZew2MOjoIDX6JmxY7ou/NRDSNQK0NSoo7NhW7HxT7NgaELdqc0Cutj6UkAO001 X-Received: by 2002:a17:90b:4ac6:b0:253:34da:480 with SMTP id mh6-20020a17090b4ac600b0025334da0480mr2240379pjb.31.1684412411400; Thu, 18 May 2023 05:20:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684412411; cv=none; d=google.com; s=arc-20160816; b=PWqrP1AVucNj/au9nwf0f8xnQ2eQ+uKX2TEABSai6KWZKIGq5O/IW/eSVIc8nSLVRG 9/LpAbR0QghOgrErOViYXheEFXONQJXf9S1pMuzOB5Edter2HI1E7ZK3I0osE/+JdUMT 1NVhJv+V6basGw0dZtyEx3KQGa7yXCHkrXUr/KP0s4qcqZi46tFRMf2uBZCCIeU6kqGk WvbeoVCeGN/GrMGUz6D+teVV/oCS0UEcIdvzn2t7pGdaZ1M5J+1l3ZYbs9PvtPdyxj40 waekT9Ji88iEgdXl2+yhk8yIWTLwI5ZYP115lt78CDzKTE+uOusTxGefRaH4c8qD2Qf/ DUSQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=E45ftUt+La85JZW631KM+z9pPgcCPE3ssg5Uuu6Ne3s=; b=Pi6uX/RwEoCfVcWShXtb6GSXWFIAJmzoZzA4uiJFxUhnhbeNCU/AvnrK+QZSc15U62 vG/tykYOkfH2kpLepHwqNgK/PYu3WmYlMsmO+s9VuRjwk9D06fJ9OcOZ2us61lUlLJRh 8IKyoadZPj5oj+qFJyw3Fyb8jlq6rgxXPaoN28nUhgwtYiRQFnxUXJRa7qo9HHLMISXD gFzm7A2nmqkK7JSSnYtVCOt25vVWQUDr94/OH4o8VI5SE+w5aw+UuJVXcb+yQC19W18q HQx/tPMaF2pnBs3adaXMN4LzB4CWXoXRRwYAYqoLp7MSXnB33AIoJan+NdFT1nKJbPZ4 0wQw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=E4LAb0nI; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bt19-20020a17090af01300b002502c10bdb4si1485644pjb.91.2023.05.18.05.19.58; Thu, 18 May 2023 05:20:11 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=E4LAb0nI; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231615AbjERLs2 (ORCPT <rfc822;cscallsign@gmail.com> + 99 others); Thu, 18 May 2023 07:48:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42198 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231466AbjERLsQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 18 May 2023 07:48:16 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 500E419A; Thu, 18 May 2023 04:47:58 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id C8C9664ED4; Thu, 18 May 2023 11:47:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D0DD8C433A0; Thu, 18 May 2023 11:47:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1684410477; bh=en20+kwYIb+rQx+Bs8McKeHFRgrQ8XEHcOk9MVmU7iY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=E4LAb0nIr9avuLF6bnhNgC2cRv8n+Y1r1x0zPOZeGu+bmo0SCl5MMh4ksQB9eom5S CEPIxNX0JYB+XiFpj7TAYcNwACJZ0aG2sE7DrRd8kZiFZMB+MkW7P2Gk7W1du57+Tx BMsp9pM/wLI2kWZmup7SdZvctIT91uMob/wvifDVVh88XCAtTac7bHJkRuxHrr8ZN2 jrlGYU74z7oZ6tQP5ksyrgyn3awO7cIF7AtozH7MMlARxYEikZaguKB2DTP6z6xi6M Wm8fgVO2JxxmGPrUz5pDiYBZcTOycshUgvJF3oY9yDGZ5hzJhsgwGrWv6Kl3g/82aL D7C397+DhZIWQ== From: Jeff Layton <jlayton@kernel.org> To: Alexander Viro <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>, "Darrick J. Wong" <djwong@kernel.org>, Hugh Dickins <hughd@google.com>, Andrew Morton <akpm@linux-foundation.org>, Dave Chinner <david@fromorbit.com>, Chuck Lever <chuck.lever@oracle.com> Cc: Jan Kara <jack@suse.cz>, Amir Goldstein <amir73il@gmail.com>, David Howells <dhowells@redhat.com>, Neil Brown <neilb@suse.de>, Matthew Wilcox <willy@infradead.org>, Andreas Dilger <adilger.kernel@dilger.ca>, Theodore T'so <tytso@mit.edu>, Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>, David Sterba <dsterba@suse.com>, Namjae Jeon <linkinjeon@kernel.org>, Steve French <sfrench@samba.org>, Sergey Senozhatsky <senozhatsky@chromium.org>, Tom Talpey <tom@talpey.com>, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-mm@kvack.org, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org Subject: [PATCH v4 4/9] nfsd: ensure we use ctime_peek to grab the inode->i_ctime Date: Thu, 18 May 2023 07:47:37 -0400 Message-Id: <20230518114742.128950-5-jlayton@kernel.org> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230518114742.128950-1-jlayton@kernel.org> References: <20230518114742.128950-1-jlayton@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766234428636085445?= X-GMAIL-MSGID: =?utf-8?q?1766234428636085445?= |
Series |
fs: implement multigrain timestamps
|
|
Commit Message
Jeff Layton
May 18, 2023, 11:47 a.m. UTC
If getattr fails, then nfsd can end up scraping the time values directly
out of the inode for pre and post-op attrs. This may or may not be the
right thing to do, but for now make it at least use ctime_peek in this
situation to ensure that the QUERIED flag is masked.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
fs/nfsd/nfsfh.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Comments
> On May 18, 2023, at 7:47 AM, Jeff Layton <jlayton@kernel.org> wrote: > > If getattr fails, then nfsd can end up scraping the time values directly > out of the inode for pre and post-op attrs. This may or may not be the > right thing to do, but for now make it at least use ctime_peek in this > situation to ensure that the QUERIED flag is masked. That code comes from: commit 39ca1bf624b6b82cc895b0217889eaaf572a7913 Author: Amir Goldstein <amir73il@gmail.com> AuthorDate: Wed Jan 3 17:14:35 2018 +0200 Commit: J. Bruce Fields <bfields@redhat.com> CommitDate: Thu Feb 8 13:40:17 2018 -0500 nfsd: store stat times in fill_pre_wcc() instead of inode times The time values in stat and inode may differ for overlayfs and stat time values are the correct ones to use. This is also consistent with the fact that fill_post_wcc() also stores stat time values. This means introducing a stat call that could fail, where previously we were just copying values out of the inode. To be conservative about changing behavior, we fall back to copying values out of the inode in the error case. It might be better just to clear fh_pre_saved (though note the BUG_ON in set_change_info). Signed-off-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> I was thinking it might have been added to handle odd corner cases around re-exporting NFS mounts, but that does not seem to be the case. The fh_getattr() can fail for legitimate reasons -- like the file is in the middle of being deleted or renamed over -- I would think. This code should really deal with that by not adding pre-op attrs, since they are optional. > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfsfh.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index ccd8485fee04..f053cf20dd8a 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -624,9 +624,14 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > inode = d_inode(fhp->fh_dentry); > err = fh_getattr(fhp, &stat); > if (err) { > - /* Grab the times from inode anyway */ > + /* > + * Grab the times from inode anyway. > + * > + * FIXME: is this the right thing to do? Or should we just > + * not send pre and post-op attrs in this case? > + */ > stat.mtime = inode->i_mtime; > - stat.ctime = inode->i_ctime; > + stat.ctime = ctime_peek(inode); > stat.size = inode->i_size; > if (v4 && IS_I_VERSION(inode)) { > stat.change_cookie = inode_query_iversion(inode); > @@ -662,7 +667,7 @@ void fh_fill_post_attrs(struct svc_fh *fhp) > err = fh_getattr(fhp, &fhp->fh_post_attr); > if (err) { > fhp->fh_post_saved = false; > - fhp->fh_post_attr.ctime = inode->i_ctime; > + fhp->fh_post_attr.ctime = ctime_peek(inode); > if (v4 && IS_I_VERSION(inode)) { > fhp->fh_post_attr.change_cookie = inode_query_iversion(inode); > fhp->fh_post_attr.result_mask |= STATX_CHANGE_COOKIE; > -- > 2.40.1 > -- Chuck Lever
On Thu, 2023-05-18 at 13:43 +0000, Chuck Lever III wrote: > > > On May 18, 2023, at 7:47 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > If getattr fails, then nfsd can end up scraping the time values directly > > out of the inode for pre and post-op attrs. This may or may not be the > > right thing to do, but for now make it at least use ctime_peek in this > > situation to ensure that the QUERIED flag is masked. > > That code comes from: > > commit 39ca1bf624b6b82cc895b0217889eaaf572a7913 > Author: Amir Goldstein <amir73il@gmail.com> > AuthorDate: Wed Jan 3 17:14:35 2018 +0200 > Commit: J. Bruce Fields <bfields@redhat.com> > CommitDate: Thu Feb 8 13:40:17 2018 -0500 > > nfsd: store stat times in fill_pre_wcc() instead of inode times > > The time values in stat and inode may differ for overlayfs and stat time > values are the correct ones to use. This is also consistent with the fact > that fill_post_wcc() also stores stat time values. > > This means introducing a stat call that could fail, where previously we > were just copying values out of the inode. To be conservative about > changing behavior, we fall back to copying values out of the inode in > the error case. It might be better just to clear fh_pre_saved (though > note the BUG_ON in set_change_info). > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > I was thinking it might have been added to handle odd corner > cases around re-exporting NFS mounts, but that does not seem > to be the case. > > The fh_getattr() can fail for legitimate reasons -- like the > file is in the middle of being deleted or renamed over -- I > would think. This code should really deal with that by not > adding pre-op attrs, since they are optional. > That sounds fine to me. I'll plan to drop this patch from the series and I'll send a separate patch to just remove those branches altogether (which should DTRT).
On Thu, May 18, 2023 at 11:31:45AM -0400, Jeff Layton wrote: > On Thu, 2023-05-18 at 13:43 +0000, Chuck Lever III wrote: > > > > > On May 18, 2023, at 7:47 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > If getattr fails, then nfsd can end up scraping the time values directly > > > out of the inode for pre and post-op attrs. This may or may not be the > > > right thing to do, but for now make it at least use ctime_peek in this > > > situation to ensure that the QUERIED flag is masked. > > > > That code comes from: > > > > commit 39ca1bf624b6b82cc895b0217889eaaf572a7913 > > Author: Amir Goldstein <amir73il@gmail.com> > > AuthorDate: Wed Jan 3 17:14:35 2018 +0200 > > Commit: J. Bruce Fields <bfields@redhat.com> > > CommitDate: Thu Feb 8 13:40:17 2018 -0500 > > > > nfsd: store stat times in fill_pre_wcc() instead of inode times > > > > The time values in stat and inode may differ for overlayfs and stat time > > values are the correct ones to use. This is also consistent with the fact > > that fill_post_wcc() also stores stat time values. > > > > This means introducing a stat call that could fail, where previously we > > were just copying values out of the inode. To be conservative about > > changing behavior, we fall back to copying values out of the inode in > > the error case. It might be better just to clear fh_pre_saved (though > > note the BUG_ON in set_change_info). > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > > I was thinking it might have been added to handle odd corner > > cases around re-exporting NFS mounts, but that does not seem > > to be the case. > > > > The fh_getattr() can fail for legitimate reasons -- like the > > file is in the middle of being deleted or renamed over -- I > > would think. This code should really deal with that by not > > adding pre-op attrs, since they are optional. > > > > That sounds fine to me. I'll plan to drop this patch from the series and > I'll send a separate patch to just remove those branches altogether > (which should DTRT). I'll wait with reviewing this until you send the next version then.
On Fri, 2023-05-19 at 12:36 +0200, Christian Brauner wrote: > On Thu, May 18, 2023 at 11:31:45AM -0400, Jeff Layton wrote: > > On Thu, 2023-05-18 at 13:43 +0000, Chuck Lever III wrote: > > > > > > > On May 18, 2023, at 7:47 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > If getattr fails, then nfsd can end up scraping the time values directly > > > > out of the inode for pre and post-op attrs. This may or may not be the > > > > right thing to do, but for now make it at least use ctime_peek in this > > > > situation to ensure that the QUERIED flag is masked. > > > > > > That code comes from: > > > > > > commit 39ca1bf624b6b82cc895b0217889eaaf572a7913 > > > Author: Amir Goldstein <amir73il@gmail.com> > > > AuthorDate: Wed Jan 3 17:14:35 2018 +0200 > > > Commit: J. Bruce Fields <bfields@redhat.com> > > > CommitDate: Thu Feb 8 13:40:17 2018 -0500 > > > > > > nfsd: store stat times in fill_pre_wcc() instead of inode times > > > > > > The time values in stat and inode may differ for overlayfs and stat time > > > values are the correct ones to use. This is also consistent with the fact > > > that fill_post_wcc() also stores stat time values. > > > > > > This means introducing a stat call that could fail, where previously we > > > were just copying values out of the inode. To be conservative about > > > changing behavior, we fall back to copying values out of the inode in > > > the error case. It might be better just to clear fh_pre_saved (though > > > note the BUG_ON in set_change_info). > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > > > > I was thinking it might have been added to handle odd corner > > > cases around re-exporting NFS mounts, but that does not seem > > > to be the case. > > > > > > The fh_getattr() can fail for legitimate reasons -- like the > > > file is in the middle of being deleted or renamed over -- I > > > would think. This code should really deal with that by not > > > adding pre-op attrs, since they are optional. > > > > > > > That sounds fine to me. I'll plan to drop this patch from the series and > > I'll send a separate patch to just remove those branches altogether > > (which should DTRT). > > I'll wait with reviewing this until you send the next version then. I don't have any other big changes queued up. So far, this would just be the exact same set, without this patch. FWIW, I'm mostly interested in your review of patches #1 and 2. Is altering prototype for generic_fillattr, and changing the logic in current_time the right approach here?
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index ccd8485fee04..f053cf20dd8a 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -624,9 +624,14 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) inode = d_inode(fhp->fh_dentry); err = fh_getattr(fhp, &stat); if (err) { - /* Grab the times from inode anyway */ + /* + * Grab the times from inode anyway. + * + * FIXME: is this the right thing to do? Or should we just + * not send pre and post-op attrs in this case? + */ stat.mtime = inode->i_mtime; - stat.ctime = inode->i_ctime; + stat.ctime = ctime_peek(inode); stat.size = inode->i_size; if (v4 && IS_I_VERSION(inode)) { stat.change_cookie = inode_query_iversion(inode); @@ -662,7 +667,7 @@ void fh_fill_post_attrs(struct svc_fh *fhp) err = fh_getattr(fhp, &fhp->fh_post_attr); if (err) { fhp->fh_post_saved = false; - fhp->fh_post_attr.ctime = inode->i_ctime; + fhp->fh_post_attr.ctime = ctime_peek(inode); if (v4 && IS_I_VERSION(inode)) { fhp->fh_post_attr.change_cookie = inode_query_iversion(inode); fhp->fh_post_attr.result_mask |= STATX_CHANGE_COOKIE;