Support follow_link in RCU-walk.
Subsystems (16)
cover Support follow_link in RCU-walk.
Hi Al (and others),
I wonder if you could look over this patchset.
It allows RCU-walk to follow symlinks in many common cases, thus removing a surprising performance hit caused by using symlinks.
The last could of patches make changes to XFS and NFS to support this but I haven't forwarded to the relevant lists yet. If/when the early code meets with approval I'll do that.
The first patch almost certainly needs to be changed. I originally wrote this code when filesystems could see inside nameidata. It is now opaque so the simplest solution was to provide an accessor function.
Maybe I should as a 'flags' arg to ->follow_link?? Or have ->follow_link and ->follow_link_rcu ??
What do you suggest?
Thanks,
NeilBrown
---
NeilBrown (9):
FS: make all ->follow_link handlers aware for LOOKUP_RCU VFS/namei: use terminate_walk when symlink lookup fails. VFS/namei: new flag to support RCU symlinks: LOOKUP_LINK_RCU. VFS/namei: abort RCU-walk on symlink if atime needs updating. VFS/namei: enhance follow_link to support RCU-walk. VFS/namei: enable RCU-walk when following symlinks. VFS/namei: handle LOOKUP_RCU in page_follow_link_light.
XFS: allow follow_link to often succeed in RCU-walk.
NFS: support LOOKUP_RCU in nfs_follow_link.
drivers/staging/lustre/lustre/llite/symlink.c | 3 + fs/9p/vfs_inode.c | 6 + fs/9p/vfs_inode_dotl.c | 5 + fs/befs/linuxvfs.c | 2 fs/cifs/link.c | 2 fs/configfs/symlink.c | 7 + fs/ecryptfs/inode.c | 7 + fs/fuse/dir.c | 2 fs/gfs2/inode.c | 2 fs/hostfs/hostfs_kern.c | 7 + fs/inode.c | 26 ++++- fs/kernfs/symlink.c | 7 + fs/namei.c | 132 ++++++++++++++++--------- fs/nfs/inode.c | 22 ++++ fs/nfs/symlink.c | 17 +++ fs/overlayfs/inode.c | 3 + fs/proc/base.c | 2 fs/proc/namespaces.c | 3 + fs/proc/self.c | 4 + fs/proc/thread_self.c | 4 + fs/xfs/xfs_ioctl.c | 2 fs/xfs/xfs_iops.c | 13 ++ fs/xfs/xfs_symlink.c | 11 ++ fs/xfs/xfs_symlink.h | 2 include/linux/fs.h | 2 include/linux/namei.h | 1 include/linux/nfs_fs.h | 1 mm/shmem.c | 6 + 28 files changed, 234 insertions(+), 67 deletions(-)
--
Signature
PATCH 1/9 FS: make all ->follow_link handlers aware for LOOKUP_RCU
In preparation for supporting ->follow_link in RCU-walk, make sure all ->follow_link handers which are not atomic will fail if LOOKUP_RCU is set.
Later patches will make some of these handle LOOKUP_RCU more gracefully.
This is current achieved by introducing a new function "nd_is_rcu" to check if a nameidata has LOOKUP_RCU set. There must be a better way.
--- drivers/staging/lustre/lustre/llite/symlink.c | 3 +++ fs/9p/vfs_inode.c | 6 +++++- fs/9p/vfs_inode_dotl.c | 5 ++++- fs/befs/linuxvfs.c | 2 ++ fs/cifs/link.c | 2 ++ fs/configfs/symlink.c | 7 ++++++- fs/ecryptfs/inode.c | 7 ++++++- fs/fuse/dir.c | 2 ++ fs/gfs2/inode.c | 2 ++ fs/hostfs/hostfs_kern.c | 7 ++++++- fs/kernfs/symlink.c | 7 ++++++- fs/namei.c | 8 ++++++++ fs/nfs/symlink.c | 2 ++ fs/overlayfs/inode.c | 3 +++ fs/proc/base.c | 2 ++ fs/proc/namespaces.c | 3 +++ fs/proc/self.c | 4 ++++ fs/proc/thread_self.c | 4 ++++ fs/xfs/xfs_iops.c | 2 ++ include/linux/fs.h | 1 + mm/shmem.c | 6 +++++- 21 files changed, 78 insertions(+), 7 deletions(-)
drivers/staging/lustre/lustre/llite/symlink.c +3 -0
@@ -125,6 +125,9 @@ static void *ll_follow_link(struct dentry *dentry, struct nameidata *nd)int rc;char *symname =NULL ; +if (nd_is_rcu (nd)) +return PTR_ERR (-ECHILD ); +CDEBUG (D_VFSTRACE ,"VFS Op\n" );/* Limit the recursive symlink depth to 5 instead of default * 8 links when kernel has 4k stack to prevent stack overflow.
fs/9p/vfs_inode.c +5 -1
@@ -1281,7 +1281,11 @@ done:static void *v9fs_vfs_follow_link (struct dentry *dentry,struct nameidata *nd) {int len =0 ; -char *link =__getname (); +char *link; + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + link =__getname ();p9_debug (P9_DEBUG_VFS ,"%pd\n" , dentry);
fs/9p/vfs_inode_dotl.c +4 -1
@@ -914,9 +914,12 @@ v9fs_vfs_follow_link_dotl(struct dentry *dentry, struct nameidata *nd) {int retval;struct p9_fid *fid; -char *link =__getname (); +char *link;char *target; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + link =__getname ();p9_debug (P9_DEBUG_VFS ,"%pd\n" , dentry);if (!link) {
fs/befs/linuxvfs.c +2 -0
@@ -477,6 +477,8 @@ befs_follow_link(struct dentry *dentry, struct nameidata *nd)befs_off_t len = data->size;char *link; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD );if (len ==0 ) {befs_error (sb,"Long symlink with illegal length" ); link =ERR_PTR (-EIO );
fs/cifs/link.c +2 -0
@@ -639,6 +639,8 @@ cifs_follow_link(struct dentry *direntry, struct nameidata *nd)struct cifs_tcon *tcon;struct TCP_Server_Info *server; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); xid =get_xid (); tlink =cifs_sb_tlink (cifs_sb);
fs/configfs/symlink.c +6 -1
@@ -282,7 +282,12 @@ static int configfs_getlink(struct dentry *dentry, char * path)static void *configfs_follow_link (struct dentry *dentry,struct nameidata *nd) {int error = -ENOMEM ; -unsigned long page =get_zeroed_page (GFP_KERNEL ); +unsigned long page; + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + + page =get_zeroed_page (GFP_KERNEL );if (page) { error =configfs_getlink (dentry, (char *)page);
fs/ecryptfs/inode.c +6 -1
@@ -678,7 +678,12 @@ out:static void *ecryptfs_follow_link (struct dentry *dentry,struct nameidata *nd) {size_t len; -char *buf =ecryptfs_readlink_lower (dentry, &len); +char *buf; + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + + buf =ecryptfs_readlink_lower (dentry, &len);if (IS_ERR (buf)) goto out; fsstack_copy_attr_atime(dentry->d_inode,
fs/fuse/dir.c +2 -0
@@ -1402,6 +1402,8 @@ static void free_link(char *link)static void *fuse_follow_link (struct dentry *dentry,struct nameidata *nd) { +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD );nd_set_link (nd,read_link (dentry));return NULL ; }
fs/gfs2/inode.c +2 -0
@@ -1557,6 +1557,8 @@ static void *gfs2_follow_link(struct dentry *dentry, struct nameidata *nd)char *buf;int error; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD );gfs2_holder_init (ip->i_gl,LM_ST_SHARED ,0 , &i_gh); error =gfs2_glock_nq (&i_gh);if (error) {
fs/hostfs/hostfs_kern.c +6 -1
@@ -884,7 +884,12 @@ static const struct inode_operations hostfs_dir_iops = {static void *hostfs_follow_link (struct dentry *dentry,struct nameidata *nd) { -char *link =__getname (); +char *link; + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + + link =__getname ();if (link) {char *path =dentry_name (dentry);int err = -ENOMEM ;
fs/kernfs/symlink.c +6 -1
@@ -115,7 +115,12 @@ static int kernfs_getlink(struct dentry *dentry, char *path)static void *kernfs_iop_follow_link (struct dentry *dentry,struct nameidata *nd) {int error = -ENOMEM ; -unsigned long page =get_zeroed_page (GFP_KERNEL ); +unsigned long page; + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + + page =get_zeroed_page (GFP_KERNEL );if (page) { error =kernfs_getlink (dentry, (char *) page);if (error <0 )
fs/namei.c +8 -0
@@ -4475,6 +4475,8 @@ EXPORT_SYMBOL(page_readlink);void *page_follow_link_light (struct dentry *dentry,struct nameidata *nd) {struct page *page =NULL ; +if (nd->flags &LOOKUP_RCU ) +return ERR_PTR (-ECHILD );nd_set_link (nd,page_getlink (dentry, &page));return page; }
@@ -4542,3 +4544,9 @@ const struct inode_operations page_symlink_inode_operations = { .put_link = page_put_link, };EXPORT_SYMBOL (page_symlink_inode_operations); + +int nd_is_rcu (struct nameidata *nd) +{ +return nd->flags &LOOKUP_RCU ; +} +EXPORT_SYMBOL (nd_is_rcu);
fs/nfs/symlink.c +2 -0
@@ -49,6 +49,8 @@ static void *nfs_follow_link(struct dentry *dentry, struct nameidata *nd)struct page *page;void *err; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); err =ERR_PTR (nfs_revalidate_mapping (inode, inode->i_mapping));if (err) goto read_failed;
fs/overlayfs/inode.c +3 -0
@@ -8,6 +8,7 @@*/ #include <linux/fs.h> +#include <linux/namei.h> #include <linux/slab.h> #include <linux/xattr.h> #include "overlayfs.h"
@@ -146,6 +147,8 @@ static void *ovl_follow_link(struct dentry *dentry, struct nameidata *nd)struct dentry *realdentry;struct inode *realinode; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); realdentry =ovl_dentry_real (dentry); realinode = realdentry->d_inode;
fs/proc/base.c +2 -0
@@ -1377,6 +1377,8 @@ static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)struct path path;int error = -EACCES ; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD );/* Are we allowed to snoop on the tasks file descriptors? */ if (!proc_fd_access_allowed (inode)) goto out;
fs/proc/namespaces.c +3 -0
@@ -38,6 +38,9 @@ static void *proc_ns_follow_link(struct dentry *dentry, struct nameidata *nd)struct path ns_path;void *error =ERR_PTR (-EACCES ); +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + task =get_proc_task (inode);if (!task)return error;
fs/proc/self.c +4 -0
@@ -24,6 +24,10 @@ static void *proc_self_follow_link(struct dentry *dentry, struct nameidata *nd)struct pid_namespace *ns = dentry->d_sb->s_fs_info;pid_t tgid =task_tgid_nr_ns (current, ns);char *name =ERR_PTR (-ENOENT ); + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); +if (tgid) {/* 11 for max length of signed int in decimal + NULL term */ name =kmalloc (12 ,GFP_KERNEL );
fs/proc/thread_self.c +4 -0
@@ -26,6 +26,10 @@ static void *proc_thread_self_follow_link(struct dentry *dentry, struct nameidatpid_t tgid =task_tgid_nr_ns (current, ns);pid_t pid =task_pid_nr_ns (current, ns);char *name =ERR_PTR (-ENOENT ); + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); +if (pid) { name =kmalloc (PROC_NUMBUF +6 +PROC_NUMBUF ,GFP_KERNEL );if (!name)
fs/xfs/xfs_iops.c +2 -0
@@ -417,6 +417,8 @@ xfs_vn_follow_link(char *link;int error = -ENOMEM ; +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); link =kmalloc (MAXPATHLEN +1 ,GFP_KERNEL );if (!link) goto out_err;
include/linux/fs.h +1 -0
@@ -2167,6 +2167,7 @@ extern struct filename *getname_flags(const char __user *, int, int *);extern struct filename *getname (const char __user *);extern struct filename *getname_kernel (const char *);extern void putname (struct filename *name); +extern int nd_is_rcu (struct nameidata *nd);enum {FILE_CREATED =1 ,
mm/shmem.c +5 -1
@@ -2483,7 +2483,11 @@ static void *shmem_follow_short_symlink(struct dentry *dentry, struct nameidatastatic void *shmem_follow_link (struct dentry *dentry,struct nameidata *nd) {struct page *page =NULL ; -int error =shmem_getpage (dentry->d_inode,0 , &page,SGP_READ ,NULL ); +int error; + +if (nd_is_rcu (nd)) +return ERR_PTR (-ECHILD ); + error =shmem_getpage (dentry->d_inode,0 , &page,SGP_READ ,NULL );nd_set_link (nd, error ?ERR_PTR (error) :kmap (page));if (page)unlock_page (page);
PATCH 2/9 VFS/namei: use terminate_walk when symlink lookup fails.
In reply to NeilBrownCurrently following a symlink never uses rcu-walk, so terminate_walk isn't needed.
That will change in a future patch. In preparation, change some
path_put_condtional()
path_put()
sequences to
path_to_nameidata()
terminate_walk()
These sequence are identical when in ref-walk, and correct when in rcu-walk.
Also change two path_put() calls to equivalent terminate_walk().
--- fs/namei.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
fs/namei.c +20 -20
@@ -726,6 +726,18 @@ char *nd_get_link(struct nameidata *nd) }EXPORT_SYMBOL (nd_get_link); +static void terminate_walk (struct nameidata *nd) +{ +if (!(nd->flags &LOOKUP_RCU )) { +path_put (&nd->path); + }else { + nd->flags &= ~LOOKUP_RCU ; +if (!(nd->flags &LOOKUP_ROOT )) + nd->root.mnt =NULL ; +rcu_read_unlock (); + } +} +static inline void put_link (struct nameidata *nd,struct path *link,void *cookie) {struct inode *inode = link->dentry->d_inode;
@@ -776,8 +788,8 @@ static inline int may_follow_link(struct path *link, struct nameidata *nd)return 0 ;audit_log_link_denied ("follow_link" , link); -path_put_conditional (link, nd); -path_put (&nd->path); +path_to_nameidata (link, nd); +terminate_walk (nd);return -EACCES ; }
@@ -886,7 +898,7 @@ follow_link(struct path *link, struct nameidata *nd, void **p) s =nd_get_link (nd);if (s) {if (unlikely (IS_ERR (s))) { -path_put (&nd->path); +terminate_walk (nd);put_link (nd, link, *p);return PTR_ERR (s); }
@@ -908,7 +920,7 @@ follow_link(struct path *link, struct nameidata *nd, void **p) out_put_nd_path: *p =NULL ; -path_put (&nd->path); +terminate_walk (nd);path_put (link);return error; }
@@ -1539,18 +1551,6 @@ static inline int handle_dots(struct nameidata *nd, int type)return 0 ; } -static void terminate_walk (struct nameidata *nd) -{ -if (!(nd->flags &LOOKUP_RCU )) { -path_put (&nd->path); - }else { - nd->flags &= ~LOOKUP_RCU ; -if (!(nd->flags &LOOKUP_ROOT )) - nd->root.mnt =NULL ; -rcu_read_unlock (); - } -} -/* * Do we need to follow links? We _really_ want to be able * to do this check without having to look at inode->i_op,
@@ -1622,8 +1622,8 @@ static inline int nested_symlink(struct path *path, struct nameidata *nd)int res;if (unlikely (current->link_count >=MAX_NESTED_LINKS )) { -path_put_conditional (path, nd); -path_put (&nd->path); +path_to_nameidata (path, nd); +terminate_walk (nd);return -ELOOP ; }BUG_ON (nd->depth >=MAX_NESTED_LINKS );
@@ -3238,8 +3238,8 @@ static struct file *path_openat(int dfd, struct filename *pathname,struct path link = path;void *cookie;if (!(nd->flags &LOOKUP_FOLLOW )) { -path_put_conditional (&path, nd); -path_put (&nd->path); +path_to_nameidata (&path, nd); +terminate_walk (nd); error = -ELOOP ;break ; }
PATCH 3/9 VFS/namei: new flag to support RCU symlinks: LOOKUP_LINK_RCU.
In reply to NeilBrownWhen we support ->follow_link in RCU-walk we will not want to take a reference to the 'struct path *link' passed to follow_link, and correspondingly will not want to drop that reference.
As link_path_walk will complete_walk() in the case of an error, and as complete_walk() will clear LOOKUP_RCU, we cannot test LOOKUP_RCU to determine if the path should be 'put'.
So introduce a new flag: LOOKUP_LINK_RCU. This is set on entry to follow_link() if appropriate and put_link() will only call path_put() if it is clear.
Also, unlazy_walk() will fail if LOOKUP_LINK_RCU is set. This is because there is no way for unlazy_walk to get references on all the "struct path *link"s that are protected by that flag.
--- fs/namei.c | 18 +++++++++++++----- include/linux/namei.h | 1 + 2 files changed, 14 insertions(+), 5 deletions(-)
fs/namei.c +13 -5
@@ -533,6 +533,9 @@ static int unlazy_walk(struct nameidata *nd, struct dentry *dentry)struct dentry *parent = nd->path.dentry;BUG_ON (!(nd->flags &LOOKUP_RCU )); +if (nd->flags &LOOKUP_LINK_RCU ) +/* Cannot unlazy in the middle of following a symlink */ +return -ECHILD ;/* * After legitimizing the bastards, terminate_walk()
@@ -743,7 +746,8 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cookistruct inode *inode = link->dentry->d_inode;if (inode->i_op->put_link) inode->i_op->put_link (link->dentry, nd, cookie); -path_put (link); +if (!(nd->flags &LOOKUP_LINK_RCU )) +path_put (link); }int sysctl_protected_symlinks__read_mostly =0 ;
@@ -869,9 +873,10 @@ follow_link(struct path *link, struct nameidata *nd, void **p)int error;char *s; -BUG_ON (nd->flags &LOOKUP_RCU ); - -if (link->mnt == nd->path.mnt) + nd->flags &= ~LOOKUP_LINK_RCU ; +if (nd->flags &LOOKUP_RCU ) + nd->flags |=LOOKUP_LINK_RCU ; +else if (link->mnt == nd->path.mnt)mntget (link->mnt); error = -ELOOP ;
@@ -921,7 +926,8 @@ follow_link(struct path *link, struct nameidata *nd, void **p) out_put_nd_path: *p =NULL ;terminate_walk (nd); -path_put (link); +if (!(nd->flags &LOOKUP_LINK_RCU )) +path_put (link);return error; }
@@ -1644,6 +1650,8 @@ static inline int nested_symlink(struct path *path, struct nameidata *nd) current->link_count--; nd->depth--; +if (!nd->depth) + nd->flags &= ~LOOKUP_LINK_RCU ;return res; }
include/linux/namei.h +1 -0
@@ -32,6 +32,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};#define LOOKUP_PARENT 0x0010#define LOOKUP_REVAL 0x0020#define LOOKUP_RCU 0x0040 +#define LOOKUP_LINK_RCU 0x0080/* * Intent data
PATCH 4/9 VFS/namei: abort RCU-walk on symlink if atime needs updating.
In reply to NeilBrowntouch_atime is not RCU-safe, and so cannot be called on an RCU walk.
However in situations where RCU-walk makes a difference, the symlink will likely to accessed much more often than it is useful to update the atime.
So split out the test of "Does the atime actually need to be updated" into atime_needs_update(), and only allow RCU-walk on a symlink if that fails.
--- fs/inode.c | 26 +++++++++++++++++++------- fs/namei.c | 7 ++++++- include/linux/fs.h | 1 + 3 files changed, 26 insertions(+), 8 deletions(-)
fs/inode.c +19 -7
@@ -1584,30 +1584,41 @@ static int update_time(struct inode *inode, struct timespec *time, int flags)* This function automatically handles read only file systems and media, * as well as the "noatime" flag and inode specific "noatime" markers. */ -void touch_atime (const struct path *path) +int atime_needs_update (const struct path *path) {struct vfsmount *mnt = path->mnt;struct inode *inode = path->dentry->d_inode;struct timespec now;if (inode->i_flags &S_NOATIME ) -return ; +return 0 ;if (IS_NOATIME (inode)) -return ; +return 0 ;if ((inode->i_sb->s_flags &MS_NODIRATIME ) &&S_ISDIR (inode->i_mode)) -return ; +return 0 ;if (mnt->mnt_flags &MNT_NOATIME ) -return ; +return 0 ;if ((mnt->mnt_flags &MNT_NODIRATIME ) &&S_ISDIR (inode->i_mode)) -return ; +return 0 ; now =current_fs_time (inode->i_sb);if (!relatime_need_update (mnt, inode, now)) -return ; +return 0 ;if (timespec_equal (&inode->i_atime, &now)) +return 0 ; +return 1 ; +} + +void touch_atime (const struct path *path) +{ +struct vfsmount *mnt = path->mnt; +struct inode *inode = path->dentry->d_inode; +struct timespec now; + +if (!atime_needs_update (path))return ;if (!sb_start_write_trylock (inode->i_sb))
@@ -1624,6 +1635,7 @@ void touch_atime(const struct path *path)* We may also fail on filesystems that have the ability to make parts * of the fs read only, e.g. subvolumes in Btrfs. */ + now =current_fs_time (inode->i_sb);update_time (inode, &now,S_ATIME );__mnt_drop_write (mnt); skip_update:
fs/namei.c +6 -1
@@ -886,7 +886,12 @@ follow_link(struct path *link, struct nameidata *nd, void **p)cond_resched (); current->total_link_count++; -touch_atime (link); +if (nd->flags &LOOKUP_RCU ) { + error = -ECHILD ; +if (atime_needs_update (link)) + goto out_put_nd_path; + }else +touch_atime (link);nd_set_link (nd,NULL ); error =security_inode_follow_link (link->dentry, nd);
include/linux/fs.h +1 -0
@@ -1845,6 +1845,7 @@ enum file_time_flags { S_VERSION = 8, }; +extern int atime_needs_update (const struct path *);extern void touch_atime (const struct path *);static inline void file_accessed (struct file *file) {
PATCH 5/9 VFS/namei: enhance follow_link to support RCU-walk.
In reply to NeilBrownIf LOOKUP_RCU is set, follow_link will not take/drop reference counts.
Replace cond_resched() with _cond_resched() as the latter is a no-op if rcu_read_lock() is held while the former will give a warning in that case.
--- fs/namei.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
fs/namei.c +13 -6
@@ -883,7 +883,8 @@ follow_link(struct path *link, struct nameidata *nd, void **p)if (unlikely (current->total_link_count >=40 )) goto out_put_nd_path; -cond_resched (); +/* If rcu_read_locked(), this will not resched, and will not warn */ +_cond_resched (); current->total_link_count++;if (nd->flags &LOOKUP_RCU ) {
@@ -913,11 +914,17 @@ follow_link(struct path *link, struct nameidata *nd, void **p)return PTR_ERR (s); }if (*s =='/' ) { -if (!nd->root.mnt) -set_root (nd); -path_put (&nd->path); - nd->path = nd->root; -path_get (&nd->root); +if (nd->flags &LOOKUP_RCU ) { +if (!nd->root.mnt) +set_root_rcu (nd); + nd->path = nd->root; + }else { +if (!nd->root.mnt) +set_root (nd); +path_put (&nd->path); + nd->path = nd->root; +path_get (&nd->root); + } nd->flags |=LOOKUP_JUMPED ; } nd->inode = nd->path.dentry->d_inode;
PATCH 6/9 VFS/namei: enable RCU-walk when following symlinks.
In reply to NeilBrownNow that follow_link handles LOOKUP_RCU, we do not need to 'unlazy_walk' when a symlink is found.
--- fs/namei.c | 12 ------------ 1 file changed, 12 deletions(-)
fs/namei.c +0 -12
@@ -1608,12 +1608,6 @@ static inline int walk_component(struct nameidata *nd, struct path *path, goto out_path_put;if (should_follow_link (path->dentry, follow)) { -if (nd->flags &LOOKUP_RCU ) { -if (unlikely (unlazy_walk (nd, path->dentry))) { - err = -ECHILD ; - goto out_err; - } - }BUG_ON (inode != path->dentry->d_inode);return 1 ; }
@@ -3066,12 +3060,6 @@ finish_lookup: }if (should_follow_link (path->dentry, !symlink_ok)) { -if (nd->flags &LOOKUP_RCU ) { -if (unlikely (unlazy_walk (nd, path->dentry))) { - error = -ECHILD ; - goto out; - } - }BUG_ON (inode != path->dentry->d_inode);return 1 ; }
PATCH 7/9 VFS/namei: handle LOOKUP_RCU in page_follow_link_light.
In reply to NeilBrownIf the symlink has already be been read-in, then page_follow_link_light can succeed in RCU-walk mode. page_getlink_rcu() is added to support this.
With this many filesystems can follow links in RCU-walk mode when everything is cached. This includes ext?fs and others.
If the page is a HighMem page we do *not* try to kmap_atomic, but simply give up - only page_address() is used. This is because we need to be able to sleep while holding the address of the page, particularly over calls to do_last() which can be quite slow and in particular takes a mutex.
If this were a problem, then copying into a GFP_ATOMIC allocation might be a workable solution.
--- fs/namei.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-)
fs/namei.c +27 -3
@@ -4468,6 +4468,28 @@ static char *page_getlink(struct dentry * dentry, struct page **ppage)return kaddr; } +/* get the link contents from pagecache under RCU */ +static char *page_getlink_rcu (struct dentry * dentry,struct page **ppage) +{ +char *kaddr; +struct page *page; +struct address_space *mapping = dentry->d_inode->i_mapping; + page =find_get_page (mapping,0 ); +if (page && + (!PageUptodate (page) ||PageHighMem (page))) { +put_page (page); + page =NULL ; + } +if (!page) { + *ppage =ERR_PTR (-ECHILD ); +return NULL ; + } + *ppage = page; + kaddr =page_address (page); +nd_terminate_link (kaddr, dentry->d_inode->i_size,PAGE_SIZE -1 ); +return kaddr; +} +int page_readlink (struct dentry *dentry,char __user *buffer,int buflen) {struct page *page =NULL ;
@@ -4484,8 +4506,9 @@ void *page_follow_link_light(struct dentry *dentry, struct nameidata *nd) {struct page *page =NULL ;if (nd->flags &LOOKUP_RCU ) -return ERR_PTR (-ECHILD ); -nd_set_link (nd,page_getlink (dentry, &page)); +nd_set_link (nd,page_getlink_rcu (dentry, &page)); +else +nd_set_link (nd,page_getlink (dentry, &page));return page; }EXPORT_SYMBOL (page_follow_link_light);
@@ -4495,7 +4518,8 @@ void page_put_link(struct dentry *dentry, struct nameidata *nd, void *cookie)struct page *page = cookie;if (page) { -kunmap (page); +if (!(nd->flags &LOOKUP_LINK_RCU )) +kunmap (page);page_cache_release (page); } }
PATCH 8/9 XFS: allow follow_link to often succeed in RCU-walk.
In reply to NeilBrownIf LOOKUP_RCU is set, use GFP_ATOMIC rather than GFP_KERNEL, and try to get the ilock without blocking.
When these succeed, follow_link() can succeed without dropping out of RCU-walk.
--- fs/xfs/xfs_ioctl.c | 2 +- fs/xfs/xfs_iops.c | 15 ++++++++++----- fs/xfs/xfs_symlink.c | 11 +++++++++-- fs/xfs/xfs_symlink.h | 2 +- 4 files changed, 21 insertions(+), 9 deletions(-)
fs/xfs/xfs_ioctl.c +1 -1
@@ -303,7 +303,7 @@ xfs_readlink_by_handle( goto out_dput; } - error =xfs_readlink (XFS_I (dentry->d_inode), link); + error =xfs_readlink (XFS_I (dentry->d_inode), link,0 );if (error) goto out_kfree; error =readlink_copy (hreq->ohandle, olen, link);
fs/xfs/xfs_iops.c +10 -5
@@ -415,15 +415,20 @@ xfs_vn_follow_link( struct nameidata *nd) {char *link; -int error = -ENOMEM ; +int error; -if (nd_is_rcu (nd)) -return ERR_PTR (-ECHILD ); - link =kmalloc (MAXPATHLEN +1 ,GFP_KERNEL ); +if (nd_is_rcu (nd)) { + error = -ECHILD ; + link =kmalloc (MAXPATHLEN +1 ,GFP_ATOMIC ); + }else { + error = -ENOMEM ; + link =kmalloc (MAXPATHLEN +1 ,GFP_KERNEL ); + }if (!link) goto out_err; - error =xfs_readlink (XFS_I (dentry->d_inode), link); + error =xfs_readlink (XFS_I (dentry->d_inode), link, +nd_is_rcu (nd));if (unlikely (error)) goto out_kfree;
fs/xfs/xfs_symlink.c +9 -2
@@ -123,7 +123,8 @@ xfs_readlink_bmap(int xfs_readlink (struct xfs_inode *ip, -char *link) +char *link, +int rcu) {struct xfs_mount *mp = ip->i_mount;xfs_fsize_t pathlen;
@@ -134,7 +135,11 @@ xfs_readlink(if (XFS_FORCED_SHUTDOWN (mp))return -EIO ; -xfs_ilock (ip,XFS_ILOCK_SHARED ); +if (rcu) { +if (xfs_ilock_nowait (ip,XFS_ILOCK_SHARED ) ==0 ) +return -ECHILD ; + }else +xfs_ilock (ip,XFS_ILOCK_SHARED ); pathlen = ip->i_d.di_size;if (!pathlen)
@@ -153,6 +158,8 @@ xfs_readlink(if (ip->i_df.if_flags &XFS_IFINLINE ) {memcpy (link, ip->i_df.if_u1.if_data, pathlen); link[pathlen] ='\0' ; + }else if (rcu) { + error = -ECHILD ; }else { error =xfs_readlink_bmap (ip, link); }
fs/xfs/xfs_symlink.h +1 -1
@@ -21,7 +21,7 @@int xfs_symlink (struct xfs_inode *dp,struct xfs_name *link_name,const char *target_path,umode_t mode,struct xfs_inode **ipp); -int xfs_readlink (struct xfs_inode *ip,char *link); +int xfs_readlink (struct xfs_inode *ip,char *link,int rcu);int xfs_inactive_symlink (struct xfs_inode *ip);#endif /* __XFS_SYMLINK_H */
PATCH 9/9 NFS: support LOOKUP_RCU in nfs_follow_link.
In reply to NeilBrownIf the inode is valid and the page has been read in, then we can follow a link in RCU-walk.
--- fs/nfs/inode.c | 22 ++++++++++++++++++++++ fs/nfs/symlink.c | 19 +++++++++++++++++-- include/linux/nfs_fs.h | 1 + 3 files changed, 40 insertions(+), 2 deletions(-)
fs/nfs/inode.c +22 -0
@@ -1123,6 +1123,28 @@ out:return ret; } +int nfs_revalidate_mapping_rcu (struct inode *inode) +{ +struct nfs_inode *nfsi =NFS_I (inode); +unsigned long *bitlock = &nfsi->flags; +int ret =0 ; + +if (IS_SWAPFILE (inode)) + goto out; +if (nfs_mapping_need_revalidate_inode (inode)) { + ret = -ECHILD ; + goto out; + } +spin_lock (&inode->i_lock); +if (test_bit (NFS_INO_INVALIDATING , bitlock) || + (nfsi->cache_validity &NFS_INO_INVALID_DATA )) + ret = -ECHILD ; +spin_unlock (&inode->i_lock); +out: +return ret; +} + +static unsigned long nfs_wcc_update_inode (struct inode *inode,struct nfs_fattr *fattr) {struct nfs_inode *nfsi =NFS_I (inode);
fs/nfs/symlink.c +17 -2
@@ -49,8 +49,23 @@ static void *nfs_follow_link(struct dentry *dentry, struct nameidata *nd)struct page *page;void *err; -if (nd_is_rcu (nd)) -return ERR_PTR (-ECHILD ); +if (nd_is_rcu (nd)) { + err =ERR_PTR (nfs_revalidate_mapping_rcu (inode)); +if (err) + goto read_failed; + page =find_get_page (inode->i_mapping,0 ); +if (page && + (!PageUptodate (page) ||PageHighMem (page))) { +put_page (page); + page =NULL ; + } +if (!page) { + err =ERR_PTR (-ECHILD ); + goto read_failed; + } +nd_set_link (nd,page_address (page)); +return page; + } err =ERR_PTR (nfs_revalidate_mapping (inode, inode->i_mapping));if (err) goto read_failed;
include/linux/nfs_fs.h +1 -0
@@ -355,6 +355,7 @@ extern int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode);extern int nfs_revalidate_inode_rcu (struct nfs_server *server,struct inode *inode);extern int __nfs_revalidate_inode (struct nfs_server *,struct inode *);extern int nfs_revalidate_mapping (struct inode *inode,struct address_space *mapping); +extern int nfs_revalidate_mapping_rcu (struct inode *inode);extern int nfs_setattr (struct dentry *,struct iattr *);extern void nfs_setattr_update_inode (struct inode *inode,struct iattr *attr); extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
Al Viro
maintainerQuoted 10 lines · NeilBrown
Hi Al (and others),
I wonder if you could look over this patchset.
It allows RCU-walk to follow symlinks in many common cases, thus removing a surprising performance hit caused by using symlinks.The last could of patches make changes to XFS and NFS to support this but I haven't forwarded to the relevant lists yet. If/when the early code meets with approval I'll do that.
Quoted 7 lines
The first patch almost certainly needs to be changed. I originally wrote this code when filesystems could see inside nameidata. It is now opaque so the simplest solution was to provide an accessor function.
Maybe I should as a 'flags' arg to ->follow_link?? Or have ->follow_link and ->follow_link_rcu ??
What do you suggest?
Umm... Some observations:
* now ->follow_link() can be called in RCU mode, which means
that it can race with fs shutdown; not a problem, except that now it joins ->lookup() et.al. in "if some data structure is needed in RCU case of that, make sure it's not destroyed without an RCU delay somewhere between the entry into ->kill_sb() and destruction.
* highmem pages in symlinks: that BS shouldn't be allowed at
all. Just make sure that at least for those filesystems symlink inodes get mapping_set_gfp_mask(&inode->i_data, GFP_KERNEL) and be done with that.
* are you sure that security_inode_follow_link() is OK to call in
RCU mode?
* what warranties are you giving for the lifetime of strings
passed to nd_set_link()? Right now it's "should not be freed until the matching ->put_link()"; what happens for RCU mode?
* really nasty one: creat(2) on a dangling symlink. What's to
preserve the last component if you get into that symlink in RCU mode?
TBH, I'm less than fond of passing nameidata to ->follow_link() at all, flags or no flags. We could kill current->link_count and current->total_link_count, replacing them with one void * current->nameidata and taking counters into struct nameidata itself. Have places like e.g. kern_path_locked() do
struct nameidata nd, *saved = set_nameidata(&nd);
...
set_nameidata(saved);
with set_nameidata(p) doing this:
old = current->nameidata;
current->nameidata = p;
if (p) {
if (!old) {
p->link_count = 0;
p->total_link_count = 0;
} else {
p->link_count = old->link_count;
p->total_link_count = old->total_link_count;
}
}
return old;Then nd_set_link() et.al. would use current->nameidata instead of an explicitly passed pointer and ->follow_link() instances wouldn't need that opaque pointer passed to them at all.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Al" == Al Viro <[email protected]> writes:
Quoted 10 lines · NeilBrown
Quoted 10 lines
Hi Al (and others),
I wonder if you could look over this patchset.
It allows RCU-walk to follow symlinks in many common cases, thus removing a surprising performance hit caused by using symlinks.The last could of patches make changes to XFS and NFS to support this but I haven't forwarded to the relevant lists yet. If/when the early code meets with approval I'll do that.
Quoted 5 lines
Quoted 5 lines
The first patch almost certainly needs to be changed. I originally wrote this code when filesystems could see inside nameidata. It is now opaque so the simplest solution was to provide an accessor function.
Maybe I should as a 'flags' arg to ->follow_link?? Or have
-> follow_link and ->follow_link_rcu ??
Quoted 1 line
Quoted 1 line
What do you suggest?
Al> Umm... Some observations:
Al> * now ->follow_link() can be called in RCU mode, which means
Al> that it can race with fs shutdown; not a problem, except that now it Al> joins ->lookup() et.al. in "if some data structure is needed in RCU Al> case of that, make sure it's not destroyed without an RCU delay somewhere Al> between the entry into ->kill_sb() and destruction.
Al> * highmem pages in symlinks: that BS shouldn't be allowed at
Al> all. Just make sure that at least for those filesystems symlink inodes Al> get mapping_set_gfp_mask(&inode->i_data, GFP_KERNEL) and be done with that.
So what happens if your filesystem is 10Tb in size, and you have 50 million files and lots of them are symlinks? I've got developers who do shit like this and wonder why performance sucks.... and I just worry that GPF_KERNEL is a limited resource. But maybe 64bit systems won't really have any problems?
Rest of this is way outside my pay grade to commment on.
Al Viro
maintainerQuoted 5 lines · John Stoffel
So what happens if your filesystem is 10Tb in size, and you have 50 million files and lots of them are symlinks? I've got developers who do shit like this and wonder why performance sucks.... and I just worry that
GPF_KERNELis a limited resource. But maybe 64bit systems won't really have any problems?
What would keep all those symlinks' contents pinned in page cache?
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Al" == Al Viro <[email protected]> writes:
Quoted 5 lines · John Stoffel
Quoted 5 lines
So what happens if your filesystem is 10Tb in size, and you have 50 million files and lots of them are symlinks? I've got developers who do shit like this and wonder why performance sucks.... and I just worry that
GPF_KERNELis a limited resource. But maybe 64bit systems won't really have any problems?
Al> What would keep all those symlinks' contents pinned in page cache?
Dunno honestly... but your statement nudges my memory that they would be evicted as memory pressure grows, so it's probably not a problem afterall. I just know that my users love tickling strange bugs like this because they hate to actually cleanup after themselves unless absolutely necessary.
Thanks for the nudge.
Quoted 58 lines · On Thu, 5 Mar 2015 06:05:20 +0000 Al Viro <[email protected]> wrote:
Quoted 10 lines · NeilBrown
Hi Al (and others),
I wonder if you could look over this patchset.
It allows RCU-walk to follow symlinks in many common cases, thus removing a surprising performance hit caused by using symlinks.The last could of patches make changes to XFS and NFS to support this but I haven't forwarded to the relevant lists yet. If/when the early code meets with approval I'll do that.
Quoted 7 lines
The first patch almost certainly needs to be changed. I originally wrote this code when filesystems could see inside nameidata. It is now opaque so the simplest solution was to provide an accessor function.
Maybe I should as a 'flags' arg to ->follow_link?? Or have ->follow_link and ->follow_link_rcu ??
What do you suggest?Umm... Some observations:
* now ->follow_link() can be called in RCU mode, which means
that it can race with fs shutdown; not a problem, except that now it joins ->lookup() et.al. in "if some data structure is needed in RCU case of that, make sure it's not destroyed without an RCU delay somewhere between the entry into ->kill_sb() and destruction.
* highmem pages in symlinks: that BS shouldn't be allowed at
all. Just make sure that at least for those filesystems symlink inodes getmapping_set_gfp_mask(&inode->i_data, GFP_KERNEL)and be done with that.
* are you sure thatsecurity_inode_follow_link()is OK to call in
RCU mode?
* what warranties are you giving for the lifetime of strings
passed tond_set_link()? Right now it's "should not be freed until the matching ->put_link()"; what happens for RCU mode?
* really nasty one: creat(2) on a dangling symlink. What's to
preserve the last component if you get into that symlink in RCU mode?TBH, I'm less than fond of passing nameidata to ->follow_link() at all, flags or no flags. We could kill
current->link_countandcurrent->total_link_count, replacing them with one void *current->nameidataand taking counters into struct nameidata itself. Have places like e.g.kern_path_locked()dostruct nameidata nd, *saved = set_nameidata(&nd); ... set_nameidata(saved); with set_nameidata(p) doing this: old = current->nameidata; current->nameidata = p; if (p) { if (!old) { p->link_count = 0; p->total_link_count = 0; } else { p->link_count = old->link_count; p->total_link_count = old->total_link_count; } } return old;
Quoted 3 lines
Then
nd_set_link()et.al. would usecurrent->nameidatainstead of an explicitly passed pointer and ->follow_link() instances wouldn't need that opaque pointer passed to them at all.
Wow, thanks for the quick response!!!
I'll look into all those issues and get back to you when I have something coherent to say.
NeilBrown
Quoted 19 lines · On Thu, 5 Mar 2015 06:05:20 +0000 Al Viro <[email protected]> wrote:
Quoted 10 lines · NeilBrown
Hi Al (and others),
I wonder if you could look over this patchset.
It allows RCU-walk to follow symlinks in many common cases, thus removing a surprising performance hit caused by using symlinks.The last could of patches make changes to XFS and NFS to support this but I haven't forwarded to the relevant lists yet. If/when the early code meets with approval I'll do that.
Quoted 7 lines
The first patch almost certainly needs to be changed. I originally wrote this code when filesystems could see inside nameidata. It is now opaque so the simplest solution was to provide an accessor function.
Maybe I should as a 'flags' arg to ->follow_link?? Or have ->follow_link and ->follow_link_rcu ??
What do you suggest?
Quoted 6 lines
Umm... Some observations:
* now ->follow_link() can be called in RCU mode, which means
that it can race with fs shutdown; not a problem, except that now it joins ->lookup() et.al. in "if some data structure is needed in RCU case of that, make sure it's not destroyed without an RCU delay somewhere between the entry into ->kill_sb() and destruction.
So inodes and dentries and associated private data should already be safe. And s_fs_info can be used if it is freed by e.g. kfree_rcu (like autofs) but not if just kfree (like ext3).
xfs_fs_put_super() directly frees the 'xfs_mount', which xfs_readlink accesses. I guess that needs to be fixed.
Quoted 3 lines
* highmem pages in symlinks: that BS shouldn't be allowed at
all. Just make sure that at least for those filesystems symlink inodes getmapping_set_gfp_mask(&inode->i_data, GFP_KERNEL)and be done with that.
page_getlink() already uses kmap(), implying that highmem pages are
supported. All I'm doing is making sure that my page_getlink_rcu()
doesn't fail horribly if the page is a highmem page.
If a filesystem needs improved follow_link performance on a highmem machine, then setting the gfp_mask as you suggest is probably a good idea, but I don't really want to impose that on filesystems if I don't need to. And at present I don't.
So I'd rather leave it to the filesystem maintainer, or someone who discovers a need.
Quoted 2 lines
* are you sure that
security_inode_follow_link()is OK to call in
RCU mode?
No.avc_has_perm() doesn't look RCU safe, even without auditing enabled. At the very least we'll need to pass a "lookup_rcu" flag in there.
Quoted 3 lines
* what warranties are you giving for the lifetime of strings
passed tond_set_link()? Right now it's "should not be freed until the matching ->put_link()"; what happens for RCU mode?
The same....
For XFS, we kmalloc a buffer GFP_ATOMIC and copy into that. Then put_link() kfrees it.
For filesystems with the symlink in the page cache, we get a reference to the page (which is a bit heavy-handed for RCU-walk, but much less so than the current code) and drop the reference in ->put_link.
For filesystems with a short symlink in the inode, we just provide a pointer to that... How long can we expect that to be around?
I cannot see any provision for keeping those inodes in memory while we follow the symlink... What am I missing?
In any case, if there is a reference held on the inode for ref-walk, then presumably complete_walk() will take a reference on that same inode when dropping out of rcu-walk.... I hope.
So I think the rules here are unchanged.
Quoted 2 lines
* really nasty one: creat(2) on a dangling symlink. What's to
preserve the last component if you get into that symlink in RCU mode?
As above - we will have a counted reference to whatever holds the text of the symlink.
Quoted 23 lines
TBH, I'm less than fond of passing nameidata to ->follow_link() at all, flags or no flags. We could kill
current->link_countandcurrent->total_link_count, replacing them with one void *current->nameidataand taking counters into struct nameidata itself. Have places like e.g.kern_path_locked()dostruct nameidata nd, *saved = set_nameidata(&nd); ... set_nameidata(saved); with set_nameidata(p) doing this: old = current->nameidata; current->nameidata = p; if (p) { if (!old) { p->link_count = 0; p->total_link_count = 0; } else { p->link_count = old->link_count; p->total_link_count = old->total_link_count; } } return old;
Quoted 3 lines
Then
nd_set_link()et.al. would usecurrent->nameidatainstead of an explicitly passed pointer and ->follow_link() instances wouldn't need that opaque pointer passed to them at all.
Sounds interesting - I might try it.
Would ->follow_link() than get a 'flags' argument, or would "nd_is_rcu()" reference current->nameidata->flags ??
Thanks,
NeilBrown