| Message ID | 20230828055310.21391-1-chengen.du@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,RESEND] NFS: Add mount option 'fasc' | expand |
Hi, The performance issue has been brought to our attention by users within the Ubuntu community. However, it seems to be confined to specific user scenarios. Canonical has taken proactive measures to tackle the problem by implementing a temporary solution [1], which has effectively resolved the issue at hand. Nonetheless, our earnest desire is for a definitive resolution of the performance concern at its source upstream. I've taken the initiative to send the patches addressing this matter. Regrettably, as of now, I've yet to receive any response. This situation leads me to consider the possibility of reservations or deliberations surrounding this issue. I am genuinely keen to gain insights and perspectives from the upstream community. I kindly ask for your valuable input on this matter. Your thoughts would significantly aid my progress and contribute to a collective consensus. Best regards, Chengen Du [1] https://bugs.launchpad.net/bugs/2022098 On Mon, Aug 28, 2023 at 1:53 PM Chengen Du <chengen.du@canonical.com> 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 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; > @@ -3003,7 +3005,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_FASC) && > + nfs_check_access_stale(current, cred, cache)) > goto out; > *mask = 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 = NFS_I(inode); > - u64 login_time = nfs_access_login_time(current, cred); > struct nfs_access_entry *cache; > int err = -ECHILD; > struct list_head *lh; > @@ -3038,7 +3040,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_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 853e8d609bb3..2fbc2d1bd775 100644 > --- a/fs/nfs/fs_context.c > +++ b/fs/nfs/fs_context.c > @@ -92,6 +92,7 @@ enum nfs_param { > Opt_wsize, > Opt_write, > Opt_xprtsec, > + Opt_fasc, > }; > > enum { > @@ -199,6 +200,7 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = { > fsparam_enum ("write", Opt_write, nfs_param_enums_write), > fsparam_u32 ("wsize", Opt_wsize), > fsparam_string("xprtsec", Opt_xprtsec), > + fsparam_flag ("fasc", Opt_fasc), > {} > }; > > @@ -925,6 +927,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc, > case Opt_sloppy: > ctx->sloppy = true; > break; > + case Opt_fasc: > + ctx->flags |= NFS_MOUNT_FASC; > + break; > } > > return 0; > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > index 2284f749d892..7a0c5280e388 100644 > --- a/fs/nfs/super.c > +++ b/fs/nfs/super.c > @@ -448,6 +448,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 20eeba8b009d..6a88ba36f7a8 100644 > --- a/include/linux/nfs_fs_sb.h > +++ b/include/linux/nfs_fs_sb.h > @@ -155,6 +155,7 @@ struct nfs_server { > #define NFS_MOUNT_WRITE_WAIT 0x02000000 > #define NFS_MOUNT_TRUNK_DISCOVERY 0x04000000 > #define NFS_MOUNT_SHUTDOWN 0x08000000 > +#define NFS_MOUNT_FASC 0x10000000 > > unsigned int fattr_valid; /* Valid attributes */ > unsigned int caps; /* server capabilities */ > -- > 2.39.2 >
On 28 Aug 2023, at 3:14, Chengen Du wrote: > Hi, > > The performance issue has been brought to our attention by users > within the Ubuntu community. > However, it seems to be confined to specific user scenarios. > Canonical has taken proactive measures to tackle the problem by > implementing a temporary solution [1], which has effectively resolved > the issue at hand. > Nonetheless, our earnest desire is for a definitive resolution of the > performance concern at its source upstream. > > I've taken the initiative to send the patches addressing this matter. > Regrettably, as of now, I've yet to receive any response. > This situation leads me to consider the possibility of reservations or > deliberations surrounding this issue. > I am genuinely keen to gain insights and perspectives from the > upstream community. > > I kindly ask for your valuable input on this matter. > Your thoughts would significantly aid my progress and contribute to a > collective consensus. Hi Chengen Du, This patch changes the default behavior for everyone. Instead of that, perhaps the new option should change the behavior. That would reduce the change surface for all NFS users. The default behavior has already been hotly debated on this list and so I think changing the default would need to be accompanied by excellent reasons. Ben
Hi Benjamin, I'd like to provide some context about the new behavior introduced by the commit - 0eb43812c027 "NFS: Clear the file access cache upon login." This recent adoption has successfully resolved a long-standing issue. The current outcome is that the file access cache gets cleared when a user logs out and subsequently logs back in. In specific scenarios, users might access NFS-mounted folders via a 'sudo'-like behavior, inadvertently adding to the NFS server's load due to the inability to use the file cache efficiently. To alleviate this performance overhead, my proposal centers on reverting to the original behavior, where the file access cache remains untouched upon user login. This stems from the rationale that the cache should only be cleared when the server's group membership changes after a user has already logged in on the client. This alteration rarely occurs in stable environments, thus rendering the file access cache reliable. With this in mind, my suggestion involves adding a mount option that empowers administrators to dictate which NFS-mounted folders adhere to the POSIX behavior - one that refreshes a user's supplementary group information upon login. The genesis of this patch places a premium on performance while also maintaining alignment with the original behavior prior to the adoption of the commit 0eb43812c027. Transitioning to adhere strictly to the POSIX policy also carries merit. I believe that further discussion can facilitate a consensus on this matter. Thank you for lending your perspective to this discourse. Best regards, Chengen Du On Tue, Aug 29, 2023 at 9:35 PM Benjamin Coddington <bcodding@redhat.com> wrote: > > On 28 Aug 2023, at 3:14, Chengen Du wrote: > > > Hi, > > > > The performance issue has been brought to our attention by users > > within the Ubuntu community. > > However, it seems to be confined to specific user scenarios. > > Canonical has taken proactive measures to tackle the problem by > > implementing a temporary solution [1], which has effectively resolved > > the issue at hand. > > Nonetheless, our earnest desire is for a definitive resolution of the > > performance concern at its source upstream. > > > > I've taken the initiative to send the patches addressing this matter. > > Regrettably, as of now, I've yet to receive any response. > > This situation leads me to consider the possibility of reservations or > > deliberations surrounding this issue. > > I am genuinely keen to gain insights and perspectives from the > > upstream community. > > > > I kindly ask for your valuable input on this matter. > > Your thoughts would significantly aid my progress and contribute to a > > collective consensus. > > Hi Chengen Du, > > This patch changes the default behavior for everyone. Instead of that, > perhaps the new option should change the behavior. That would reduce the > change surface for all NFS users. > > The default behavior has already been hotly debated on this list and so I > think changing the default would need to be accompanied by excellent > reasons. > > Ben >
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 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; @@ -3003,7 +3005,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_FASC) && + nfs_check_access_stale(current, cred, cache)) goto out; *mask = 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 = NFS_I(inode); - u64 login_time = nfs_access_login_time(current, cred); struct nfs_access_entry *cache; int err = -ECHILD; struct list_head *lh; @@ -3038,7 +3040,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_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 853e8d609bb3..2fbc2d1bd775 100644 --- a/fs/nfs/fs_context.c +++ b/fs/nfs/fs_context.c @@ -92,6 +92,7 @@ enum nfs_param { Opt_wsize, Opt_write, Opt_xprtsec, + Opt_fasc, }; enum { @@ -199,6 +200,7 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = { fsparam_enum ("write", Opt_write, nfs_param_enums_write), fsparam_u32 ("wsize", Opt_wsize), fsparam_string("xprtsec", Opt_xprtsec), + fsparam_flag ("fasc", Opt_fasc), {} }; @@ -925,6 +927,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc, case Opt_sloppy: ctx->sloppy = true; break; + case Opt_fasc: + ctx->flags |= NFS_MOUNT_FASC; + break; } return 0; diff --git a/fs/nfs/super.c b/fs/nfs/super.c index 2284f749d892..7a0c5280e388 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -448,6 +448,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 20eeba8b009d..6a88ba36f7a8 100644 --- a/include/linux/nfs_fs_sb.h +++ b/include/linux/nfs_fs_sb.h @@ -155,6 +155,7 @@ struct nfs_server { #define NFS_MOUNT_WRITE_WAIT 0x02000000 #define NFS_MOUNT_TRUNK_DISCOVERY 0x04000000 #define NFS_MOUNT_SHUTDOWN 0x08000000 +#define NFS_MOUNT_FASC 0x10000000 unsigned int fattr_valid; /* Valid attributes */ unsigned int caps; /* server capabilities */
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(-)