optimize symlink handling

optimize symlink handling

[Christoph Hellwig]

This series rewrite the highlevel XFS follow_link code.  Now long
symlinks will be put into the pagecache like most filesystem already
do, and inline symlinks can be directly returned to the VFS.  In
either case the kmalloc in ->follow_link is gone.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/6] xfs: factor out a helper to initialize a local format inode fork

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c    |  9 +++-----
 fs/xfs/libxfs/xfs_inode_fork.c | 48 ++++++++++++++++++++++++++----------------
 fs/xfs/libxfs/xfs_inode_fork.h |  1 +
 fs/xfs/xfs_symlink.c           | 12 ++---------
 4 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 4af3c23..bf83116 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -257,15 +257,12 @@ xfs_dir2_block_to_sf(
 	 *
 	 * Convert the inode to local format and copy the data in.
 	 */
-	dp->i_df.if_flags &= ~XFS_IFEXTENTS;
-	dp->i_df.if_flags |= XFS_IFINLINE;
-	dp->i_d.di_format = XFS_DINODE_FMT_LOCAL;
 	ASSERT(dp->i_df.if_bytes == 0);
-	xfs_idata_realloc(dp, size, XFS_DATA_FORK);
+	xfs_init_local_fork(dp, XFS_DATA_FORK, dst, size);
+	dp->i_d.di_format = XFS_DINODE_FMT_LOCAL;
+	dp->i_d.di_size = size;
 
 	logflags |= XFS_ILOG_DDATA;
-	memcpy(dp->i_df.if_u1.if_data, dst, size);
-	dp->i_d.di_size = size;
 	xfs_dir2_sf_check(args);
 out:
 	xfs_trans_log_inode(args->trans, dp, logflags);
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 9d2b716..86a3e11 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -230,6 +230,34 @@ xfs_iformat_fork(
 	return error;
 }
 
+void
+xfs_init_local_fork(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	const void		*data,
+	int			size)
+{
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	int			real_size = 0;
+
+	if (size == 0)
+		ifp->if_u1.if_data = NULL;
+	else if (size <= sizeof(ifp->if_u2.if_inline_data))
+		ifp->if_u1.if_data = ifp->if_u2.if_inline_data;
+	else {
+		real_size = roundup(size, 4);
+		ifp->if_u1.if_data = kmem_alloc(real_size, KM_SLEEP | KM_NOFS);
+	}
+
+	if (size)
+		memcpy(ifp->if_u1.if_data, data, size);
+
+	ifp->if_bytes = size;
+	ifp->if_real_bytes = real_size;
+	ifp->if_flags &= ~(XFS_IFEXTENTS | XFS_IFBROOT);
+	ifp->if_flags |= XFS_IFINLINE;
+}
+
 /*
  * The file is in-lined in the on-disk inode.
  * If it fits into if_inline_data, then copy
@@ -247,8 +275,6 @@ xfs_iformat_local(
 	int		whichfork,
 	int		size)
 {
-	xfs_ifork_t	*ifp;
-	int		real_size;
 
 	/*
 	 * If the size is unreasonable, then something
@@ -264,22 +290,8 @@ xfs_iformat_local(
 				     ip->i_mount, dip);
 		return -EFSCORRUPTED;
 	}
-	ifp = XFS_IFORK_PTR(ip, whichfork);
-	real_size = 0;
-	if (size == 0)
-		ifp->if_u1.if_data = NULL;
-	else if (size <= sizeof(ifp->if_u2.if_inline_data))
-		ifp->if_u1.if_data = ifp->if_u2.if_inline_data;
-	else {
-		real_size = roundup(size, 4);
-		ifp->if_u1.if_data = kmem_alloc(real_size, KM_SLEEP | KM_NOFS);
-	}
-	ifp->if_bytes = size;
-	ifp->if_real_bytes = real_size;
-	if (size)
-		memcpy(ifp->if_u1.if_data, XFS_DFORK_PTR(dip, whichfork), size);
-	ifp->if_flags &= ~XFS_IFEXTENTS;
-	ifp->if_flags |= XFS_IFINLINE;
+
+	xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 749fd5a..dfb2966 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -135,6 +135,7 @@ void		xfs_iroot_realloc(struct xfs_inode *, int, int);
 int		xfs_iread_extents(struct xfs_trans *, struct xfs_inode *, int);
 int		xfs_iextents_copy(struct xfs_inode *, struct xfs_bmbt_rec *,
 				  int);
+void		xfs_init_local_fork(struct xfs_inode *, int, const void *, int);
 
 struct xfs_bmbt_rec_host *
 		xfs_iext_get_ext(struct xfs_ifork *, xfs_extnum_t);
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index f9ee5fe..7fa94dc 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -306,19 +306,11 @@ xfs_symlink(
 	 * If the symlink will fit into the inode, write it inline.
 	 */
 	if (pathlen <= XFS_IFORK_DSIZE(ip)) {
-		xfs_idata_realloc(ip, pathlen, XFS_DATA_FORK);
-		memcpy(ip->i_df.if_u1.if_data, target_path, pathlen);
-		ip->i_d.di_size = pathlen;
-
-		/*
-		 * The inode was initially created in extent format.
-		 */
-		ip->i_df.if_flags &= ~(XFS_IFEXTENTS | XFS_IFBROOT);
-		ip->i_df.if_flags |= XFS_IFINLINE;
+		xfs_init_local_fork(ip, XFS_DATA_FORK, target_path, pathlen);
 
+		ip->i_d.di_size = pathlen;
 		ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
 		xfs_trans_log_inode(tp, ip, XFS_ILOG_DDATA | XFS_ILOG_CORE);
-
 	} else {
 		int	offset;
 
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/6] xfs: set up inode operation vectors later

[Christoph Hellwig]

In the next patch we'll set up different inode operations for inline vs
out of line symlinks, for that we need to make sure the flags are already
set up properly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode.h |  5 ++++-
 fs/xfs/xfs_iops.c  | 59 ++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 8f22d20..d49e293 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -407,6 +407,9 @@ int	xfs_iozero(struct xfs_inode *ip, loff_t pos, size_t count);
 
 
 /* from xfs_iops.c */
+extern void xfs_setup_inode(struct xfs_inode *ip);
+extern void xfs_setup_iops(struct xfs_inode *ip);
+
 /*
  * When setting up a newly allocated inode, we need to call
  * xfs_finish_inode_setup() once the inode is fully instantiated at
@@ -414,7 +417,6 @@ int	xfs_iozero(struct xfs_inode *ip, loff_t pos, size_t count);
  * before we've completed instantiation. Otherwise we can do it
  * the moment the inode lookup is complete.
  */
-extern void xfs_setup_inode(struct xfs_inode *ip);
 static inline void xfs_finish_inode_setup(struct xfs_inode *ip)
 {
 	xfs_iflags_clear(ip, XFS_INEW);
@@ -425,6 +427,7 @@ static inline void xfs_finish_inode_setup(struct xfs_inode *ip)
 static inline void xfs_setup_existing_inode(struct xfs_inode *ip)
 {
 	xfs_setup_inode(ip);
+	xfs_setup_iops(ip);
 	xfs_finish_inode_setup(ip);
 }
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 2f1839e..d3505ff 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -182,6 +182,8 @@ xfs_generic_create(
 	}
 #endif
 
+	xfs_setup_iops(ip);
+
 	if (tmpfile)
 		d_tmpfile(dentry, inode);
 	else
@@ -369,6 +371,8 @@ xfs_vn_symlink(
 	if (unlikely(error))
 		goto out_cleanup_inode;
 
+	xfs_setup_iops(cip);
+
 	d_instantiate(dentry, inode);
 	xfs_finish_inode_setup(cip);
 	return 0;
@@ -1210,7 +1214,7 @@ xfs_diflags_to_iflags(
 }
 
 /*
- * Initialize the Linux inode and set up the operation vectors.
+ * Initialize the Linux inode.
  *
  * When reading existing inodes from disk this is called directly from xfs_iget,
  * when creating a new inode it is called from xfs_ialloc after setting up the
@@ -1258,8 +1262,38 @@ xfs_setup_inode(
 	inode->i_ctime.tv_nsec	= ip->i_d.di_ctime.t_nsec;
 	xfs_diflags_to_iflags(inode, ip);
 
-	ip->d_ops = ip->i_mount->m_nondir_inode_ops;
-	lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class);
+	if (S_ISDIR(inode->i_mode)) {
+		lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class);
+		ip->d_ops = ip->i_mount->m_dir_inode_ops;
+	} else {
+		ip->d_ops = ip->i_mount->m_nondir_inode_ops;
+		lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class);
+	}
+
+	/*
+	 * Ensure all page cache allocations are done from GFP_NOFS context to
+	 * prevent direct reclaim recursion back into the filesystem and blowing
+	 * stacks or deadlocking.
+	 */
+	gfp_mask = mapping_gfp_mask(inode->i_mapping);
+	mapping_set_gfp_mask(inode->i_mapping, (gfp_mask & ~(__GFP_FS)));
+
+	/*
+	 * If there is no attribute fork no ACL can exist on this inode,
+	 * and it can't have any file capabilities attached to it either.
+	 */
+	if (!XFS_IFORK_Q(ip)) {
+		inode_has_no_xattr(inode);
+		cache_no_acl(inode);
+	}
+}
+
+void
+xfs_setup_iops(
+	struct xfs_inode	*ip)
+{
+	struct inode		*inode = &ip->i_vnode;
+
 	switch (inode->i_mode & S_IFMT) {
 	case S_IFREG:
 		inode->i_op = &xfs_inode_operations;
@@ -1267,13 +1301,11 @@ xfs_setup_inode(
 		inode->i_mapping->a_ops = &xfs_address_space_operations;
 		break;
 	case S_IFDIR:
-		lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class);
 		if (xfs_sb_version_hasasciici(&XFS_M(inode->i_sb)->m_sb))
 			inode->i_op = &xfs_dir_ci_inode_operations;
 		else
 			inode->i_op = &xfs_dir_inode_operations;
 		inode->i_fop = &xfs_dir_file_operations;
-		ip->d_ops = ip->i_mount->m_dir_inode_ops;
 		break;
 	case S_IFLNK:
 		inode->i_op = &xfs_symlink_inode_operations;
@@ -1285,21 +1317,4 @@ xfs_setup_inode(
 		init_special_inode(inode, inode->i_mode, inode->i_rdev);
 		break;
 	}
-
-	/*
-	 * Ensure all page cache allocations are done from GFP_NOFS context to
-	 * prevent direct reclaim recursion back into the filesystem and blowing
-	 * stacks or deadlocking.
-	 */
-	gfp_mask = mapping_gfp_mask(inode->i_mapping);
-	mapping_set_gfp_mask(inode->i_mapping, (gfp_mask & ~(__GFP_FS)));
-
-	/*
-	 * If there is no attribute fork no ACL can exist on this inode,
-	 * and it can't have any file capabilities attached to it either.
-	 */
-	if (!XFS_IFORK_Q(ip)) {
-		inode_has_no_xattr(inode);
-		cache_no_acl(inode);
-	}
 }
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/6] xfs: use ->readlink to implement the readlink_by_handle ioctl

[Christoph Hellwig]

Also drop the now unused readlink_copy export.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/namei.c         |  1 -
 fs/xfs/xfs_ioctl.c | 19 ++-----------------
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c83145a..482c6a9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4420,7 +4420,6 @@ int readlink_copy(char __user *buffer, int buflen, const char *link)
 out:
 	return len;
 }
-EXPORT_SYMBOL(readlink_copy);
 
 /*
  * A helper for ->readlink().  This should be used *ONLY* for symlinks that
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5f4a396..597609e 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -276,7 +276,6 @@ xfs_readlink_by_handle(
 {
 	struct dentry		*dentry;
 	__u32			olen;
-	void			*link;
 	int			error;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -287,7 +286,7 @@ xfs_readlink_by_handle(
 		return PTR_ERR(dentry);
 
 	/* Restrict this handle operation to symlinks only. */
-	if (!d_is_symlink(dentry)) {
+	if (!dentry->d_inode->i_op->readlink) {
 		error = -EINVAL;
 		goto out_dput;
 	}
@@ -297,21 +296,7 @@ xfs_readlink_by_handle(
 		goto out_dput;
 	}
 
-	link = kmalloc(MAXPATHLEN+1, GFP_KERNEL);
-	if (!link) {
-		error = -ENOMEM;
-		goto out_dput;
-	}
-
-	error = xfs_readlink(XFS_I(dentry->d_inode), link);
-	if (error)
-		goto out_kfree;
-	error = readlink_copy(hreq->ohandle, olen, link);
-	if (error)
-		goto out_kfree;
-
- out_kfree:
-	kfree(link);
+	error = dentry->d_inode->i_op->readlink(dentry, hreq->ohandle, olen);
  out_dput:
 	dput(dentry);
 	return error;
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Christoph Hellwig]

We can use the generic symlink in pagecache code for XFS non-inline
symlinks.  Because links are always shorter than a page we will
get the zero termination for the link for free.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iops.c    |  40 ++++++++++++++---
 fs/xfs/xfs_symlink.c | 120 ---------------------------------------------------
 fs/xfs/xfs_symlink.h |   1 -
 fs/xfs/xfs_trace.h   |   1 -
 4 files changed, 34 insertions(+), 128 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index d3505ff..57c0998 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -419,21 +419,32 @@ xfs_vn_rename(
  * uio is kmalloced for this reason...
  */
 STATIC void *
-xfs_vn_follow_link(
+xfs_vn_follow_link_inline(
 	struct dentry		*dentry,
 	struct nameidata	*nd)
 {
+	struct xfs_inode	*ip = XFS_I(dentry->d_inode);
+	xfs_fsize_t		pathlen;
 	char			*link;
 	int			error = -ENOMEM;
 
+	error = -ENOMEM;
 	link = kmalloc(MAXPATHLEN+1, GFP_KERNEL);
 	if (!link)
 		goto out_err;
 
-	error = xfs_readlink(XFS_I(dentry->d_inode), link);
-	if (unlikely(error))
+	error = -EIO;
+	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) 
 		goto out_kfree;
 
+	xfs_ilock(ip, XFS_ILOCK_SHARED);
+	pathlen = ip->i_d.di_size;
+	if (pathlen) {
+		memcpy(link, ip->i_df.if_u1.if_data, pathlen);
+		link[pathlen] = '\0';
+	}
+	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+
 	nd_set_link(nd, link);
 	return NULL;
 
@@ -1179,7 +1190,20 @@ static const struct inode_operations xfs_dir_ci_inode_operations = {
 
 static const struct inode_operations xfs_symlink_inode_operations = {
 	.readlink		= generic_readlink,
-	.follow_link		= xfs_vn_follow_link,
+	.follow_link		= page_follow_link_light,
+	.put_link		= page_put_link,
+	.getattr		= xfs_vn_getattr,
+	.setattr		= xfs_vn_setattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
+	.listxattr		= xfs_vn_listxattr,
+	.update_time		= xfs_vn_update_time,
+};
+
+static const struct inode_operations xfs_inline_symlink_inode_operations = {
+	.readlink		= generic_readlink,
+	.follow_link		= xfs_vn_follow_link_inline,
 	.put_link		= kfree_put_link,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
@@ -1190,6 +1214,7 @@ static const struct inode_operations xfs_symlink_inode_operations = {
 	.update_time		= xfs_vn_update_time,
 };
 
+
 STATIC void
 xfs_diflags_to_iflags(
 	struct inode		*inode,
@@ -1308,9 +1333,12 @@ xfs_setup_iops(
 		inode->i_fop = &xfs_dir_file_operations;
 		break;
 	case S_IFLNK:
-		inode->i_op = &xfs_symlink_inode_operations;
-		if (!(ip->i_df.if_flags & XFS_IFINLINE))
+		if (ip->i_df.if_flags & XFS_IFINLINE) {
+			inode->i_op = &xfs_inline_symlink_inode_operations;
+		} else {
 			inode->i_mapping->a_ops = &xfs_address_space_operations;
+			inode->i_op = &xfs_symlink_inode_operations;
+		}
 		break;
 	default:
 		inode->i_op = &xfs_inode_operations;
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 7fa94dc..c1d7775 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -42,126 +42,6 @@
 #include "xfs_log.h"
 
 /* ----- Kernel only functions below ----- */
-STATIC int
-xfs_readlink_bmap(
-	struct xfs_inode	*ip,
-	char			*link)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	struct xfs_bmbt_irec	mval[XFS_SYMLINK_MAPS];
-	struct xfs_buf		*bp;
-	xfs_daddr_t		d;
-	char			*cur_chunk;
-	int			pathlen = ip->i_d.di_size;
-	int			nmaps = XFS_SYMLINK_MAPS;
-	int			byte_cnt;
-	int			n;
-	int			error = 0;
-	int			fsblocks = 0;
-	int			offset;
-
-	fsblocks = xfs_symlink_blocks(mp, pathlen);
-	error = xfs_bmapi_read(ip, 0, fsblocks, mval, &nmaps, 0);
-	if (error)
-		goto out;
-
-	offset = 0;
-	for (n = 0; n < nmaps; n++) {
-		d = XFS_FSB_TO_DADDR(mp, mval[n].br_startblock);
-		byte_cnt = XFS_FSB_TO_B(mp, mval[n].br_blockcount);
-
-		bp = xfs_buf_read(mp->m_ddev_targp, d, BTOBB(byte_cnt), 0,
-				  &xfs_symlink_buf_ops);
-		if (!bp)
-			return -ENOMEM;
-		error = bp->b_error;
-		if (error) {
-			xfs_buf_ioerror_alert(bp, __func__);
-			xfs_buf_relse(bp);
-
-			/* bad CRC means corrupted metadata */
-			if (error == -EFSBADCRC)
-				error = -EFSCORRUPTED;
-			goto out;
-		}
-		byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt);
-		if (pathlen < byte_cnt)
-			byte_cnt = pathlen;
-
-		cur_chunk = bp->b_addr;
-		if (xfs_sb_version_hascrc(&mp->m_sb)) {
-			if (!xfs_symlink_hdr_ok(ip->i_ino, offset,
-							byte_cnt, bp)) {
-				error = -EFSCORRUPTED;
-				xfs_alert(mp,
-"symlink header does not match required off/len/owner (0x%x/Ox%x,0x%llx)",
-					offset, byte_cnt, ip->i_ino);
-				xfs_buf_relse(bp);
-				goto out;
-
-			}
-
-			cur_chunk += sizeof(struct xfs_dsymlink_hdr);
-		}
-
-		memcpy(link + offset, bp->b_addr, byte_cnt);
-
-		pathlen -= byte_cnt;
-		offset += byte_cnt;
-
-		xfs_buf_relse(bp);
-	}
-	ASSERT(pathlen == 0);
-
-	link[ip->i_d.di_size] = '\0';
-	error = 0;
-
- out:
-	return error;
-}
-
-int
-xfs_readlink(
-	struct xfs_inode *ip,
-	char		*link)
-{
-	struct xfs_mount *mp = ip->i_mount;
-	xfs_fsize_t	pathlen;
-	int		error = 0;
-
-	trace_xfs_readlink(ip);
-
-	if (XFS_FORCED_SHUTDOWN(mp))
-		return -EIO;
-
-	xfs_ilock(ip, XFS_ILOCK_SHARED);
-
-	pathlen = ip->i_d.di_size;
-	if (!pathlen)
-		goto out;
-
-	if (pathlen < 0 || pathlen > MAXPATHLEN) {
-		xfs_alert(mp, "%s: inode (%llu) bad symlink length (%lld)",
-			 __func__, (unsigned long long) ip->i_ino,
-			 (long long) pathlen);
-		ASSERT(0);
-		error = -EFSCORRUPTED;
-		goto out;
-	}
-
-
-	if (ip->i_df.if_flags & XFS_IFINLINE) {
-		memcpy(link, ip->i_df.if_u1.if_data, pathlen);
-		link[pathlen] = '\0';
-	} else {
-		error = xfs_readlink_bmap(ip, link);
-	}
-
- out:
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
-	return error;
-}
-
 int
 xfs_symlink(
 	struct xfs_inode	*dp,
diff --git a/fs/xfs/xfs_symlink.h b/fs/xfs/xfs_symlink.h
index e75245d..d6816af 100644
--- a/fs/xfs/xfs_symlink.h
+++ b/fs/xfs/xfs_symlink.h
@@ -21,7 +21,6 @@
 
 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_inactive_symlink(struct xfs_inode *ip);
 
 #endif /* __XFS_SYMLINK_H */
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 615781b..6aaff5c 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -658,7 +658,6 @@ DEFINE_INODE_EVENT(xfs_iget_miss);
 
 DEFINE_INODE_EVENT(xfs_getattr);
 DEFINE_INODE_EVENT(xfs_setattr);
-DEFINE_INODE_EVENT(xfs_readlink);
 DEFINE_INODE_EVENT(xfs_inactive_symlink);
 DEFINE_INODE_EVENT(xfs_alloc_file_space);
 DEFINE_INODE_EVENT(xfs_free_file_space);
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 6/6] xfs: optimize inline symlinks

[Christoph Hellwig]

By overallocating the in-core inode fork data buffer and zero
terminating the link target in xfs_init_local_fork we can avoid
the memory allocation in ->follow_link.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_dir2_sf.c    |  2 +-
 fs/xfs/libxfs/xfs_inode_fork.c | 29 +++++++++++++++++++----------
 fs/xfs/libxfs/xfs_inode_fork.h |  3 ++-
 fs/xfs/xfs_inode_item.c        |  4 ++--
 fs/xfs/xfs_iops.c              | 36 +-----------------------------------
 fs/xfs/xfs_symlink.c           |  5 +++--
 6 files changed, 28 insertions(+), 51 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index bf83116..a438058 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -258,7 +258,7 @@ xfs_dir2_block_to_sf(
 	 * Convert the inode to local format and copy the data in.
 	 */
 	ASSERT(dp->i_df.if_bytes == 0);
-	xfs_init_local_fork(dp, XFS_DATA_FORK, dst, size);
+	xfs_init_local_fork(dp, XFS_DATA_FORK, dst, size, false);
 	dp->i_d.di_format = XFS_DINODE_FMT_LOCAL;
 	dp->i_d.di_size = size;
 
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 86a3e11..dcadd07 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -235,22 +235,29 @@ xfs_init_local_fork(
 	struct xfs_inode	*ip,
 	int			whichfork,
 	const void		*data,
-	int			size)
+	int			size,
+	bool			zero_terminate)
 {
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
-	int			real_size = 0;
+	int			mem_size = size, real_size = 0;
+
+	if (zero_terminate)
+		mem_size = size + 1;
 
 	if (size == 0)
 		ifp->if_u1.if_data = NULL;
-	else if (size <= sizeof(ifp->if_u2.if_inline_data))
+	else if (mem_size <= sizeof(ifp->if_u2.if_inline_data))
 		ifp->if_u1.if_data = ifp->if_u2.if_inline_data;
 	else {
-		real_size = roundup(size, 4);
+		real_size = roundup(mem_size, 4);
 		ifp->if_u1.if_data = kmem_alloc(real_size, KM_SLEEP | KM_NOFS);
 	}
 
-	if (size)
+	if (size) {
 		memcpy(ifp->if_u1.if_data, data, size);
+		if (zero_terminate)
+			ifp->if_u1.if_data[size] = '\0';
+	}
 
 	ifp->if_bytes = size;
 	ifp->if_real_bytes = real_size;
@@ -270,11 +277,12 @@ xfs_init_local_fork(
  */
 STATIC int
 xfs_iformat_local(
-	xfs_inode_t	*ip,
-	xfs_dinode_t	*dip,
-	int		whichfork,
-	int		size)
+	struct xfs_inode	*ip,
+	struct xfs_dinode	*dip,
+	int			whichfork,
+	int			size)
 {
+	bool			zero_terminate = S_ISDIR(dip->di_mode);
 
 	/*
 	 * If the size is unreasonable, then something
@@ -291,7 +299,8 @@ xfs_iformat_local(
 		return -EFSCORRUPTED;
 	}
 
-	xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
+	xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size,
+			zero_terminate);
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index dfb2966..f5f640f 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -135,7 +135,8 @@ void		xfs_iroot_realloc(struct xfs_inode *, int, int);
 int		xfs_iread_extents(struct xfs_trans *, struct xfs_inode *, int);
 int		xfs_iextents_copy(struct xfs_inode *, struct xfs_bmbt_rec *,
 				  int);
-void		xfs_init_local_fork(struct xfs_inode *, int, const void *, int);
+void		xfs_init_local_fork(struct xfs_inode *, int, const void *, int,
+			bool);
 
 struct xfs_bmbt_rec_host *
 		xfs_iext_get_ext(struct xfs_ifork *, xfs_extnum_t);
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index bf13a5a..786d91a 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -210,7 +210,7 @@ xfs_inode_item_format_data_fork(
 			 */
 			data_bytes = roundup(ip->i_df.if_bytes, 4);
 			ASSERT(ip->i_df.if_real_bytes == 0 ||
-			       ip->i_df.if_real_bytes == data_bytes);
+			       ip->i_df.if_real_bytes >= data_bytes);
 			ASSERT(ip->i_df.if_u1.if_data != NULL);
 			ASSERT(ip->i_d.di_size > 0);
 			xlog_copy_iovec(lv, vecp, XLOG_REG_TYPE_ILOCAL,
@@ -305,7 +305,7 @@ xfs_inode_item_format_attr_fork(
 			 */
 			data_bytes = roundup(ip->i_afp->if_bytes, 4);
 			ASSERT(ip->i_afp->if_real_bytes == 0 ||
-			       ip->i_afp->if_real_bytes == data_bytes);
+			       ip->i_afp->if_real_bytes >= data_bytes);
 			ASSERT(ip->i_afp->if_u1.if_data != NULL);
 			xlog_copy_iovec(lv, vecp, XLOG_REG_TYPE_IATTR_LOCAL,
 					ip->i_afp->if_u1.if_data,
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 57c0998..89e0cc9 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -413,45 +413,12 @@ xfs_vn_rename(
 			  new_inode ? XFS_I(new_inode) : NULL, flags);
 }
 
-/*
- * careful here - this function can get called recursively, so
- * we need to be very careful about how much stack we use.
- * uio is kmalloced for this reason...
- */
 STATIC void *
 xfs_vn_follow_link_inline(
 	struct dentry		*dentry,
 	struct nameidata	*nd)
 {
-	struct xfs_inode	*ip = XFS_I(dentry->d_inode);
-	xfs_fsize_t		pathlen;
-	char			*link;
-	int			error = -ENOMEM;
-
-	error = -ENOMEM;
-	link = kmalloc(MAXPATHLEN+1, GFP_KERNEL);
-	if (!link)
-		goto out_err;
-
-	error = -EIO;
-	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) 
-		goto out_kfree;
-
-	xfs_ilock(ip, XFS_ILOCK_SHARED);
-	pathlen = ip->i_d.di_size;
-	if (pathlen) {
-		memcpy(link, ip->i_df.if_u1.if_data, pathlen);
-		link[pathlen] = '\0';
-	}
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
-
-	nd_set_link(nd, link);
-	return NULL;
-
- out_kfree:
-	kfree(link);
- out_err:
-	nd_set_link(nd, ERR_PTR(error));
+	nd_set_link(nd, XFS_I(dentry->d_inode)->i_df.if_u1.if_data);
 	return NULL;
 }
 
@@ -1204,7 +1171,6 @@ static const struct inode_operations xfs_symlink_inode_operations = {
 static const struct inode_operations xfs_inline_symlink_inode_operations = {
 	.readlink		= generic_readlink,
 	.follow_link		= xfs_vn_follow_link_inline,
-	.put_link		= kfree_put_link,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
 	.setxattr		= generic_setxattr,
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index c1d7775..d6b59f6 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -182,12 +182,13 @@ xfs_symlink(
 
 	if (resblks)
 		resblks -= XFS_IALLOC_SPACE_RES(mp);
+
 	/*
 	 * If the symlink will fit into the inode, write it inline.
 	 */
 	if (pathlen <= XFS_IFORK_DSIZE(ip)) {
-		xfs_init_local_fork(ip, XFS_DATA_FORK, target_path, pathlen);
-
+		xfs_init_local_fork(ip, XFS_DATA_FORK, target_path, pathlen,
+				true);
 		ip->i_d.di_size = pathlen;
 		ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
 		xfs_trans_log_inode(tp, ip, XFS_ILOG_DDATA | XFS_ILOG_CORE);
-- 
1.9.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Dave Chinner]

On Thu, Apr 23, 2015 at 09:07:43PM +0200, Christoph Hellwig wrote:
> We can use the generic symlink in pagecache code for XFS non-inline
> symlinks.  Because links are always shorter than a page we will
> get the zero termination for the link for free.

Doesn't work for v5 filesystems where headers and CRCs are embedded
into the same blocks as the symlink data. i.e. this now falls down
to ->readpage to read the link data into the page cache, and that
just reads the entire blocks into the page cache. So what we can end
up within the page cache is this:

   4k block/page
+---|--------------+
 hdr  symlink data

And for a 1k block size v5 filesystem it could be this within a 4k
page:

    1k block            1k block
+---|--------------+----|---------------+
 hdr  symlink data   hdr   symlink data

Either way, it will be completely misinterpretted by the generic
symlink code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Christoph Hellwig]

On Fri, Apr 24, 2015 at 08:29:42AM +1000, Dave Chinner wrote:
> On Thu, Apr 23, 2015 at 09:07:43PM +0200, Christoph Hellwig wrote:
> > We can use the generic symlink in pagecache code for XFS non-inline
> > symlinks.  Because links are always shorter than a page we will
> > get the zero termination for the link for free.
> 
> Doesn't work for v5 filesystems where headers and CRCs are embedded
> into the same blocks as the symlink data. i.e. this now falls down
> to ->readpage to read the link data into the page cache, and that
> just reads the entire blocks into the page cache. So what we can end
> up within the page cache is this:

Uh, ok.  Guess the pagecache symlink support isn't too useful then.
I'll respin to only optimize the inline symlinks, which should
still we worthwhile.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Christoph Hellwig]

Al, what do you think about adding a new

i_link member to the union of i_pipe, i_bdev and i_cdev.  That we
we can cache a link acquired by any way for direct use in the VFS.

This has a few use cases:  inline links can be set up directly
when reading the inode, and we never need to call into ->follow_link.

Formats like the XFS v5 symlinks can be read in once by whatever
way we want, and following accesses can be done RCU safe and
without calling into the filesystem.

Note that caching the symlink in a kmalloc'ed buffer might be
more efficient than the pagecache for most cases anyway.

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Christoph Hellwig]

Al, what do you think about adding a new

i_link member to the union of i_pipe, i_bdev and i_cdev.  That we
we can cache a link acquired by any way for direct use in the VFS.

This has a few use cases:  inline links can be set up directly
when reading the inode, and we never need to call into ->follow_link.

Formats like the XFS v5 symlinks can be read in once by whatever
way we want, and following accesses can be done RCU safe and
without calling into the filesystem.

Note that caching the symlink in a kmalloc'ed buffer might be
more efficient than the pagecache for most cases anyway.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Al Viro]

On Sat, Apr 25, 2015 at 04:16:12PM +0200, Christoph Hellwig wrote:
> Al, what do you think about adding a new
> 
> i_link member to the union of i_pipe, i_bdev and i_cdev.  That we
> we can cache a link acquired by any way for direct use in the VFS.
> 
> This has a few use cases:  inline links can be set up directly
> when reading the inode, and we never need to call into ->follow_link.
> 
> Formats like the XFS v5 symlinks can be read in once by whatever
> way we want, and following accesses can be done RCU safe and
> without calling into the filesystem.
> 
> Note that caching the symlink in a kmalloc'ed buffer might be
> more efficient than the pagecache for most cases anyway.

Hmm...  When would you free the sucker?

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Al Viro]

On Sat, Apr 25, 2015 at 04:16:12PM +0200, Christoph Hellwig wrote:
> Al, what do you think about adding a new
> 
> i_link member to the union of i_pipe, i_bdev and i_cdev.  That we
> we can cache a link acquired by any way for direct use in the VFS.
> 
> This has a few use cases:  inline links can be set up directly
> when reading the inode, and we never need to call into ->follow_link.
> 
> Formats like the XFS v5 symlinks can be read in once by whatever
> way we want, and following accesses can be done RCU safe and
> without calling into the filesystem.
> 
> Note that caching the symlink in a kmalloc'ed buffer might be
> more efficient than the pagecache for most cases anyway.

Hmm...  When would you free the sucker?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Al Viro]

On Sat, Apr 25, 2015 at 03:57:28PM +0100, Al Viro wrote:
> > i_link member to the union of i_pipe, i_bdev and i_cdev.  That we
> > we can cache a link acquired by any way for direct use in the VFS.
> > 
> > This has a few use cases:  inline links can be set up directly
> > when reading the inode, and we never need to call into ->follow_link.
> > 
> > Formats like the XFS v5 symlinks can be read in once by whatever
> > way we want, and following accesses can be done RCU safe and
> > without calling into the filesystem.
> > 
> > Note that caching the symlink in a kmalloc'ed buffer might be
> > more efficient than the pagecache for most cases anyway.
> 
> Hmm...  When would you free the sucker?

FWIW, I'm not particularly opposed to doing that, but we'd better be careful
about not losing ->follow_link() itself.  Reason: we use its presence to
tell symlinks from non-symlinks.  OTOH, something like

	/* have already decided it's a symlink */
	if (inode->i_link)
		return inode->i_link;
	res = inode->i_op->follow_link(...);
with ->follow_link() instance returning ERR_PTR(-EIO) would work.  Such
sucker could live in fs/libfs.c just fine, with rule being "if you use it
for ->follow_link(), you'd better set ->i_link"...

Note, BTW, that there are symlinks where we _do_ have "traverse a string"
for semantics, and it's even kmalloc'ed, but we very much do not want it
to be cached.  Consider /proc/self, for example.  Different processes should
see different link bodies there, without any serialization between them.

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Al Viro]

On Sat, Apr 25, 2015 at 03:57:28PM +0100, Al Viro wrote:
> > i_link member to the union of i_pipe, i_bdev and i_cdev.  That we
> > we can cache a link acquired by any way for direct use in the VFS.
> > 
> > This has a few use cases:  inline links can be set up directly
> > when reading the inode, and we never need to call into ->follow_link.
> > 
> > Formats like the XFS v5 symlinks can be read in once by whatever
> > way we want, and following accesses can be done RCU safe and
> > without calling into the filesystem.
> > 
> > Note that caching the symlink in a kmalloc'ed buffer might be
> > more efficient than the pagecache for most cases anyway.
> 
> Hmm...  When would you free the sucker?

FWIW, I'm not particularly opposed to doing that, but we'd better be careful
about not losing ->follow_link() itself.  Reason: we use its presence to
tell symlinks from non-symlinks.  OTOH, something like

	/* have already decided it's a symlink */
	if (inode->i_link)
		return inode->i_link;
	res = inode->i_op->follow_link(...);
with ->follow_link() instance returning ERR_PTR(-EIO) would work.  Such
sucker could live in fs/libfs.c just fine, with rule being "if you use it
for ->follow_link(), you'd better set ->i_link"...

Note, BTW, that there are symlinks where we _do_ have "traverse a string"
for semantics, and it's even kmalloc'ed, but we very much do not want it
to be cached.  Consider /proc/self, for example.  Different processes should
see different link bodies there, without any serialization between them.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Christoph Hellwig]

On Sat, Apr 25, 2015 at 03:57:28PM +0100, Al Viro wrote:
> > Note that caching the symlink in a kmalloc'ed buffer might be
> > more efficient than the pagecache for most cases anyway.
> 
> Hmm...  When would you free the sucker?

final iput.  Similar design to the generic ACL cache.

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Christoph Hellwig]

On Sat, Apr 25, 2015 at 03:57:28PM +0100, Al Viro wrote:
> > Note that caching the symlink in a kmalloc'ed buffer might be
> > more efficient than the pagecache for most cases anyway.
> 
> Hmm...  When would you free the sucker?

final iput.  Similar design to the generic ACL cache.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Al Viro]

On Sat, Apr 25, 2015 at 08:32:47PM +0200, Christoph Hellwig wrote:
> On Sat, Apr 25, 2015 at 03:57:28PM +0100, Al Viro wrote:
> > > Note that caching the symlink in a kmalloc'ed buffer might be
> > > more efficient than the pagecache for most cases anyway.
> > 
> > Hmm...  When would you free the sucker?
> 
> final iput.  Similar design to the generic ACL cache.

Except that in this case you have to deal with the cases when it should
_not_ be freed in ->evict_inode() (and doing that in generic code is
right out).  I'm not sure it will be simpler that way, actually...

Re: [PATCH 5/6] xfs: move non-inline symlinks to the pagecache

[Al Viro]

On Sat, Apr 25, 2015 at 08:32:47PM +0200, Christoph Hellwig wrote:
> On Sat, Apr 25, 2015 at 03:57:28PM +0100, Al Viro wrote:
> > > Note that caching the symlink in a kmalloc'ed buffer might be
> > > more efficient than the pagecache for most cases anyway.
> > 
> > Hmm...  When would you free the sucker?
> 
> final iput.  Similar design to the generic ACL cache.

Except that in this case you have to deal with the cases when it should
_not_ be freed in ->evict_inode() (and doing that in generic code is
right out).  I'm not sure it will be simpler that way, actually...

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs