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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> On Mar 8, 2023, at 03:03, Chengen Du <[email protected]> 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 <[email protected]>
> ---
> 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
[email protected]
On Tue, Mar 14, 2023 at 2:51 PM Trond Myklebust <[email protected]> wrote:
>
>
>
> > On Mar 8, 2023, at 03:03, Chengen Du <[email protected]> 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 <[email protected]>
> > ---
> > 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
> [email protected]
>