Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230602020935.43367-1-chengen.du@canonical.com>
In-Reply-To: <20230602020935.43367-1-chengen.du@canonical.com>
From:   Chengen Du <chengen.du@canonical.com>
Date:   Mon, 26 Jun 2023 18:25:17 +0800
Message-ID: <CAPza5qdv7sS4d5f7nKKJgpEnxh41ZpuLwNp10=Zf2q_SMf23fw@mail.gmail.com>
Subject: Re: [PATCH v2 RESEND] NFS: Add mount option 'fasc'
To:     trond.myklebust@hammerspace.com, anna@kernel.org
Cc:     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 Trond, Anna,

I apologize for interrupting you, as I understand you may be engaged
in other important tasks.

In order to enhance the flexibility of deploying the NFS service,
I believe it would be beneficial to introduce a mount option that
enables file access stale checking for users.

There are certain users experiencing performance degradation in
specific usage scenarios.
It would be greatly appreciated if you could share some insights on
how to address this issue.
If you have the time, I would greatly appreciate the opportunity to
discuss this matter further.

Thank you in advance for your attention to this request.

Best regards,
Chengen Du

On Fri, Jun 2, 2023 at 10:09=E2=80=AFAM Chengen Du <chengen.du@canonical.co=
m> wrote:
>
> In certain instances, users or applications switch to other privileged
> users by executing commands like 'su' to carry out operations on NFS-
> mounted folders. However, when this happens, the login time for the
> privileged user is reset, and any NFS ACCESS operations must be resent,
> which can result in a decrease in performance. In specific production
> environments where the access cache can be trusted due to stable group
> membership, there's no need to verify the cache stall situation.
> To maintain the initial behavior and performance, a new mount option
> called 'fasc' has been introduced. This option triggers the mechanism
> of clearing the file access cache upon login.
>
> Signed-off-by: Chengen Du <chengen.du@canonical.com>
> ---
>  fs/nfs/dir.c              | 21 ++++++++++++---------
>  fs/nfs/fs_context.c       |  5 +++++
>  fs/nfs/super.c            |  1 +
>  include/linux/nfs_fs_sb.h |  1 +
>  4 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 8f3112e71a6a..cefdb23d4cd7 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -2951,12 +2951,14 @@ static struct nfs_access_entry *nfs_access_search=
_rbtree(struct inode *inode, co
>         return NULL;
>  }
>
> -static u64 nfs_access_login_time(const struct task_struct *task,
> -                                const struct cred *cred)
> +static inline
> +bool nfs_check_access_stale(const struct task_struct *task,
> +                           const struct cred *cred,
> +                           const struct nfs_access_entry *cache)
>  {
>         const struct task_struct *parent;
>         const struct cred *pcred;
> -       u64 ret;
> +       u64 login_time;
>
>         rcu_read_lock();
>         for (;;) {
> @@ -2966,15 +2968,15 @@ static u64 nfs_access_login_time(const struct tas=
k_struct *task,
>                         break;
>                 task =3D parent;
>         }
> -       ret =3D task->start_time;
> +       login_time =3D task->start_time;
>         rcu_read_unlock();
> -       return ret;
> +
> +       return ((s64)(login_time - cache->timestamp) > 0);
>  }
>
>  static int nfs_access_get_cached_locked(struct inode *inode, const struc=
t cred *cred, u32 *mask, bool may_block)
>  {
>         struct nfs_inode *nfsi =3D NFS_I(inode);
> -       u64 login_time =3D nfs_access_login_time(current, cred);
>         struct nfs_access_entry *cache;
>         bool retry =3D true;
>         int err;
> @@ -3003,7 +3005,8 @@ static int nfs_access_get_cached_locked(struct inod=
e *inode, const struct cred *
>                 retry =3D false;
>         }
>         err =3D -ENOENT;
> -       if ((s64)(login_time - cache->timestamp) > 0)
> +       if ((NFS_SERVER(inode)->flags & NFS_MOUNT_FASC) &&
> +           nfs_check_access_stale(current, cred, cache))
>                 goto out;
>         *mask =3D cache->mask;
>         list_move_tail(&cache->lru, &nfsi->access_cache_entry_lru);
> @@ -3023,7 +3026,6 @@ static int nfs_access_get_cached_rcu(struct inode *=
inode, const struct cred *cre
>          * but do it without locking.
>          */
>         struct nfs_inode *nfsi =3D NFS_I(inode);
> -       u64 login_time =3D nfs_access_login_time(current, cred);
>         struct nfs_access_entry *cache;
>         int err =3D -ECHILD;
>         struct list_head *lh;
> @@ -3038,7 +3040,8 @@ static int nfs_access_get_cached_rcu(struct inode *=
inode, const struct cred *cre
>                 cache =3D NULL;
>         if (cache =3D=3D NULL)
>                 goto out;
> -       if ((s64)(login_time - cache->timestamp) > 0)
> +       if ((NFS_SERVER(inode)->flags & NFS_MOUNT_FASC) &&
> +           nfs_check_access_stale(current, cred, cache))
>                 goto out;
>         if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_ACCESS))
>                 goto out;
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 9bcd53d5c7d4..0a14bd67efc1 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -88,6 +88,7 @@ enum nfs_param {
>         Opt_vers,
>         Opt_wsize,
>         Opt_write,
> +       Opt_fasc,
>  };
>
>  enum {
> @@ -194,6 +195,7 @@ static const struct fs_parameter_spec nfs_fs_paramete=
rs[] =3D {
>         fsparam_string("vers",          Opt_vers),
>         fsparam_enum  ("write",         Opt_write, nfs_param_enums_write)=
,
>         fsparam_u32   ("wsize",         Opt_wsize),
> +       fsparam_flag  ("fasc",          Opt_fasc),
>         {}
>  };
>
> @@ -861,6 +863,9 @@ static int nfs_fs_context_parse_param(struct fs_conte=
xt *fc,
>         case Opt_sloppy:
>                 ctx->sloppy =3D true;
>                 break;
> +       case Opt_fasc:
> +               ctx->flags |=3D NFS_MOUNT_FASC;
> +               break;
>         }
>
>         return 0;
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 30e53e93049e..e8d0ffd04b16 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -444,6 +444,7 @@ static void nfs_show_mount_options(struct seq_file *m=
, struct nfs_server *nfss,
>                 { NFS_MOUNT_NORDIRPLUS, ",nordirplus", "" },
>                 { NFS_MOUNT_UNSHARED, ",nosharecache", "" },
>                 { NFS_MOUNT_NORESVPORT, ",noresvport", "" },
> +               { NFS_MOUNT_FASC, ",fasc", "" },
>                 { 0, NULL, NULL }
>         };
>         const struct proc_nfs_info *nfs_infop;
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index ea2f7e6b1b0b..332ceb11be6c 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -153,6 +153,7 @@ struct nfs_server {
>  #define NFS_MOUNT_WRITE_EAGER          0x01000000
>  #define NFS_MOUNT_WRITE_WAIT           0x02000000
>  #define NFS_MOUNT_TRUNK_DISCOVERY      0x04000000
> +#define NFS_MOUNT_FASC                 0x08000000
>
>         unsigned int            fattr_valid;    /* Valid attributes */
>         unsigned int            caps;           /* server capabilities */
> --
> 2.39.2
>