Re: [PATCH] NFS: Add mount option 'nofasc'

[Chengen Du]

Hi,

May I ask if there are any concerns or opinions regarding the
introduction of the new mount option?
If there is a more suitable solution, we can discuss it, and I can
work on implementing it.

Best regards,
Chengen Du

On Wed, Apr 19, 2023 at 3:18 PM Daire Byrne <daire@xxxxxxxx> wrote:
>
> Just to say, I am personally for this or some other way to opt out of
> the increased ACCESS calls on select servers (e.g. ones with high
> latency or with lots of multi user logins).
>
> I see it as similar to "actimeo" and "nocto" options in allowing users
> to reduce "correctness" at their own risk if it helps with their
> workloads and they deem the risk worth it.
>
> I am currently reverting the original patches in our kernel for our
> nfs re-exporting workloads.
>
> Daire
>
>
> On Tue, 11 Apr 2023 at 04:03, Chengen Du <chengen.du@xxxxxxxxxxxxx> wrote:
> >
> > This mount option is used to skip clearing the file access cache
> > upon login. Some users or applications switch to other privileged
> > users via commands such as 'su' to operate on NFS-mounted folders.
> > In such cases, the privileged user's login time will be renewed,
> > and NFS ACCESS operations will need to be re-sent, potentially
> > leading to performance degradation.
> > In some production environments where the access cache can be
> > trusted due to stable group membership, this option could be
> > particularly useful.
> >
> > Signed-off-by: Chengen Du <chengen.du@xxxxxxxxxxxxx>
> > ---
> >  fs/nfs/dir.c              | 21 ++++++++++++---------
> >  fs/nfs/fs_context.c       |  8 ++++++++
> >  fs/nfs/super.c            |  1 +
> >  include/linux/nfs_fs_sb.h |  1 +
> >  4 files changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index 6fbcbb8d6587..9a6d86e2044e 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -2953,12 +2953,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 (;;) {
> > @@ -2968,15 +2970,15 @@ static u64 nfs_access_login_time(const struct task_struct *task,
> >                         break;
> >                 task = parent;
> >         }
> > -       ret = task->start_time;
> > +       login_time = 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 struct cred *cred, u32 *mask, bool may_block)
> >  {
> >         struct nfs_inode *nfsi = NFS_I(inode);
> > -       u64 login_time = nfs_access_login_time(current, cred);
> >         struct nfs_access_entry *cache;
> >         bool retry = true;
> >         int err;
> > @@ -3005,7 +3007,8 @@ static int nfs_access_get_cached_locked(struct inode *inode, const struct cred *
> >                 retry = false;
> >         }
> >         err = -ENOENT;
> > -       if ((s64)(login_time - cache->timestamp) > 0)
> > +       if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NOFASC) &&
> > +           nfs_check_access_stale(current, cred, cache))
> >                 goto out;
> >         *mask = cache->mask;
> >         list_move_tail(&cache->lru, &nfsi->access_cache_entry_lru);
> > @@ -3025,7 +3028,6 @@ static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cre
> >          * but do it without locking.
> >          */
> >         struct nfs_inode *nfsi = NFS_I(inode);
> > -       u64 login_time = nfs_access_login_time(current, cred);
> >         struct nfs_access_entry *cache;
> >         int err = -ECHILD;
> >         struct list_head *lh;
> > @@ -3040,7 +3042,8 @@ static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cre
> >                 cache = NULL;
> >         if (cache == NULL)
> >                 goto out;
> > -       if ((s64)(login_time - cache->timestamp) > 0)
> > +       if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NOFASC) &&
> > +           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..5cd9b2882c79 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_parameters[] = {
> >         fsparam_string("vers",          Opt_vers),
> >         fsparam_enum  ("write",         Opt_write, nfs_param_enums_write),
> >         fsparam_u32   ("wsize",         Opt_wsize),
> > +       fsparam_flag_no("fasc",         Opt_fasc),
> >         {}
> >  };
> >
> > @@ -861,6 +863,12 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
> >         case Opt_sloppy:
> >                 ctx->sloppy = true;
> >                 break;
> > +       case Opt_fasc:
> > +               if (result.negated)
> > +                       ctx->flags |= NFS_MOUNT_NOFASC;
> > +               else
> > +                       ctx->flags &= ~NFS_MOUNT_NOFASC;
> > +               break;
> >         }
> >
> >         return 0;
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index 05ae23657527..059513d403f8 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_NOFASC, ",nofasc", "" },
> >                 { 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..a39ae1bd2217 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_NOFASC               0x08000000
> >
> >         unsigned int            fattr_valid;    /* Valid attributes */
> >         unsigned int            caps;           /* server capabilities */
> > --
> > 2.37.2
> >