> On May 18, 2023, at 7:47 AM, Jeff Layton <[email protected]> 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 <[email protected]>
AuthorDate: Wed Jan 3 17:14:35 2018 +0200
Commit: J. Bruce Fields <[email protected]>
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 <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> AuthorDate: Wed Jan 3 17:14:35 2018 +0200
> Commit: J. Bruce Fields <[email protected]>
> 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 <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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).
--
Jeff Layton <[email protected]>
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 <[email protected]> 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 <[email protected]>
> > AuthorDate: Wed Jan 3 17:14:35 2018 +0200
> > Commit: J. Bruce Fields <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: J. Bruce Fields <[email protected]>
> >
> > 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 <[email protected]> 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 <[email protected]>
> > > AuthorDate: Wed Jan 3 17:14:35 2018 +0200
> > > Commit: J. Bruce Fields <[email protected]>
> > > 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 <[email protected]>
> > > Signed-off-by: J. Bruce Fields <[email protected]>
> > >
> > > 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?
--
Jeff Layton <[email protected]>