Skip to contentSkip to conversation

simplify validata_fields

PATCH In review Christoph Hellwig 2 participants 4 messages Sep 14, 2007 – Sep 18, 2007 XFS filesystem

lore
Share & tools
lore
#0 lore raw

PATCH 1/4 simplify validata_fields

Stop using xfs_getattr and a onstack bhv_vattr_t just to get three fields from the underlying inode and opencode copying from the inode fields instead.

fs/xfs/linux-2.6/xfs_iops.c +24 -27
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
@@ -179,18 +179,19 @@ xfs_ichgtime_fast(
  */
 STATIC void
 xfs_validate_fields(
-	struct inode	*ip,
-	bhv_vattr_t	*vattr)
+	struct inode		*inode)
 {
-	vattr->va_mask = XFS_AT_NLINK|XFS_AT_SIZE|XFS_AT_NBLOCKS;
-	if (!xfs_getattr(XFS_I(ip), vattr, ATTR_LAZY)) {
-		ip->i_nlink = vattr->va_nlink;
-		ip->i_blocks = vattr->va_nblocks;
-
-		/* we're under i_sem so i_size can't change under us */
-		if (i_size_read(ip) != vattr->va_size)
-			i_size_write(ip, vattr->va_size);
-	}
+	struct xfs_inode	*ip = XFS_I(inode);
+	loff_t size;
+
+	inode->i_nlink = ip->i_d.di_nlink;
+	inode->i_blocks =
+		XFS_FSB_TO_BB(ip->i_mount, ip->i_d.di_nblocks +
+					   ip->i_delayed_blks);
+	/* we're under i_sem so i_size can't change under us */
+	size = XFS_ISIZE(ip);
+	if (i_size_read(inode) != size)
+		i_size_write(inode, size);
 }
 
 /*
@@ -340,9 +341,9 @@ xfs_vn_mknod(
 		if (S_ISCHR(mode) || S_ISBLK(mode))
 			ip->i_rdev = rdev;
 		else if (S_ISDIR(mode))
-			xfs_validate_fields(ip, &vattr);
+			xfs_validate_fields(ip);
 		d_instantiate(dentry, ip);
-		xfs_validate_fields(dir, &vattr);
+		xfs_validate_fields(dir);
 	}
 	return -error;
 }
@@ -397,7 +398,6 @@ xfs_vn_link(
 {
 	struct inode	*ip;	/* inode of guy being linked to */
 	bhv_vnode_t	*vp;	/* vp of name being linked */
-	bhv_vattr_t	vattr;
 	int		error;
 
 	ip = old_dentry->d_inode;	/* inode being linked to */
@@ -409,7 +409,7 @@ xfs_vn_link(
 		VN_RELE(vp);
 	} else {
 		xfs_iflags_set(XFS_I(dir), XFS_IMODIFIED);
-		xfs_validate_fields(ip, &vattr);
+		xfs_validate_fields(ip);
 		d_instantiate(dentry, ip);
 	}
 	return -error;
@@ -421,15 +421,14 @@ xfs_vn_unlink(
 	struct dentry	*dentry)
 {
 	struct inode	*inode;
-	bhv_vattr_t	vattr;
 	int		error;
 
 	inode = dentry->d_inode;
 
 	error = xfs_remove(XFS_I(dir), dentry);
 	if (likely(!error)) {
-		xfs_validate_fields(dir, &vattr);	/* size needs update */
-		xfs_validate_fields(inode, &vattr);
+		xfs_validate_fields(dir);	/* size needs update */
+		xfs_validate_fields(inode);
 	}
 	return -error;
 }
@@ -458,8 +457,8 @@ xfs_vn_symlink(
 		if (likely(!error)) {
 			ip = vn_to_inode(cvp);
 			d_instantiate(dentry, ip);
-			xfs_validate_fields(dir, &va);
-			xfs_validate_fields(ip, &va);
+			xfs_validate_fields(dir);
+			xfs_validate_fields(ip);
 		} else {
 			xfs_cleanup_inode(dir, cvp, dentry, 0);
 		}
@@ -473,13 +472,12 @@ xfs_vn_rmdir(
 	struct dentry	*dentry)
 {
 	struct inode	*inode = dentry->d_inode;
-	bhv_vattr_t	vattr;
 	int		error;
 
 	error = xfs_rmdir(XFS_I(dir), dentry);
 	if (likely(!error)) {
-		xfs_validate_fields(inode, &vattr);
-		xfs_validate_fields(dir, &vattr);
+		xfs_validate_fields(inode);
+		xfs_validate_fields(dir);
 	}
 	return -error;
 }
@@ -493,7 +491,6 @@ xfs_vn_rename(
 {
 	struct inode	*new_inode = ndentry->d_inode;
 	bhv_vnode_t	*tvp;	/* target directory */
-	bhv_vattr_t	vattr;
 	int		error;
 
 	tvp = vn_from_inode(ndir);
@@ -501,10 +498,10 @@ xfs_vn_rename(
 	error = xfs_rename(XFS_I(odir), odentry, tvp, ndentry);
 	if (likely(!error)) {
 		if (new_inode)
-			xfs_validate_fields(new_inode, &vattr);
-		xfs_validate_fields(odir, &vattr);
+			xfs_validate_fields(new_inode);
+		xfs_validate_fields(odir);
 		if (ndir != odir)
-			xfs_validate_fields(ndir, &vattr);
+			xfs_validate_fields(ndir);
 	}
 	return -error;
 }
#1 lore raw

This looks good, just one comment...

Quoted 29 lines · Christoph Hellwig

Stop using xfs_getattr and a onstack bhv_vattr_t just to get three fields from the underlying inode and opencode copying from the inode fields instead.

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
===================================================================

fs/xfs/linux-2.6/xfs_iops.c +2 -11
@@ -179,18 +179,19 @@ xfs_ichgtime_fast(
  */
 STATIC void
 xfs_validate_fields(
-	struct inode	*ip,
-	bhv_vattr_t	*vattr)
+	struct inode		*inode)
 {
-	vattr->va_mask = XFS_AT_NLINK|XFS_AT_SIZE|XFS_AT_NBLOCKS;
-	if (!xfs_getattr(XFS_I(ip), vattr, ATTR_LAZY)) {
-		ip->i_nlink = vattr->va_nlink;
-		ip->i_blocks = vattr->va_nblocks;
-
-		/* we're under i_sem so i_size can't change under us */
-		if (i_size_read(ip) != vattr->va_size)
-			i_size_write(ip, vattr->va_size);
-	}
+	struct xfs_inode	*ip = XFS_I(inode);

fs/xfs/linux-2.6/xfs_iops.c xfs_ichgtime_fast( · line 194

Quoted 10 lines
fs/xfs/linux-2.6/xfs_iops.c +10 -0
+	loff_t size;
+
+	inode->i_nlink = ip->i_d.di_nlink;
+	inode->i_blocks =
+		XFS_FSB_TO_BB(ip->i_mount, ip->i_d.di_nblocks +
+					   ip->i_delayed_blks);
+	/* we're under i_sem so i_size can't change under us */
+	size = XFS_ISIZE(ip);
+	if (i_size_read(inode) != size)
+		i_size_write(inode, size);

Can we drop the i_size_read() check and just call i_size_write()? The work we may not do by not calling i_size_write() we have to do in i_size_read() anyway, sometimes doing the work twice. And we get to lose another stack variable.

Quoted 102 lines
fs/xfs/linux-2.6/xfs_iops.c +12 -16
 }
 
 /*
@@ -340,9 +341,9 @@ xfs_vn_mknod(
 		if (S_ISCHR(mode) || S_ISBLK(mode))
 			ip->i_rdev = rdev;
 		else if (S_ISDIR(mode))
-			xfs_validate_fields(ip, &vattr);
+			xfs_validate_fields(ip);
 		d_instantiate(dentry, ip);
-		xfs_validate_fields(dir, &vattr);
+		xfs_validate_fields(dir);
 	}
 	return -error;
 }
@@ -397,7 +398,6 @@ xfs_vn_link(
 {
 	struct inode	*ip;	/* inode of guy being linked to */
 	bhv_vnode_t	*vp;	/* vp of name being linked */
-	bhv_vattr_t	vattr;
 	int		error;
 
 	ip = old_dentry->d_inode;	/* inode being linked to */
@@ -409,7 +409,7 @@ xfs_vn_link(
 		VN_RELE(vp);
 	} else {
 		xfs_iflags_set(XFS_I(dir), XFS_IMODIFIED);
-		xfs_validate_fields(ip, &vattr);
+		xfs_validate_fields(ip);
 		d_instantiate(dentry, ip);
 	}
 	return -error;
@@ -421,15 +421,14 @@ xfs_vn_unlink(
 	struct dentry	*dentry)
 {
 	struct inode	*inode;
-	bhv_vattr_t	vattr;
 	int		error;
 
 	inode = dentry->d_inode;
 
 	error = xfs_remove(XFS_I(dir), dentry);
 	if (likely(!error)) {
-		xfs_validate_fields(dir, &vattr);	/* size needs update */
-		xfs_validate_fields(inode, &vattr);
+		xfs_validate_fields(dir);	/* size needs update */
+		xfs_validate_fields(inode);
 	}
 	return -error;
 }
@@ -458,8 +457,8 @@ xfs_vn_symlink(
 		if (likely(!error)) {
 			ip = vn_to_inode(cvp);
 			d_instantiate(dentry, ip);
-			xfs_validate_fields(dir, &va);
-			xfs_validate_fields(ip, &va);
+			xfs_validate_fields(dir);
+			xfs_validate_fields(ip);
 		} else {
 			xfs_cleanup_inode(dir, cvp, dentry, 0);
 		}
@@ -473,13 +472,12 @@ xfs_vn_rmdir(
 	struct dentry	*dentry)
 {
 	struct inode	*inode = dentry->d_inode;
-	bhv_vattr_t	vattr;
 	int		error;
 
 	error = xfs_rmdir(XFS_I(dir), dentry);
 	if (likely(!error)) {
-		xfs_validate_fields(inode, &vattr);
-		xfs_validate_fields(dir, &vattr);
+		xfs_validate_fields(inode);
+		xfs_validate_fields(dir);
 	}
 	return -error;
 }
@@ -493,7 +491,6 @@ xfs_vn_rename(
 {
 	struct inode	*new_inode = ndentry->d_inode;
 	bhv_vnode_t	*tvp;	/* target directory */
-	bhv_vattr_t	vattr;
 	int		error;
 
 	tvp = vn_from_inode(ndir);
@@ -501,10 +498,10 @@ xfs_vn_rename(
 	error = xfs_rename(XFS_I(odir), odentry, tvp, ndentry);
 	if (likely(!error)) {
 		if (new_inode)
-			xfs_validate_fields(new_inode, &vattr);
-		xfs_validate_fields(odir, &vattr);
+			xfs_validate_fields(new_inode);
+		xfs_validate_fields(odir);
 		if (ndir != odir)
-			xfs_validate_fields(ndir, &vattr);
+			xfs_validate_fields(ndir);
 	}
 	return -error;
 }
#2 lore raw
Quoted 4 lines · Lachlan McIlroy

Can we drop the i_size_read() check and just call i_size_write()? The work we may not do by not calling i_size_write() we have to do in i_size_read() anyway, sometimes doing the work twice. And we get to lose another stack variable.

Sounds good to me, but I'd prefer to keep it in a separate patch so this is just the mechanical removal of the xfs_getattr call.

I'll send a separate patch for it today or tomorrow.

#3 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Lachlan McIlroy

Can we drop the i_size_read() check and just call i_size_write()? The work we may not do by not calling i_size_write() we have to do in i_size_read() anyway, sometimes doing the work twice. And we get to lose another stack variable.

Sounds good to me, but I'd prefer to keep it in a separate patch so this is just the mechanical removal of the xfs_getattr call.

I'll send a separate patch for it today or tomorrow.

Okay, thanks.