MIME-Version: 1.0
References: <20230308080327.33906-1-chengen.du@canonical.com> <1B808CC0-49C6-4AF6-B7E2-0772A71DB490@redhat.com>
In-Reply-To: <1B808CC0-49C6-4AF6-B7E2-0772A71DB490@redhat.com>
From:   Chengen Du <chengen.du@canonical.com>
Date:   Fri, 10 Mar 2023 10:57:01 +0800
Message-ID: <CAPza5qe=e-CkG+j4NYzF24cv5ZDFbheOFfAi+LQ_OW0pZmfc9g@mail.gmail.com>
Subject: Re: [PATCH] NFS: Correct timing for assigning access cache timestamp
To:     trond.myklebust@hammerspace.com
Cc:     Benjamin Coddington <bcodding@redhat.com>, anna@kernel.org,
        linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=E2=80=AFPM Benjamin Coddington <bcodding@redha=
t.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 *i=
node,
> >               else
> >                       goto found;
> >       }
> > -     set->timestamp =3D 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, st=
ruct nfs_access_entry *set,
> >       cache->fsgid =3D cred->fsgid;
> >       cache->group_info =3D get_group_info(cred->group_info);
> >       cache->mask =3D set->mask;
> > +     cache->timestamp =3D 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>
>