diff mbox series

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

Message ID 20240613041136.506908-13-trond.myklebust@hammerspace.com (mailing list archive)
State New
Headers show
Series OPEN optimisations and Attribute delegations | expand

Commit Message

Trond Myklebust June 13, 2024, 4:11 a.m. UTC 
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(+)

Comments

Anna Schumaker June 14, 2024, 4:32 p.m. UTC | #1 
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.

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 June 14, 2024, 7:59 p.m. UTC | #2 
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 June 15, 2024, 12:25 a.m. UTC | #3 
On Fri, 2024-06-14 at 15:59 -0400, Trond Myklebust wrote:
> 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.

Sigh... Never mind. I discovered a discrepancy between what I had in
the topic branch for these features vs. what I had in our master
branch. At some point in the last few years, I must have updated the
latter without remembering to update the former.

> 
> Version 2 will be forthcoming.
> 

...and it will not affect the behaviour of the regular delegations.


> > 
> > I hope this helps!
> > Anna

Yes. I've checked both the patchsets that I sent out the other day, and
I believe this is the only patch which deviates from our master branch.

Again thanks!

> > 
> > > 
> > > 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
> > > 
> > > 
>
diff mbox series

Patch

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