* [PATCH] NFS: Add mount option 'nofasc' @ 2023-04-11 3:02 Chengen Du 2023-04-19 7:17 ` Daire Byrne 0 siblings, 1 reply; 7+ messages in thread From: Chengen Du @ 2023-04-11 3:02 UTC (permalink / raw) To: trond.myklebust; +Cc: anna, linux-nfs, linux-kernel, Chengen Du 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@canonical.com> --- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFS: Add mount option 'nofasc' 2023-04-11 3:02 [PATCH] NFS: Add mount option 'nofasc' Chengen Du @ 2023-04-19 7:17 ` Daire Byrne 2023-04-25 1:41 ` Chengen Du 0 siblings, 1 reply; 7+ messages in thread From: Daire Byrne @ 2023-04-19 7:17 UTC (permalink / raw) To: Chengen Du; +Cc: trond.myklebust, anna, linux-nfs, linux-kernel 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@canonical.com> 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@canonical.com> > --- > 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFS: Add mount option 'nofasc' 2023-04-19 7:17 ` Daire Byrne @ 2023-04-25 1:41 ` Chengen Du 2023-05-09 2:31 ` Chengen Du 2023-05-09 15:16 ` Benjamin Coddington 0 siblings, 2 replies; 7+ messages in thread From: Chengen Du @ 2023-04-25 1:41 UTC (permalink / raw) To: trond.myklebust; +Cc: anna, linux-nfs, linux-kernel 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@dneg.com> 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@canonical.com> 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@canonical.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFS: Add mount option 'nofasc' 2023-04-25 1:41 ` Chengen Du @ 2023-05-09 2:31 ` Chengen Du 2023-05-09 11:53 ` Jan Ingvoldstad 2023-05-09 15:16 ` Benjamin Coddington 1 sibling, 1 reply; 7+ messages in thread From: Chengen Du @ 2023-05-09 2:31 UTC (permalink / raw) To: trond.myklebust; +Cc: anna, linux-nfs, linux-kernel Hi Trond, I sincerely apologize for interrupting you, as I understand that you may have other pressing matters to attend to. However, I was hoping to bring your attention to a pressing matter that a number of community users are currently experiencing. As can be seen in this link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2015827, the issue at hand is causing a significant amount of inconvenience to many individuals. If it wouldn't be too much trouble, I would be immensely grateful if you could spare a moment to examine the patch and perhaps offer some guidance on how best to proceed. Your assistance in this matter would be greatly appreciated. Best regards, Chengen Du On Tue, Apr 25, 2023 at 9:41 AM Chengen Du <chengen.du@canonical.com> wrote: > > 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@dneg.com> 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@canonical.com> 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@canonical.com> > > > --- > > > 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFS: Add mount option 'nofasc' 2023-05-09 2:31 ` Chengen Du @ 2023-05-09 11:53 ` Jan Ingvoldstad 0 siblings, 0 replies; 7+ messages in thread From: Jan Ingvoldstad @ 2023-05-09 11:53 UTC (permalink / raw) To: Chengen Du, trond.myklebust; +Cc: anna, linux-nfs, linux-kernel Hi, The issues causing inconvenience could be easily resolved by Canonical/Ubuntu without involving this upstream list, but they are of concern for anyone trying to track non-stable Linux kernel releases as well. May I be so bold as to suggest that the feature in question be reversed to an opt-in instead of an opt-out? As an option "fasc" for having these features enabled, with "nofasc" the default. By making it opt-out, as suggested, or keeping the current patches in place, there is around a 50-100 fold resource requirement on the NFS server, which makes the Linux versions using these patches (from 6.2-rc3 onwards) completely unsuitable for NFS clients in a multi-user client-server environment. It amazes me that this keeps going on without resolution for well over two months now, and I request that this matter is bumped in priority. Cheers, Jan Den 09.05.2023 04:31, skrev Chengen Du: > Hi Trond, > > I sincerely apologize for interrupting you, as I understand that you > may have other pressing matters to attend to. > However, I was hoping to bring your attention to a pressing matter > that a number of community users are currently experiencing. > As can be seen in this link: > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2015827, > the issue at hand is causing a significant amount of inconvenience to > many individuals. > > If it wouldn't be too much trouble, I would be immensely grateful if > you could spare a moment to examine the patch and perhaps offer some > guidance on how best to proceed. > Your assistance in this matter would be greatly appreciated. > > Best regards, > Chengen Du > > On Tue, Apr 25, 2023 at 9:41 AM Chengen Du <chengen.du@canonical.com> wrote: >> 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@dneg.com> 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@canonical.com> 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@canonical.com> >>>> --- >>>> 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 >>>> -- Cheers, Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFS: Add mount option 'nofasc' 2023-04-25 1:41 ` Chengen Du 2023-05-09 2:31 ` Chengen Du @ 2023-05-09 15:16 ` Benjamin Coddington 2023-05-10 2:26 ` Chengen Du 1 sibling, 1 reply; 7+ messages in thread From: Benjamin Coddington @ 2023-05-09 15:16 UTC (permalink / raw) To: Chengen Du; +Cc: trond.myklebust, anna, linux-nfs, linux-kernel On 24 Apr 2023, at 21:41, Chengen Du wrote: > 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. I suspect there's some weariness of mount options, we have a lot of them and they are not easily removed once implemented. Additionally, requests to add them usually can show the appropriate changes to the nfs-utils mount.nfs and man pages required. Incompleteness here may be the reason you're not hearing back from a maintainer. However, without guidance from a maintainer, you might end up doing extra work trying to meet unclear standards. There's a couple of other ways to address the access cache performance "degradation" that was introduced by the changes that other folks desperately needed for correctness. We can change nfs_access_login_time to have a module parameter modifying the behavior. The downside is this would affect every mount. We can grow a sysfs knob to change the behavior. Downside is we only have very preliminary sysfs scaffolding as of yet. However, if you want to keep pushing for the mount option, I'd suggest doing a v2 with the userspace patches, and if that gets ignored then do a "PATCH RESEND" on that a month or so before each mainline merge window. I've found that bump-replying to old patches isn't as effective at getting work merged here. I believe the maintainers want to see that you're rebasing as mainline progresses, and you have active ownership over the work to fix bugs that may follow or address other fallout from the community. Ben ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] NFS: Add mount option 'nofasc' 2023-05-09 15:16 ` Benjamin Coddington @ 2023-05-10 2:26 ` Chengen Du 0 siblings, 0 replies; 7+ messages in thread From: Chengen Du @ 2023-05-10 2:26 UTC (permalink / raw) To: Benjamin Coddington; +Cc: trond.myklebust, anna, linux-nfs, linux-kernel Hi Benjamin, Thank you so much for your valuable advice. I truly appreciate your input and it has been very helpful for me. In my humble opinion, users should base their decision on the remote target they use to mount. If the remote target has stable group membership or if they have performance concerns, they can consider skipping the clearing of the file access cache. In order to avoid affecting current behavior, I will also change the default to not clear the file access cache. I understand that my choice may not be the best, but I will try to propose the mount option first. After I modify the man page description, I will resend the patches and discuss with the upstream. Once again, I really appreciate your help and guidance. Best regards, Chengen Du On Tue, May 9, 2023 at 11:16 PM Benjamin Coddington <bcodding@redhat.com> wrote: > > On 24 Apr 2023, at 21:41, Chengen Du wrote: > > > 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. > > I suspect there's some weariness of mount options, we have a lot of them and > they are not easily removed once implemented. Additionally, requests to add > them usually can show the appropriate changes to the nfs-utils mount.nfs and > man pages required. Incompleteness here may be the reason you're not > hearing back from a maintainer. > > However, without guidance from a maintainer, you might end up doing extra > work trying to meet unclear standards. > > There's a couple of other ways to address the access cache performance > "degradation" that was introduced by the changes that other folks > desperately needed for correctness. > > We can change nfs_access_login_time to have a module parameter modifying > the behavior. The downside is this would affect every mount. > > We can grow a sysfs knob to change the behavior. Downside is we only have > very preliminary sysfs scaffolding as of yet. > > However, if you want to keep pushing for the mount option, I'd suggest doing > a v2 with the userspace patches, and if that gets ignored then do a "PATCH > RESEND" on that a month or so before each mainline merge window. > > I've found that bump-replying to old patches isn't as effective at getting > work merged here. I believe the maintainers want to see that you're > rebasing as mainline progresses, and you have active ownership over the work > to fix bugs that may follow or address other fallout from the community. > > Ben > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-10 2:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-11 3:02 [PATCH] NFS: Add mount option 'nofasc' Chengen Du 2023-04-19 7:17 ` Daire Byrne 2023-04-25 1:41 ` Chengen Du 2023-05-09 2:31 ` Chengen Du 2023-05-09 11:53 ` Jan Ingvoldstad 2023-05-09 15:16 ` Benjamin Coddington 2023-05-10 2:26 ` Chengen Du
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®