Re: [PATCH 12/19] NFSv4: Fix up delegated attributes in nfs_setattr

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2024-06-14 at 12:32 -0400, Anna Schumaker wrote:
> Hi Trond,
> 
> On Thu, Jun 13, 2024 at 12:18 AM <trondmy@gmail.com> wrote:
> > 
> > From: Trond Myklebust <trond.myklebust@primarydata.com>
> > 
> > nfs_setattr calls nfs_update_inode() directly, so we have to reset
> > the
> > m/ctime there.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/inode.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> 
> After applying this patch I see a handful of new failures on
> xfstests:
> generic/075, generic/086, generic/112, generic/332, generic/346,
> generic/647, and generic/729. I see the first five on NFS v4.2, but
> 647 and 729 both fail on v4.1 in addition to v4.2.

Thanks Anna!

Yes, I think that is a consequence of the changes that were made in
commit cc7f2dae63bc ("NFS: Don't store NFS_INO_REVAL_FORCED"). I can
just remove that "if (!(cache_validity & NFS_INO_REVAL_FORCED))"
altogether with that change applied.

Version 2 will be forthcoming.

> 
> I hope this helps!
> Anna
> 
> > 
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 91c0aeaf6c1e..e03c512c8535 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -605,6 +605,46 @@ nfs_fhget(struct super_block *sb, struct
> > nfs_fh *fh, struct nfs_fattr *fattr)
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_fhget);
> > 
> > +static void
> > +nfs_fattr_fixup_delegated(struct inode *inode, struct nfs_fattr
> > *fattr)
> > +{
> > +       unsigned long cache_validity = NFS_I(inode)-
> > >cache_validity;
> > +
> > +       if (!nfs_have_read_or_write_delegation(inode))
> > +               return;
> > +
> > +       if (!(cache_validity & NFS_INO_REVAL_FORCED))
> > +               cache_validity &= ~(NFS_INO_INVALID_ATIME
> > +                               | NFS_INO_INVALID_CHANGE
> > +                               | NFS_INO_INVALID_CTIME
> > +                               | NFS_INO_INVALID_MTIME
> > +                               | NFS_INO_INVALID_SIZE);
> > +
> > +       if (!(cache_validity & NFS_INO_INVALID_SIZE))
> > +               fattr->valid &= ~(NFS_ATTR_FATTR_PRESIZE
> > +                               | NFS_ATTR_FATTR_SIZE);
> > +
> > +       if (!(cache_validity & NFS_INO_INVALID_CHANGE))
> > +               fattr->valid &= ~(NFS_ATTR_FATTR_PRECHANGE
> > +                               | NFS_ATTR_FATTR_CHANGE);
> > +
> > +       if (nfs_have_delegated_mtime(inode)) {
> > +               if (!(cache_validity & NFS_INO_INVALID_CTIME))
> > +                       fattr->valid &= ~(NFS_ATTR_FATTR_PRECTIME
> > +                                       | NFS_ATTR_FATTR_CTIME);
> > +
> > +               if (!(cache_validity & NFS_INO_INVALID_MTIME))
> > +                       fattr->valid &= ~(NFS_ATTR_FATTR_PREMTIME
> > +                                       | NFS_ATTR_FATTR_MTIME);
> > +
> > +               if (!(cache_validity & NFS_INO_INVALID_ATIME))
> > +                       fattr->valid &= ~NFS_ATTR_FATTR_ATIME;
> > +       } else if (nfs_have_delegated_atime(inode)) {
> > +               if (!(cache_validity & NFS_INO_INVALID_ATIME))
> > +                       fattr->valid &= ~NFS_ATTR_FATTR_ATIME;
> > +       }
> > +}
> > +
> >  void nfs_update_delegated_atime(struct inode *inode)
> >  {
> >         spin_lock(&inode->i_lock);
> > @@ -2163,6 +2203,9 @@ static int nfs_update_inode(struct inode
> > *inode, struct nfs_fattr *fattr)
> >          */
> >         nfsi->read_cache_jiffies = fattr->time_start;
> > 
> > +       /* Fix up any delegated attributes in the struct nfs_fattr
> > */
> > +       nfs_fattr_fixup_delegated(inode, fattr);
> > +
> >         save_cache_validity = nfsi->cache_validity;
> >         nfsi->cache_validity &= ~(NFS_INO_INVALID_ATTR
> >                         | NFS_INO_INVALID_ATIME
> > --
> > 2.45.2
> > 
> > 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com