[PATCH] NFS: Correct timing for assigning access cache timestamp

[PATCH] NFS: Correct timing for assigning access cache timestamp

[Chengen Du]

When the user's login time is newer than the cache's timestamp,
the original entry in the RB-tree will be replaced by a new entry.
Currently, the timestamp is only set if the entry is not found in
the RB-tree, which can cause the timestamp to be undefined when
the entry exists. This may result in a significant increase in
ACCESS operations if the timestamp is set to zero.

Signed-off-by: Chengen Du <chengen.du@canonical.com>
---
 fs/nfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index a41c3ee4549c..6fbcbb8d6587 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -3089,7 +3089,6 @@ static void nfs_access_add_rbtree(struct inode *inode,
 		else
 			goto found;
 	}
-	set->timestamp = ktime_get_ns();
 	rb_link_node(&set->rb_node, parent, p);
 	rb_insert_color(&set->rb_node, root_node);
 	list_add_tail(&set->lru, &nfsi->access_cache_entry_lru);
@@ -3114,6 +3113,7 @@ void nfs_access_add_cache(struct inode *inode, struct nfs_access_entry *set,
 	cache->fsgid = cred->fsgid;
 	cache->group_info = get_group_info(cred->group_info);
 	cache->mask = set->mask;
+	cache->timestamp = ktime_get_ns();
 
 	/* The above field assignments must be visible
 	 * before this item appears on the lru.  We cannot easily
-- 
2.37.2

Re: [PATCH] NFS: Correct timing for assigning access cache timestamp

[Benjamin Coddington]

On 8 Mar 2023, at 3:03, Chengen Du wrote:

> When the user's login time is newer than the cache's timestamp,
> the original entry in the RB-tree will be replaced by a new entry.
> Currently, the timestamp is only set if the entry is not found in
> the RB-tree, which can cause the timestamp to be undefined when
> the entry exists. This may result in a significant increase in
> ACCESS operations if the timestamp is set to zero.
>
> Signed-off-by: Chengen Du <chengen.du@canonical.com>
> ---
>  fs/nfs/dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index a41c3ee4549c..6fbcbb8d6587 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -3089,7 +3089,6 @@ static void nfs_access_add_rbtree(struct inode *inode,
>  		else
>  			goto found;
>  	}
> -	set->timestamp = ktime_get_ns();
>  	rb_link_node(&set->rb_node, parent, p);
>  	rb_insert_color(&set->rb_node, root_node);
>  	list_add_tail(&set->lru, &nfsi->access_cache_entry_lru);
> @@ -3114,6 +3113,7 @@ void nfs_access_add_cache(struct inode *inode, struct nfs_access_entry *set,
>  	cache->fsgid = cred->fsgid;
>  	cache->group_info = get_group_info(cred->group_info);
>  	cache->mask = set->mask;
> +	cache->timestamp = ktime_get_ns();
>
>  	/* The above field assignments must be visible
>  	 * before this item appears on the lru.  We cannot easily
> -- 
> 2.37.2

Looks good to me.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Re: [PATCH] NFS: Correct timing for assigning access cache timestamp

[Chengen Du]

Hi,

We would like to inform you that we have utilized the patch to create
a test kernel,
which has been made available for community testing.
We are pleased to report that the feedback received thus far indicates
that the patch has successfully resolved the issue at hand.

Please refer to the following link for further details on the related
bug report:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2009325

Thank you for your attention to this matter.

Best regards,
Chengen Du

On Wed, Mar 8, 2023 at 10:45 PM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 8 Mar 2023, at 3:03, Chengen Du wrote:
>
> > When the user's login time is newer than the cache's timestamp,
> > the original entry in the RB-tree will be replaced by a new entry.
> > Currently, the timestamp is only set if the entry is not found in
> > the RB-tree, which can cause the timestamp to be undefined when
> > the entry exists. This may result in a significant increase in
> > ACCESS operations if the timestamp is set to zero.
> >
> > Signed-off-by: Chengen Du <chengen.du@canonical.com>
> > ---
> >  fs/nfs/dir.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index a41c3ee4549c..6fbcbb8d6587 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -3089,7 +3089,6 @@ static void nfs_access_add_rbtree(struct inode *inode,
> >               else
> >                       goto found;
> >       }
> > -     set->timestamp = ktime_get_ns();
> >       rb_link_node(&set->rb_node, parent, p);
> >       rb_insert_color(&set->rb_node, root_node);
> >       list_add_tail(&set->lru, &nfsi->access_cache_entry_lru);
> > @@ -3114,6 +3113,7 @@ void nfs_access_add_cache(struct inode *inode, struct nfs_access_entry *set,
> >       cache->fsgid = cred->fsgid;
> >       cache->group_info = get_group_info(cred->group_info);
> >       cache->mask = set->mask;
> > +     cache->timestamp = ktime_get_ns();
> >
> >       /* The above field assignments must be visible
> >        * before this item appears on the lru.  We cannot easily
> > --
> > 2.37.2
>
> Looks good to me.
>
> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
>

Re: [PATCH] NFS: Correct timing for assigning access cache timestamp

[Trond Myklebust]

> On Mar 8, 2023, at 03:03, Chengen Du <chengen.du@canonical.com> wrote:
> 
> When the user's login time is newer than the cache's timestamp,
> the original entry in the RB-tree will be replaced by a new entry.
> Currently, the timestamp is only set if the entry is not found in
> the RB-tree, which can cause the timestamp to be undefined when
> the entry exists. This may result in a significant increase in
> ACCESS operations if the timestamp is set to zero.
> 
> Signed-off-by: Chengen Du <chengen.du@canonical.com>
> ---
> fs/nfs/dir.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index a41c3ee4549c..6fbcbb8d6587 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -3089,7 +3089,6 @@ static void nfs_access_add_rbtree(struct inode *inode,
> else
> goto found;
> }
> - set->timestamp = ktime_get_ns();
> rb_link_node(&set->rb_node, parent, p);
> rb_insert_color(&set->rb_node, root_node);
> list_add_tail(&set->lru, &nfsi->access_cache_entry_lru);
> @@ -3114,6 +3113,7 @@ void nfs_access_add_cache(struct inode *inode, struct nfs_access_entry *set,
> cache->fsgid = cred->fsgid;
> cache->group_info = get_group_info(cred->group_info);
> cache->mask = set->mask;
> + cache->timestamp = ktime_get_ns();
> 
> /* The above field assignments must be visible
> * before this item appears on the lru.  We cannot easily
> -- 
> 2.37.2
> 

Doh! Nice catch…

Anna, please pick this up for 6.2-rcX and add an appropriate

Fixes: 0eb43812c027 ("NFS: Clear the file access cache upon login”)

Thanks!
  Trond

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

Re: [PATCH] NFS: Correct timing for assigning access cache timestamp

[Anna Schumaker]

On Tue, Mar 14, 2023 at 2:51 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
>
>
> > On Mar 8, 2023, at 03:03, Chengen Du <chengen.du@canonical.com> wrote:
> >
> > When the user's login time is newer than the cache's timestamp,
> > the original entry in the RB-tree will be replaced by a new entry.
> > Currently, the timestamp is only set if the entry is not found in
> > the RB-tree, which can cause the timestamp to be undefined when
> > the entry exists. This may result in a significant increase in
> > ACCESS operations if the timestamp is set to zero.
> >
> > Signed-off-by: Chengen Du <chengen.du@canonical.com>
> > ---
> > fs/nfs/dir.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index a41c3ee4549c..6fbcbb8d6587 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -3089,7 +3089,6 @@ static void nfs_access_add_rbtree(struct inode *inode,
> > else
> > goto found;
> > }
> > - set->timestamp = ktime_get_ns();
> > rb_link_node(&set->rb_node, parent, p);
> > rb_insert_color(&set->rb_node, root_node);
> > list_add_tail(&set->lru, &nfsi->access_cache_entry_lru);
> > @@ -3114,6 +3113,7 @@ void nfs_access_add_cache(struct inode *inode, struct nfs_access_entry *set,
> > cache->fsgid = cred->fsgid;
> > cache->group_info = get_group_info(cred->group_info);
> > cache->mask = set->mask;
> > + cache->timestamp = ktime_get_ns();
> >
> > /* The above field assignments must be visible
> > * before this item appears on the lru.  We cannot easily
> > --
> > 2.37.2
> >
>
> Doh! Nice catch…
>
> Anna, please pick this up for 6.2-rcX and add an appropriate
>
> Fixes: 0eb43812c027 ("NFS: Clear the file access cache upon login”)

Will do!

Anna
>
> Thanks!
>   Trond
>
> _________________________________
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>