[RFC] Create with EA initial work

[RFC] Create with EA initial work

[Niv Sardi]

Here's a dump of current work for Create+EA, this is buggy, it's
dropped out as I'm a bit busy and some people out there might be
interested in having a look.

These set of patches will move things around to make it possible to
pass a transaction down to the EA subsystem on create, but it seemed
to need more love.

Comments are welcome.

[PATCH] Move attr log alloc size calculator to another function.

[Niv Sardi]

From: Niv Sardi <xaiki@debian.org>

We will need that to be able to calculate the size of log we need for
a specific attr (for parent pointers in create). We need the local so
that we can fail if we run into ENOSPC when trying to alloc blocks

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c |   78 +++++++++++++++++++++++++++++++---------------------
 fs/xfs/xfs_attr.h |    2 +-
 2 files changed, 47 insertions(+), 33 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index e58f321..0d19e90 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -185,6 +185,43 @@ xfs_attr_get(
 }
 
 int
+xfs_attr_calc_size(
+	xfs_inode_t	*ip,
+	int		namelen,
+	int		valuelen,
+	int		*local)
+{
+	xfs_mount_t	*mp = ip->i_mount;
+	int		size;
+	int		nblks;
+
+	/*
+	 * Determine space new attribute will use, and if it would be
+	 * "local" or "remote" (note: local != inline).
+	 */
+	size = xfs_attr_leaf_newentsize(namelen, valuelen,
+					mp->m_sb.sb_blocksize, local);
+
+	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
+	if (*local) {
+		if (size > (mp->m_sb.sb_blocksize >> 1)) {
+			/* Double split possible */
+			nblks <<= 1;
+		}
+	} else {
+		/* 
+		 * Out of line attribute, cannot double split, but
+		 * make room for the attribute value itself.
+		 */
+		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
+		nblks += dblocks;
+		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
+	}
+
+	return nblks;
+}
+
+int
 xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 		 char *value, int valuelen, int flags)
 {
@@ -192,10 +229,9 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	xfs_fsblock_t	firstblock;
 	xfs_bmap_free_t flist;
 	int		error, err2, committed;
-	int		local, size;
-	uint		nblks;
 	xfs_mount_t	*mp = dp->i_mount;
 	int             rsvd = (flags & ATTR_ROOT) != 0;
+	int		local;
 
 	/*
 	 * Attach the dquots to the inode.
@@ -232,30 +268,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	args.addname = 1;
 	args.oknoent = 1;
 
-	/*
-	 * Determine space new attribute will use, and if it would be
-	 * "local" or "remote" (note: local != inline).
-	 */
-	size = xfs_attr_leaf_newentsize(namelen, valuelen,
-					mp->m_sb.sb_blocksize, &local);
-
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	if (local) {
-		if (size > (mp->m_sb.sb_blocksize >> 1)) {
-			/* Double split possible */
-			nblks <<= 1;
-		}
-	} else {
-		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
-		/* Out of line attribute, cannot double split, but make
-		 * room for the attribute value itself.
-		 */
-		nblks += dblocks;
-		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
-	}
-
 	/* Size is now blocks for attribute data */
-	args.total = nblks;
+	args.total = xfs_attr_calc_size(dp, namelen, valuelen, &local);
 
 	/*
 	 * Start our first transaction of the day.
@@ -277,18 +291,18 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	if (rsvd)
 		args.trans->t_flags |= XFS_TRANS_RESERVE;
 
-	if ((error = xfs_trans_reserve(args.trans, (uint) nblks,
-				      XFS_ATTRSET_LOG_RES(mp, nblks),
-				      0, XFS_TRANS_PERM_LOG_RES,
-				      XFS_ATTRSET_LOG_COUNT))) {
+	if ((error = xfs_trans_reserve(args.trans, (uint) args.total,
+				       XFS_ATTRSET_LOG_RES(mp, args.total),
+				       0, XFS_TRANS_PERM_LOG_RES,
+				       XFS_ATTRSET_LOG_COUNT))) {
 		xfs_trans_cancel(args.trans, 0);
 		return(error);
 	}
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, nblks, 0,
-			 rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-				XFS_QMOPT_RES_REGBLKS);
+	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, args.total, 0,
+				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+				       XFS_QMOPT_RES_REGBLKS);
 	if (error) {
 		xfs_iunlock(dp, XFS_ILOCK_EXCL);
 		xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index 786eba3..b0b5405 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -158,11 +158,11 @@ struct xfs_da_args;
 /*
  * Overall external interface routines.
  */
+int xfs_attr_calc_size(struct xfs_inode *, int, int, int *);
 int xfs_attr_set_int(struct xfs_inode *, const char *, int, char *, int, int);
 int xfs_attr_remove_int(struct xfs_inode *, const char *, int, int);
 int xfs_attr_list_int(struct xfs_attr_list_context *);
 int xfs_attr_inactive(struct xfs_inode *dp);
-
 int xfs_attr_shortform_getvalue(struct xfs_da_args *);
 int xfs_attr_fetch(struct xfs_inode *, const char *, int,
 			char *, int *, int, struct cred *);
-- 
1.5.5.4

[PATCH] Move xfs_attr_rolltrans to xfs_trans_roll

[Niv Sardi]

Move it from the attr code to the transaction code and make the attr
code call the new function.

We rolltrans is really usefull whenever we want to use rolling
transaction, should be generic, it isn't dependent on any part of the
attr code anyway.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c      |   22 ++++++++-------
 fs/xfs/xfs_attr_leaf.c |   69 ++++-------------------------------------------
 fs/xfs/xfs_attr_leaf.h |    2 -
 fs/xfs/xfs_trans.c     |   57 +++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h     |    1 +
 5 files changed, 76 insertions(+), 75 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 0d19e90..be6bfc4 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -389,7 +389,9 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 		 * Commit the leaf transformation.  We'll need another (linked)
 		 * transaction to add the new attribute to the leaf.
 		 */
-		if ((error = xfs_attr_rolltrans(&args.trans, dp)))
+
+		error = xfs_trans_roll(&args.trans, dp);
+		if (error)
 			goto out;
 
 	}
@@ -1019,7 +1021,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 		 * Commit the current trans (including the inode) and start
 		 * a new one.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		if ((error = xfs_trans_roll(&args->trans, dp)))
 			return (error);
 
 		/*
@@ -1033,7 +1035,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 	 * Commit the transaction that added the attr name so that
 	 * later routines can manage their own transactions.
 	 */
-	if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+	if ((error = xfs_trans_roll(&args->trans, dp)))
 		return (error);
 
 	/*
@@ -1122,7 +1124,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 		/*
 		 * Commit the remove and start the next trans in series.
 		 */
-		error = xfs_attr_rolltrans(&args->trans, dp);
+		error = xfs_trans_roll(&args->trans, dp);
 
 	} else if (args->rmtblkno > 0) {
 		/*
@@ -1353,7 +1355,7 @@ restart:
 			 * Commit the node conversion and start the next
 			 * trans in the chain.
 			 */
-			if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+			if ((error = xfs_trans_roll(&args->trans, dp)))
 				goto out;
 
 			goto restart;
@@ -1404,7 +1406,7 @@ restart:
 	 * Commit the leaf addition or btree split and start the next
 	 * trans in the chain.
 	 */
-	if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+	if ((error = xfs_trans_roll(&args->trans, dp)))
 		goto out;
 
 	/*
@@ -1504,7 +1506,7 @@ restart:
 		/*
 		 * Commit and start the next trans in the chain.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		if ((error = xfs_trans_roll(&args->trans, dp)))
 			goto out;
 
 	} else if (args->rmtblkno > 0) {
@@ -1636,7 +1638,7 @@ xfs_attr_node_removename(xfs_da_args_t *args)
 		/*
 		 * Commit the Btree join operation and start a new trans.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		if ((error = xfs_trans_roll(&args->trans, dp)))
 			goto out;
 	}
 
@@ -2137,7 +2139,7 @@ xfs_attr_rmtval_set(xfs_da_args_t *args)
 		/*
 		 * Start the next trans in the chain.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		if ((error = xfs_trans_roll(&args->trans, dp)))
 			return (error);
 	}
 
@@ -2287,7 +2289,7 @@ xfs_attr_rmtval_remove(xfs_da_args_t *args)
 		/*
 		 * Close out trans and start the next one in the chain.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, args->dp)))
+		if ((error = xfs_trans_roll(&args->trans, args->dp)))
 			return (error);
 	}
 	return(0);
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index b08e2a2..9465807 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -2543,7 +2543,7 @@ xfs_attr_leaf_clearflag(xfs_da_args_t *args)
 	/*
 	 * Commit the flag value change and start the next trans in series.
 	 */
-	error = xfs_attr_rolltrans(&args->trans, args->dp);
+	error = xfs_trans_roll(&args->trans, args->dp);
 
 	return(error);
 }
@@ -2592,7 +2592,7 @@ xfs_attr_leaf_setflag(xfs_da_args_t *args)
 	/*
 	 * Commit the flag value change and start the next trans in series.
 	 */
-	error = xfs_attr_rolltrans(&args->trans, args->dp);
+	error = xfs_trans_roll(&args->trans, args->dp);
 
 	return(error);
 }
@@ -2710,7 +2710,7 @@ xfs_attr_leaf_flipflags(xfs_da_args_t *args)
 	/*
 	 * Commit the flag value change and start the next trans in series.
 	 */
-	error = xfs_attr_rolltrans(&args->trans, args->dp);
+	error = xfs_trans_roll(&args->trans, args->dp);
 
 	return(error);
 }
@@ -2768,7 +2768,7 @@ xfs_attr_root_inactive(xfs_trans_t **trans, xfs_inode_t *dp)
 	/*
 	 * Commit the invalidate and start the next transaction.
 	 */
-	error = xfs_attr_rolltrans(trans, dp);
+	error = xfs_trans_roll(trans, dp);
 
 	return (error);
 }
@@ -2870,7 +2870,7 @@ xfs_attr_node_inactive(xfs_trans_t **trans, xfs_inode_t *dp, xfs_dabuf_t *bp,
 		/*
 		 * Atomically commit the whole invalidate stuff.
 		 */
-		if ((error = xfs_attr_rolltrans(trans, dp)))
+		if ((error = xfs_trans_roll(trans, dp)))
 			return (error);
 	}
 
@@ -3009,7 +3009,7 @@ xfs_attr_leaf_freextent(xfs_trans_t **trans, xfs_inode_t *dp,
 			/*
 			 * Roll to next transaction.
 			 */
-			if ((error = xfs_attr_rolltrans(trans, dp)))
+			if ((error = xfs_trans_roll(trans, dp)))
 				return (error);
 		}
 
@@ -3019,60 +3019,3 @@ xfs_attr_leaf_freextent(xfs_trans_t **trans, xfs_inode_t *dp,
 
 	return(0);
 }
-
-
-/*
- * Roll from one trans in the sequence of PERMANENT transactions to the next.
- */
-int
-xfs_attr_rolltrans(xfs_trans_t **transp, xfs_inode_t *dp)
-{
-	xfs_trans_t *trans;
-	unsigned int logres, count;
-	int	error;
-
-	/*
-	 * Ensure that the inode is always logged.
-	 */
-	trans = *transp;
-	xfs_trans_log_inode(trans, dp, XFS_ILOG_CORE);
-
-	/*
-	 * Copy the critical parameters from one trans to the next.
-	 */
-	logres = trans->t_log_res;
-	count = trans->t_log_count;
-	*transp = xfs_trans_dup(trans);
-
-	/*
-	 * Commit the current transaction.
-	 * If this commit failed, then it'd just unlock those items that
-	 * are not marked ihold. That also means that a filesystem shutdown
-	 * is in progress. The caller takes the responsibility to cancel
-	 * the duplicate transaction that gets returned.
-	 */
-	if ((error = xfs_trans_commit(trans, 0)))
-		return (error);
-
-	trans = *transp;
-
-	/*
-	 * Reserve space in the log for th next transaction.
-	 * This also pushes items in the "AIL", the list of logged items,
-	 * out to disk if they are taking up space at the tail of the log
-	 * that we want to use.  This requires that either nothing be locked
-	 * across this call, or that anything that is locked be logged in
-	 * the prior and the next transactions.
-	 */
-	error = xfs_trans_reserve(trans, 0, logres, 0,
-				  XFS_TRANS_PERM_LOG_RES, count);
-	/*
-	 *  Ensure that the inode is in the new transaction and locked.
-	 */
-	if (!error) {
-		xfs_trans_ijoin(trans, dp, XFS_ILOCK_EXCL);
-		xfs_trans_ihold(trans, dp);
-	}
-	return (error);
-
-}
diff --git a/fs/xfs/xfs_attr_leaf.h b/fs/xfs/xfs_attr_leaf.h
index 040f732..698e078 100644
--- a/fs/xfs/xfs_attr_leaf.h
+++ b/fs/xfs/xfs_attr_leaf.h
@@ -301,6 +301,4 @@ int	xfs_attr_leaf_order(struct xfs_dabuf *leaf1_bp,
 				   struct xfs_dabuf *leaf2_bp);
 int	xfs_attr_leaf_newentsize(int namelen, int valuelen, int blocksize,
 					int *local);
-int	xfs_attr_rolltrans(struct xfs_trans **transp, struct xfs_inode *dp);
-
 #endif	/* __XFS_ATTR_LEAF_H__ */
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 1403864..9e5e0c2 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -43,6 +43,7 @@
 #include "xfs_quota.h"
 #include "xfs_trans_priv.h"
 #include "xfs_trans_space.h"
+#include "xfs_inode_item.h"
 
 
 STATIC void	xfs_trans_apply_sb_deltas(xfs_trans_t *);
@@ -1216,6 +1217,62 @@ xfs_trans_free(
 	kmem_zone_free(xfs_trans_zone, tp);
 }
 
+/*
+ * Roll from one trans in the sequence of PERMANENT transactions to the next.
+ */
+int
+xfs_trans_roll(
+	xfs_trans_t	**tpp,
+	xfs_inode_t	*dp)
+{
+	xfs_trans_t	*trans;
+	unsigned int	logres, count;
+	int		error;
+
+	/*
+	 * Ensure that the inode is always logged.
+	 */
+	trans = *tpp;
+	xfs_trans_log_inode(trans, dp, XFS_ILOG_CORE);
+
+	/*
+	 * Copy the critical parameters from one trans to the next.
+	 */
+	logres = trans->t_log_res;
+	count = trans->t_log_count;
+	*tpp = xfs_trans_dup(trans);
+
+	/*
+	 * Commit the current transaction.
+	 * If this commit failed, then it'd just unlock those items that
+	 * are not marked ihold. That also means that a filesystem shutdown
+	 * is in progress. The caller takes the responsibility to cancel
+	 * the duplicate transaction that gets returned.
+	 */
+	if ((error = xfs_trans_commit(trans, 0)))
+		return (error);
+
+	trans = *tpp;
+
+	/*
+	 * Reserve space in the log for th next transaction.
+	 * This also pushes items in the "AIL", the list of logged items,
+	 * out to disk if they are taking up space at the tail of the log
+	 * that we want to use.  This requires that either nothing be locked
+	 * across this call, or that anything that is locked be logged in
+	 * the prior and the next transactions.
+	 */
+	error = xfs_trans_reserve(trans, 0, logres, 0,
+				  XFS_TRANS_PERM_LOG_RES, count);
+	/*
+	 *  Ensure that the inode is in the new transaction and locked.
+	 */
+	if (!error) {
+		xfs_trans_ijoin(trans, dp, XFS_ILOCK_EXCL);
+		xfs_trans_ihold(trans, dp);
+	}
+	return (error);
+}
 
 /*
  * THIS SHOULD BE REWRITTEN TO USE xfs_trans_next_item().
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 7f40628..7b4299e 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -992,6 +992,7 @@ int		_xfs_trans_commit(xfs_trans_t *,
 				  int *);
 #define xfs_trans_commit(tp, flags)	_xfs_trans_commit(tp, flags, NULL)
 void		xfs_trans_cancel(xfs_trans_t *, int);
+int		xfs_trans_roll(struct xfs_trans **, struct xfs_inode *);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);
 void		xfs_trans_push_ail(struct xfs_mount *, xfs_lsn_t);
-- 
1.5.5.4

[PATCH] Introduce xfs_trans_bmap_add_attrfork.

[Niv Sardi]

That takes a transaction and doesn't require everything to be locked
anymore.  this uses the roll trans call.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_bmap.c |   79 ++++++++++++++++++++++++++++++++++-------------------
 fs/xfs/xfs_bmap.h |    7 +++++
 2 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 1c0a5a5..c5cfbcb 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -3941,12 +3941,22 @@ xfs_bunmap_trace(
 }
 #endif
 
+int
+xfs_bmap_add_attrfork(
+	xfs_inode_t		*ip,		/* incore inode pointer */
+	int			size,		/* space new attribute needs */
+	int			rsvd)		/* xact may use reserved blks */
+{
+	return xfs_trans_bmap_add_attrfork(NULL, ip, size, rsvd);
+}
+
 /*
  * Convert inode from non-attributed to attributed.
  * Must not be in a transaction, ip must not be locked.
  */
 int						/* error code */
-xfs_bmap_add_attrfork(
+xfs_trans_bmap_add_attrfork(
+	xfs_trans_t		**tpp,		/* transaction pointer */	    
 	xfs_inode_t		*ip,		/* incore inode pointer */
 	int			size,		/* space new attribute needs */
 	int			rsvd)		/* xact may use reserved blks */
@@ -3967,24 +3977,33 @@ xfs_bmap_add_attrfork(
 
 	mp = ip->i_mount;
 	ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
-	tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
-	blks = XFS_ADDAFORK_SPACE_RES(mp);
-	if (rsvd)
-		tp->t_flags |= XFS_TRANS_RESERVE;
-	if ((error = xfs_trans_reserve(tp, blks, XFS_ADDAFORK_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_ADDAFORK_LOG_COUNT)))
-		goto error0;
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, tp, ip, blks, 0, rsvd ?
-			XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-			XFS_QMOPT_RES_REGBLKS);
-	if (error) {
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
-		return error;
+	if (tpp) {
+		ASSERT(*tpp);
+		tp = *tpp;
+	} else {
+		tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
+		blks = XFS_ADDAFORK_SPACE_RES(mp);
+		if (rsvd)
+			tp->t_flags |= XFS_TRANS_RESERVE;
+		if ((error = xfs_trans_reserve(tp, blks, XFS_ADDAFORK_LOG_RES(mp), 0,
+					       XFS_TRANS_PERM_LOG_RES, XFS_ADDAFORK_LOG_COUNT)))
+			goto error0;
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
+		error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, tp, ip, blks, 0, rsvd ?
+						      XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+						      XFS_QMOPT_RES_REGBLKS);
+		if (error) {
+			xfs_iunlock(ip, XFS_ILOCK_EXCL);
+			xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
+			return error;
+		}
+		if (XFS_IFORK_Q(ip))
+			goto error1;
+		ASSERT(ip->i_d.di_anextents == 0);
+		VN_HOLD(XFS_ITOV(ip));
+		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	}
-	if (XFS_IFORK_Q(ip))
-		goto error1;
 	if (ip->i_d.di_aformat != XFS_DINODE_FMT_EXTENTS) {
 		/*
 		 * For inodes coming from pre-6.2 filesystems.
@@ -3992,10 +4011,7 @@ xfs_bmap_add_attrfork(
 		ASSERT(ip->i_d.di_aformat == 0);
 		ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
 	}
-	ASSERT(ip->i_d.di_anextents == 0);
-	VN_HOLD(XFS_ITOV(ip));
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
 	switch (ip->i_d.di_format) {
 	case XFS_DINODE_FMT_DEV:
 		ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
@@ -4060,23 +4076,30 @@ xfs_bmap_add_attrfork(
 			XFS_SB_VERSION_ADDATTR2(&mp->m_sb);
 			sbfields |= (XFS_SB_VERSIONNUM | XFS_SB_FEATURES2);
 		}
-		if (sbfields) {
-			spin_unlock(&mp->m_sb_lock);
+		spin_unlock(&mp->m_sb_lock);
+		if (sbfields)
 			xfs_mod_sb(tp, sbfields);
-		} else
-			spin_unlock(&mp->m_sb_lock);
 	}
 	if ((error = xfs_bmap_finish(&tp, &flist, &committed)))
 		goto error2;
-	error = xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
+	if (tpp) {
+		error = xfs_trans_roll(&tp, ip);
+	} else {
+		error = xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
+	}
 	ASSERT(ip->i_df.if_ext_max ==
 	       XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t));
 	return error;
 error2:
+	if (tpp)
+		tpp = &tp;
 	xfs_bmap_cancel(&flist);
 error1:
 	ASSERT(ismrlocked(&ip->i_lock,MR_UPDATE));
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	if (tpp)
+		tpp = &tp;
+	else
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
 error0:
 	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
 	ASSERT(ip->i_df.if_ext_max ==
diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
index 87224b7..7a21e41 100644
--- a/fs/xfs/xfs_bmap.h
+++ b/fs/xfs/xfs_bmap.h
@@ -166,6 +166,13 @@ xfs_bmap_add_attrfork(
 	int			size,	/* space needed for new attribute */
 	int			rsvd);	/* flag for reserved block allocation */
 
+int					/* error code */
+xfs_trans_bmap_add_attrfork(
+	struct xfs_trans	**tpp,   /* transaction */
+	struct xfs_inode	*ip,	/* incore inode pointer */
+	int			size,	/* space needed for new attribute */
+	int			rsvd);	/* flag for reserved block allocation */
+
 /*
  * Add the extent to the list of extents to be free at transaction end.
  * The list is maintained sorted (by block number).
-- 
1.5.5.4

[PATCH] Give a transaction to xfs_attr_set_int

[Niv Sardi]

From: Niv Sardi <xaiki@debian.org>

We introduce xfs_trans_attr_set_int that takes a transaction pointer
as an argument (or creates one if NULL) and only finishes the
transaction if it has created it. We use xfs_attr_rolltrans to do the
tranS_dup dance.

xfs_attr_set_int is changed to a wrapper that will only call
xfs_trans_attr_set_int with a NULL transaction.

make it use the new xfs_trans_bmap_add_attrfork();

This is needed for Create+EA/Parent Pointers

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c |  140 +++++++++++++++++++++++++++++++++++-----------------
 fs/xfs/xfs_attr.h |    2 +
 2 files changed, 96 insertions(+), 46 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index be6bfc4..a4787f7 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -209,7 +209,7 @@ xfs_attr_calc_size(
 			nblks <<= 1;
 		}
 	} else {
-		/* 
+		/*
 		 * Out of line attribute, cannot double split, but
 		 * make room for the attribute value itself.
 		 */
@@ -221,9 +221,20 @@ xfs_attr_calc_size(
 	return nblks;
 }
 
+/*
+ * So if the trans is given we don't have the right to dispose of it,
+ * we can't commit it either as we don't know if the upper class is
+ * done with it.
+ */
 int
-xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
-		 char *value, int valuelen, int flags)
+xfs_trans_attr_set_int(
+	xfs_trans_t	**tpp,
+	xfs_inode_t	*dp,
+	const char	*name,
+	int		namelen,
+	char		*value,
+	int		valuelen,
+	int		flags)
 {
 	xfs_da_args_t	args;
 	xfs_fsblock_t	firstblock;
@@ -247,8 +258,9 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
 			      XFS_ATTR_SF_ENTSIZE_BYNAME(namelen, valuelen);
 
-		if ((error = xfs_bmap_add_attrfork(dp, sf_size, rsvd)))
-			return(error);
+		error = xfs_trans_bmap_add_attrfork(tpp, dp, sf_size, rsvd);
+		if (error)
+			return error;
 	}
 
 	/*
@@ -271,46 +283,53 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	/* Size is now blocks for attribute data */
 	args.total = xfs_attr_calc_size(dp, namelen, valuelen, &local);
 
-	/*
-	 * Start our first transaction of the day.
-	 *
-	 * All future transactions during this code must be "chained" off
-	 * this one via the trans_dup() call.  All transactions will contain
-	 * the inode, and the inode will always be marked with trans_ihold().
-	 * Since the inode will be locked in all transactions, we must log
-	 * the inode in every transaction to let it float upward through
-	 * the log.
-	 */
-	args.trans = xfs_trans_alloc(mp, XFS_TRANS_ATTR_SET);
+	if (tpp) {
+		ASSERT(*tpp);
+		args.trans = *tpp;
+	} else {
+		/*
+		 * Start our first transaction of the day.
+		 *
+		 * All future transactions during this code must be "chained" off
+		 * this one via the trans_dup() call.  All transactions will contain
+		 * the inode, and the inode will always be marked with trans_ihold().
+		 * Since the inode will be locked in all transactions, we must log
+		 * the inode in every transaction to let it float upward through
+		 * the log.
+		 */
+		args.trans = xfs_trans_alloc(mp, XFS_TRANS_ATTR_SET);
 
-	/*
-	 * Root fork attributes can use reserved data blocks for this
-	 * operation if necessary
-	 */
+		/*
+		 * Root fork attributes can use reserved data blocks for this
+		 * operation if necessary
+		 */
 
-	if (rsvd)
-		args.trans->t_flags |= XFS_TRANS_RESERVE;
+		if (rsvd)
+			args.trans->t_flags |= XFS_TRANS_RESERVE;
 
-	if ((error = xfs_trans_reserve(args.trans, (uint) args.total,
-				       XFS_ATTRSET_LOG_RES(mp, args.total),
-				       0, XFS_TRANS_PERM_LOG_RES,
-				       XFS_ATTRSET_LOG_COUNT))) {
-		xfs_trans_cancel(args.trans, 0);
-		return(error);
-	}
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
+		error = xfs_trans_reserve(args.trans, args.total,
+					  XFS_ATTRSET_LOG_RES(mp, args.total),
+					  0, XFS_TRANS_PERM_LOG_RES,
+					  XFS_ATTRSET_LOG_COUNT);
+		if (error) {
+			xfs_trans_cancel(args.trans, 0);
+			return(error);
+		}
+		xfs_ilock(dp, XFS_ILOCK_EXCL);
 
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, args.total, 0,
-				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+		error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp,
+					args.total, 0,
+				rsvd?XFS_QMOPT_RES_REGBLKS|XFS_QMOPT_FORCE_RES:
 				       XFS_QMOPT_RES_REGBLKS);
-	if (error) {
-		xfs_iunlock(dp, XFS_ILOCK_EXCL);
-		xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
-		return (error);
-	}
+		if (error) {
+			xfs_iunlock(dp, XFS_ILOCK_EXCL);
+			xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
+			return (error);
+		}
 
-	xfs_trans_ijoin(args.trans, dp, XFS_ILOCK_EXCL);
-	xfs_trans_ihold(args.trans, dp);
+		xfs_trans_ijoin(args.trans, dp, XFS_ILOCK_EXCL);
+		xfs_trans_ihold(args.trans, dp);
+	}
 
 	/*
 	 * If the attribute list is non-existent or a shortform list,
@@ -346,8 +365,14 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 			if (mp->m_flags & XFS_MOUNT_WSYNC) {
 				xfs_trans_set_sync(args.trans);
 			}
-			err2 = xfs_trans_commit(args.trans,
-						 XFS_TRANS_RELEASE_LOG_RES);
+
+			/* Only finish trans if it's ours */
+			if (tpp) {
+				err2 = xfs_trans_roll(&args.trans, dp);
+			} else {
+				err2 = xfs_trans_commit(args.trans,
+							XFS_TRANS_RELEASE_LOG_RES);
+			}
 			xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
 			/*
@@ -356,6 +381,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 			if (!error && (flags & ATTR_KERNOTIME) == 0) {
 				xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
 			}
+			if (tpp)
+				tpp = &args.trans;
 			return(error == 0 ? err2 : error);
 		}
 
@@ -401,13 +428,13 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	} else {
 		error = xfs_attr_node_addname(&args);
 	}
-	if (error) {
+	if (error)
 		goto out;
-	}
 
 	/*
 	 * If this is a synchronous mount, make sure that the
-	 * transaction goes to disk before returning to the user.
+	 * transaction goes to disk before returning to the user. If
+	 * this is not our transaction, the allocator should do this.
 	 */
 	if (mp->m_flags & XFS_MOUNT_WSYNC) {
 		xfs_trans_set_sync(args.trans);
@@ -416,8 +443,12 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	/*
 	 * Commit the last in the sequence of transactions.
 	 */
-	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
-	error = xfs_trans_commit(args.trans, XFS_TRANS_RELEASE_LOG_RES);
+	if (tpp) {
+		xfs_trans_roll(&args.trans, dp);
+	} else {
+		xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
+		error = xfs_trans_commit(args.trans, XFS_TRANS_RELEASE_LOG_RES);
+	}
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
 	/*
@@ -427,6 +458,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 		xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
 	}
 
+	if (tpp)
+		tpp = &args.trans;
 	return(error);
 
 out:
@@ -434,10 +467,25 @@ out:
 		xfs_trans_cancel(args.trans,
 			XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
+	if (tpp)
+		tpp = &args.trans;
 	return(error);
 }
 
 int
+xfs_attr_set_int(
+	xfs_inode_t	*dp,
+	const char	*name,
+	int		namelen,
+	char		*value,
+	int		valuelen,
+	int		flags)
+{
+	return xfs_trans_attr_set_int(NULL, dp, name, namelen,
+				      value, valuelen, flags);
+}
+
+int
 xfs_attr_set(
 	xfs_inode_t	*dp,
 	const char	*name,
diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index b0b5405..c076c85 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -160,6 +160,8 @@ struct xfs_da_args;
  */
 int xfs_attr_calc_size(struct xfs_inode *, int, int, int *);
 int xfs_attr_set_int(struct xfs_inode *, const char *, int, char *, int, int);
+ int xfs_trans_attr_set_int(struct xfs_trans **, struct xfs_inode *, const char *,
+			    int, char *, int, int);
 int xfs_attr_remove_int(struct xfs_inode *, const char *, int, int);
 int xfs_attr_list_int(struct xfs_attr_list_context *);
 int xfs_attr_inactive(struct xfs_inode *dp);
-- 
1.5.5.4

Re: [PATCH] Give a transaction to xfs_attr_set_int

[Dave Chinner]

On Mon, Jun 23, 2008 at 02:42:30PM +1000, Niv Sardi wrote:
> From: Niv Sardi <xaiki@debian.org>
> 
> We introduce xfs_trans_attr_set_int that takes a transaction pointer
> as an argument (or creates one if NULL) and only finishes the
> transaction if it has created it. We use xfs_attr_rolltrans to do the
> tranS_dup dance.
> 
> xfs_attr_set_int is changed to a wrapper that will only call
> xfs_trans_attr_set_int with a NULL transaction.

As a general comment to the entire patch set, I dislike the
namespace pollution caused by changing xfs_attr_...  to
xfs_trans_attr...

The xfs_trans_... namespace is used for stuff inside xfs_trans*[ch],
not for attr code. I'd suggest making the "trans" a suffix rather
than a prefix, or something similar, so that these attr functions
are not easily confused with core transaction code....

BTW:

> @@ -356,6 +381,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
>  			if (!error && (flags & ATTR_KERNOTIME) == 0) {
>  				xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
>  			}
> +			if (tpp)
> +				tpp = &args.trans;

That's busted too. Can you please review all the places where you
return transactio pointers to the caller via a function parameterrr
for this bug as you've made in at least a couple of places.

> +	if (tpp)
> +		tpp = &args.trans;

Here as well.

>  	return(error);
>  
>  out:
> @@ -434,10 +467,25 @@ out:
>  		xfs_trans_cancel(args.trans,
>  			XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
>  	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> +	if (tpp)
> +		tpp = &args.trans;

And again.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] Give a transaction to xfs_attr_set_int

[Josef 'Jeff' Sipek]

On Mon, Jun 30, 2008 at 08:08:59AM +1000, Dave Chinner wrote:
...
> > @@ -356,6 +381,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
> >  			if (!error && (flags & ATTR_KERNOTIME) == 0) {
> >  				xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
> >  			}
> > +			if (tpp)
> > +				tpp = &args.trans;
> 
> That's busted too. Can you please review all the places where you
> return transactio pointers to the caller via a function parameterrr
> for this bug as you've made in at least a couple of places.

Niv: Why not return the pointer as a return value?

Josef 'Jeff' Sipek.

-- 
Humans were created by water to transport it upward.

Re: [PATCH] Introduce xfs_trans_bmap_add_attrfork.

[Christoph Hellwig]

> +int
> +xfs_bmap_add_attrfork(
> +	xfs_inode_t		*ip,		/* incore inode pointer */
> +	int			size,		/* space new attribute needs */
> +	int			rsvd)		/* xact may use reserved blks */
> +{
> +	return xfs_trans_bmap_add_attrfork(NULL, ip, size, rsvd);
> +}

Care to add this below xfs_trans_bmap_add_attrfork?  Also please
us struct xfs_inode instead of the typedef.

> +	if (tpp) {
> +		ASSERT(*tpp);
> +		tp = *tpp;
> +	} else {
> +		tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
> +		blks = XFS_ADDAFORK_SPACE_RES(mp);
> +		if (rsvd)
> +			tp->t_flags |= XFS_TRANS_RESERVE;
> +		if ((error = xfs_trans_reserve(tp, blks, XFS_ADDAFORK_LOG_RES(mp), 0,
> +					       XFS_TRANS_PERM_LOG_RES, XFS_ADDAFORK_LOG_COUNT)))
> +			goto error0;
> +		xfs_ilock(ip, XFS_ILOCK_EXCL);
> +		error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, tp, ip, blks, 0, rsvd ?
> +						      XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
> +						      XFS_QMOPT_RES_REGBLKS);
> +		if (error) {
> +			xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +			xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
> +			return error;
> +		}
> +		if (XFS_IFORK_Q(ip))
> +			goto error1;
> +		ASSERT(ip->i_d.di_anextents == 0);
> +		VN_HOLD(XFS_ITOV(ip));
> +		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

> +	if (tpp) {
> +		error = xfs_trans_roll(&tp, ip);
> +	} else {
> +		error = xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
> +	}

I think it would be much cleaner if all the transaction setup, joining
etc was done in xfs_bmap_add_attrfork, and xfs_trans_bmap_add_attrfork
just gets an always non-NULL xfs_trans pointer.  That would mean
the common code would become just a helper, and
xfs_trans_bmap_add_attrfork would be a small wrapper including the 
xfs_trans_roll.  Alternatively the caller could always do the roll.

> -	if (XFS_IFORK_Q(ip))
> -		goto error1;
> +	if (tpp) {
> +		error = xfs_trans_roll(&tp, ip);
> +	} else {
> +		error = xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
> +	}

Why is this check made conditional?

> -	ASSERT(ip->i_d.di_anextents == 0);
> -	VN_HOLD(XFS_ITOV(ip));
> -	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> -	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +
>  	switch (ip->i_d.di_format) {
>  	case XFS_DINODE_FMT_DEV:
>  		ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
> @@ -4060,23 +4076,30 @@ xfs_bmap_add_attrfork(
>  			XFS_SB_VERSION_ADDATTR2(&mp->m_sb);
>  			sbfields |= (XFS_SB_VERSIONNUM | XFS_SB_FEATURES2);
>  		}
> -		if (sbfields) {
> -			spin_unlock(&mp->m_sb_lock);
> +		spin_unlock(&mp->m_sb_lock);
> +		if (sbfields)
>  			xfs_mod_sb(tp, sbfields);
> -		} else
> -			spin_unlock(&mp->m_sb_lock);

I don't think this change belongs into this patch.

>  	ASSERT(ip->i_df.if_ext_max ==
>  	       XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t));
>  	return error;
>  error2:
> +	if (tpp)
> +		tpp = &tp;
>  	xfs_bmap_cancel(&flist);
>  error1:
>  	ASSERT(ismrlocked(&ip->i_lock,MR_UPDATE));
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	if (tpp)
> +		tpp = &tp;
> +	else
> +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  error0:
>  	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
>  	ASSERT(ip->i_df.if_ext_max ==
> diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
> index 87224b7..7a21e41 100644
> --- a/fs/xfs/xfs_bmap.h
> +++ b/fs/xfs/xfs_bmap.h
> @@ -166,6 +166,13 @@ xfs_bmap_add_attrfork(
>  	int			size,	/* space needed for new attribute */
>  	int			rsvd);	/* flag for reserved block allocation */
>  
> +int					/* error code */
> +xfs_trans_bmap_add_attrfork(
> +	struct xfs_trans	**tpp,   /* transaction */
> +	struct xfs_inode	*ip,	/* incore inode pointer */
> +	int			size,	/* space needed for new attribute */
> +	int			rsvd);	/* flag for reserved block allocation */
> +
>  /*
>   * Add the extent to the list of extents to be free at transaction end.
>   * The list is maintained sorted (by block number).
> -- 
> 1.5.5.4
> 
> 
---end quoted text---

Re: [PATCH] Introduce xfs_trans_bmap_add_attrfork.

[Niv Sardi]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]


Updated patch attached,
Moved case where there is no transaction back into
xfs_bmap_add_attrfork() and rely on caller to call xfs_trans_roll(),

Christoph Hellwig <hch@infradead.org> writes:
>> +xfs_bmap_add_attrfork(
[...]
> Care to add this below xfs_trans_bmap_add_attrfork?  Also please
> us struct xfs_inode instead of the typedef.

Done,

>> +	if (tpp) {
>> +		ASSERT(*tpp);
>> +		tp = *tpp;
>> +	} else {
[...]
>
> I think it would be much cleaner if all the transaction setup, joining
> etc was done in xfs_bmap_add_attrfork, and xfs_trans_bmap_add_attrfork
> just gets an always non-NULL xfs_trans pointer.  That would mean
> the common code would become just a helper, and
> xfs_trans_bmap_add_attrfork would be a small wrapper including the 
> xfs_trans_roll.  Alternatively the caller could always do the roll.

I think I got it right, but error handeling is a bit weird this way,
looks logically identical.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Introduce-xfs_trans_bmap_add_attrfork.patch --]
[-- Type: text/x-diff, Size: 6308 bytes --]

>From 8409ab9129b788ec9b1c2fb81131f70de0f4bf65 Mon Sep 17 00:00:00 2001
From: Niv Sardi <xaiki@sgi.com>
Date: Thu, 22 May 2008 16:18:00 +1000
Subject: [PATCH] Introduce xfs_trans_bmap_add_attrfork.

That takes a transaction and doesn't require everything to be locked
anymore. This doesn't commit the transaction ! so direct callers,
willing to use xfs_trans_roll() should do it themselves.

Change xfs_bmap_add_attrfork to do the initialization/allocation of
the transaction and commit arround xfs_trans_bmap_add_attrfork.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_bmap.c |  100 ++++++++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_bmap.h |    7 ++++
 2 files changed, 72 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 1c0a5a5..d4bd6e7 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -3946,16 +3946,16 @@ xfs_bunmap_trace(
  * Must not be in a transaction, ip must not be locked.
  */
 int						/* error code */
-xfs_bmap_add_attrfork(
-	xfs_inode_t		*ip,		/* incore inode pointer */
+xfs_trans_bmap_add_attrfork(
+	struct xfs_trans	**tpp,		/* transaction pointer */
+	struct xfs_inode	*ip,		/* incore inode pointer */
 	int			size,		/* space new attribute needs */
 	int			rsvd)		/* xact may use reserved blks */
 {
 	xfs_fsblock_t		firstblock;	/* 1st block/ag allocated */
-	xfs_bmap_free_t		flist;		/* freed extent records */
-	xfs_mount_t		*mp;		/* mount structure */
-	xfs_trans_t		*tp;		/* transaction pointer */
-	int			blks;		/* space reservation */
+	struct xfs_bmap_free	flist;		/* freed extent records */
+	struct xfs_mount	*mp;		/* mount structure */
+	struct xfs_trans	*tp;		/* transaction pointer */
 	int			version = 1;	/* superblock attr version */
 	int			committed;	/* xaction was committed */
 	int			logflags;	/* logging flags */
@@ -3967,24 +3967,8 @@ xfs_bmap_add_attrfork(
 
 	mp = ip->i_mount;
 	ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
-	tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
-	blks = XFS_ADDAFORK_SPACE_RES(mp);
-	if (rsvd)
-		tp->t_flags |= XFS_TRANS_RESERVE;
-	if ((error = xfs_trans_reserve(tp, blks, XFS_ADDAFORK_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_ADDAFORK_LOG_COUNT)))
-		goto error0;
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, tp, ip, blks, 0, rsvd ?
-			XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-			XFS_QMOPT_RES_REGBLKS);
-	if (error) {
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
-		return error;
-	}
-	if (XFS_IFORK_Q(ip))
-		goto error1;
+	ASSERT(*tpp);
+	tp = *tpp;
 	if (ip->i_d.di_aformat != XFS_DINODE_FMT_EXTENTS) {
 		/*
 		 * For inodes coming from pre-6.2 filesystems.
@@ -3992,10 +3976,7 @@ xfs_bmap_add_attrfork(
 		ASSERT(ip->i_d.di_aformat == 0);
 		ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
 	}
-	ASSERT(ip->i_d.di_anextents == 0);
-	VN_HOLD(XFS_ITOV(ip));
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
 	switch (ip->i_d.di_format) {
 	case XFS_DINODE_FMT_DEV:
 		ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
@@ -4015,7 +3996,7 @@ xfs_bmap_add_attrfork(
 	default:
 		ASSERT(0);
 		error = XFS_ERROR(EINVAL);
-		goto error1;
+		goto error0;
 	}
 	ip->i_df.if_ext_max =
 		XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
@@ -4046,7 +4027,7 @@ xfs_bmap_add_attrfork(
 	if (logflags)
 		xfs_trans_log_inode(tp, ip, logflags);
 	if (error)
-		goto error2;
+		goto error1;
 	if (!XFS_SB_VERSION_HASATTR(&mp->m_sb) ||
 	   (!XFS_SB_VERSION_HASATTR2(&mp->m_sb) && version == 2)) {
 		__int64_t sbfields = 0;
@@ -4066,14 +4047,63 @@ xfs_bmap_add_attrfork(
 		} else
 			spin_unlock(&mp->m_sb_lock);
 	}
-	if ((error = xfs_bmap_finish(&tp, &flist, &committed)))
-		goto error2;
-	error = xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
+	error = xfs_bmap_finish(tpp, &flist, &committed);
+	if (error)
+		goto error1;
 	ASSERT(ip->i_df.if_ext_max ==
 	       XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t));
-	return error;
-error2:
+	return 0;
+error1:
 	xfs_bmap_cancel(&flist);
+	ASSERT(ismrlocked(&ip->i_lock, MR_UPDATE));
+error0:
+	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
+	ASSERT(ip->i_df.if_ext_max ==
+	       XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t));
+	return error;
+}
+
+int
+xfs_bmap_add_attrfork(
+	struct xfs_inode	*ip,		/* incore inode pointer */
+	int			size,		/* space new attribute needs */
+	int			rsvd)		/* xact may use reserved blks */
+{
+	struct xfs_trans	*tp;		/* transaction pointer */
+	struct xfs_mount	*mp;		/* mount structure */
+	int			blks;		/* space reservation */
+	int			error;		/* error return value */
+
+	mp = ip->i_mount;
+	tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
+	blks = XFS_ADDAFORK_SPACE_RES(mp);
+
+	if (rsvd)
+		tp->t_flags |= XFS_TRANS_RESERVE;
+	error = xfs_trans_reserve(tp, blks, XFS_ADDAFORK_LOG_RES(mp), 0,
+				XFS_TRANS_PERM_LOG_RES, XFS_ADDAFORK_LOG_COUNT);
+	if (error)
+		goto error0;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, tp, ip, blks, 0, rsvd ?
+			      XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+			      XFS_QMOPT_RES_REGBLKS);
+	if (error)
+		goto error1;
+
+	if (XFS_IFORK_Q(ip))
+		goto error1;
+
+	ASSERT(ip->i_d.di_anextents == 0);
+	VN_HOLD(XFS_ITOV(ip));
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
+	error = xfs_trans_bmap_add_attrfork(NULL, ip, size, rsvd);
+	if (error)
+		return error;
+	return xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
 error1:
 	ASSERT(ismrlocked(&ip->i_lock,MR_UPDATE));
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
index 87224b7..7a21e41 100644
--- a/fs/xfs/xfs_bmap.h
+++ b/fs/xfs/xfs_bmap.h
@@ -166,6 +166,13 @@ xfs_bmap_add_attrfork(
 	int			size,	/* space needed for new attribute */
 	int			rsvd);	/* flag for reserved block allocation */
 
+int					/* error code */
+xfs_trans_bmap_add_attrfork(
+	struct xfs_trans	**tpp,   /* transaction */
+	struct xfs_inode	*ip,	/* incore inode pointer */
+	int			size,	/* space needed for new attribute */
+	int			rsvd);	/* flag for reserved block allocation */
+
 /*
  * Add the extent to the list of extents to be free at transaction end.
  * The list is maintained sorted (by block number).
-- 
1.5.6


[-- Attachment #3: Type: text/plain, Size: 51 bytes --]


Thanks for this extensive review =)
-- 
Niv Sardi

Re: [PATCH] Introduce xfs_trans_bmap_add_attrfork.

[Timothy Shimmin]

Niv Sardi wrote:
> Updated patch attached,
> Moved case where there is no transaction back into
> xfs_bmap_add_attrfork() and rely on caller to call xfs_trans_roll(),
> 
> Christoph Hellwig <hch@infradead.org> writes:
>>> +xfs_bmap_add_attrfork(
> [...]
>> Care to add this below xfs_trans_bmap_add_attrfork?  Also please
>> us struct xfs_inode instead of the typedef.
> 
> Done,
> 
>>> +	if (tpp) {
>>> +		ASSERT(*tpp);
>>> +		tp = *tpp;
>>> +	} else {
> [...]
>> I think it would be much cleaner if all the transaction setup, joining
>> etc was done in xfs_bmap_add_attrfork, and xfs_trans_bmap_add_attrfork
>> just gets an always non-NULL xfs_trans pointer.  That would mean
>> the common code would become just a helper, and
>> xfs_trans_bmap_add_attrfork would be a small wrapper including the 
>> xfs_trans_roll.  Alternatively the caller could always do the roll.
> 
> I think I got it right, but error handeling is a bit weird this way,
> looks logically identical.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 
> Thanks for this extensive review =)
>
> +	if (error)
> +		goto error1;
> +
> +	if (XFS_IFORK_Q(ip))
> +		goto error1;
> +
> +	ASSERT(ip->i_d.di_anextents == 0);
> +	VN_HOLD(XFS_ITOV(ip));
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +
> +	error = xfs_trans_bmap_add_attrfork(NULL, ip, size, rsvd);
> +	if (error)
> +		return error;
> +	return xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);

I was kind of expecting the transaction, &tp, to be passed into
xfs_trans_bmap_add_attrfork().
(And then xfs_trans_bmap_add_attrfork() which calls
xfs_bmap_finish() which calls xfs_trans_dup() and so from that
point on we would then have to update tp if we were to use it.)

So how come we pass in NULL?
I'm tired so I'm probably missing something obvious.

--Tim

Re: [PATCH] Introduce xfs_trans_bmap_add_attrfork.

[Niv Sardi]

Timothy Shimmin <tes@sgi.com> writes:
[...]
>> +	if (error)
>> +		goto error1;
>> +
>> +	if (XFS_IFORK_Q(ip))
>> +		goto error1;
>> +
>> +	ASSERT(ip->i_d.di_anextents == 0);
>> +	VN_HOLD(XFS_ITOV(ip));
>> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>> +
>> +	error = xfs_trans_bmap_add_attrfork(NULL, ip, size, rsvd);
>> +	if (error)
>> +		return error;
>> +	return xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
>
> I was kind of expecting the transaction, &tp, to be passed into
> xfs_trans_bmap_add_attrfork().
> (And then xfs_trans_bmap_add_attrfork() which calls
> xfs_bmap_finish() which calls xfs_trans_dup() and so from that
> point on we would then have to update tp if we were to use it.)
>
> So how come we pass in NULL?
> I'm tired so I'm probably missing something obvious.

No you're right, it took me a while to remember you're working out of
git. I haven't really re-read it properly yet but thanks for this one.

Cheers,
-- 
Niv Sardi

Re: [PATCH] Introduce xfs_trans_bmap_add_attrfork.

[Dave Chinner]

On Mon, Jun 23, 2008 at 02:42:29PM +1000, Niv Sardi wrote:
> That takes a transaction and doesn't require everything to be locked
> anymore.  this uses the roll trans call.
.....
>  error2:
> +	if (tpp)
> +		tpp = &tp;

That's clearly busted.

	if (tpp)
		*tpp = tp;

>  	xfs_bmap_cancel(&flist);
>  error1:
>  	ASSERT(ismrlocked(&ip->i_lock,MR_UPDATE));
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	if (tpp)
> +		tpp = &tp;

ditto.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] Move xfs_attr_rolltrans to xfs_trans_roll

[Christoph Hellwig]

> -		if ((error = xfs_attr_rolltrans(&args.trans, dp)))
> +
> +		error = xfs_trans_roll(&args.trans, dp);
> +		if (error)
>  			goto out;
>  
>  	}
> @@ -1019,7 +1021,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
>  		 * Commit the current trans (including the inode) and start
>  		 * a new one.
>  		 */
> -		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
> +		if ((error = xfs_trans_roll(&args->trans, dp)))

Please do the transformation to move all assignments out of the
coditionals everywhere.

> index b08e2a2..9465807 100644
> --- a/fs/xfs/xfs_attr_leaf.c
> +++ b/fs/xfs/xfs_attr_leaf.c
> @@ -2543,7 +2543,7 @@ xfs_attr_leaf_clearflag(xfs_da_args_t *args)
>  	/*
>  	 * Commit the flag value change and start the next trans in series.
>  	 */
> -	error = xfs_attr_rolltrans(&args->trans, args->dp);
> +	error = xfs_trans_roll(&args->trans, args->dp);
>  
>  	return(error);

	return xfs_trans_roll(&args->trans, args->dp);

> +/*
> + * Roll from one trans in the sequence of PERMANENT transactions to the next.
> + */

The comment could be a little more verbose :)

> +int
> +xfs_trans_roll(
> +	xfs_trans_t	**tpp,
> +	xfs_inode_t	*dp)
> +{
> +	xfs_trans_t	*trans;
> +	unsigned int	logres, count;
> +	int		error;

Please convert this to the struct types and canonical variable names
for the given types:

int
xfs_trans_roll(
	struct xfs_trans	*tpp,
	struct xfs_inode	*ip)
{
	struct xfs_trans	*tp;


> +	if ((error = xfs_trans_commit(trans, 0)))
> +		return (error);

	error = xfs_trans_commit(trans, 0);
	if (error)
		return error;

> +	error = xfs_trans_reserve(trans, 0, logres, 0,
> +				  XFS_TRANS_PERM_LOG_RES, count);
> +	/*
> +	 *  Ensure that the inode is in the new transaction and locked.
> +	 */
> +	if (!error) {
> +		xfs_trans_ijoin(trans, dp, XFS_ILOCK_EXCL);
> +		xfs_trans_ihold(trans, dp);
> +	}
> +	return (error);

	if (error)
		return error;

	xfs_trans_ijoin(trans, ip, XFS_ILOCK_EXCL);
	xfs_trans_ihold(trans, ip);
	return 0;

Re: [PATCH] Move xfs_attr_rolltrans to xfs_trans_roll

[Niv Sardi]

[-- Attachment #1: Type: text/plain, Size: 98 bytes --]


New patch attached, assignemnts out of conditionals, updated comments
and types.


-- 
Niv Sardi

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Move-attr-log-alloc-size-calculator-to-another-funct.patch --]
[-- Type: text/x-diff, Size: 4641 bytes --]

>From 1ca0cbca566944d8d26027d4b877be11b2ebaad0 Mon Sep 17 00:00:00 2001
From: Niv Sardi <xaiki@sgi.com>
Date: Thu, 8 May 2008 16:34:32 +1000
Subject: [PATCH] Move attr log alloc size calculator to another function.

We will need that to be able to calculate the size of log we need for
a specific attr (for Create+EA). The local flag is needed so that we
can fail if we run into ENOSPC when trying to alloc blocks.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c |   80 +++++++++++++++++++++++++++++++---------------------
 fs/xfs/xfs_attr.h |    1 +
 2 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index e58f321..ca3cabf 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -184,6 +184,46 @@ xfs_attr_get(
 	return(error);
 }
 
+/*
+ * Calculate how many blocks we need for the new attribute,
+ */
+int
+xfs_attr_calc_size(
+	struct xfs_inode *ip,
+	int		namelen,
+	int		valuelen,
+	int		*local)
+{
+	struct xfs_mount *mp = ip->i_mount;
+	int		size;
+	int		nblks;
+
+	/*
+	 * Determine space new attribute will use, and if it would be
+	 * "local" or "remote" (note: local != inline).
+	 */
+	size = xfs_attr_leaf_newentsize(namelen, valuelen,
+					mp->m_sb.sb_blocksize, local);
+
+	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
+	if (*local) {
+		if (size > (mp->m_sb.sb_blocksize >> 1)) {
+			/* Double split possible */
+			nblks *= 2;
+		}
+	} else {
+		/*
+		 * Out of line attribute, cannot double split, but
+		 * make room for the attribute value itself.
+		 */
+		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
+		nblks += dblocks;
+		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
+	}
+
+	return nblks;
+}
+
 int
 xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 		 char *value, int valuelen, int flags)
@@ -192,10 +232,9 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	xfs_fsblock_t	firstblock;
 	xfs_bmap_free_t flist;
 	int		error, err2, committed;
-	int		local, size;
-	uint		nblks;
 	xfs_mount_t	*mp = dp->i_mount;
 	int             rsvd = (flags & ATTR_ROOT) != 0;
+	int		local;
 
 	/*
 	 * Attach the dquots to the inode.
@@ -232,30 +271,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	args.addname = 1;
 	args.oknoent = 1;
 
-	/*
-	 * Determine space new attribute will use, and if it would be
-	 * "local" or "remote" (note: local != inline).
-	 */
-	size = xfs_attr_leaf_newentsize(namelen, valuelen,
-					mp->m_sb.sb_blocksize, &local);
-
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	if (local) {
-		if (size > (mp->m_sb.sb_blocksize >> 1)) {
-			/* Double split possible */
-			nblks <<= 1;
-		}
-	} else {
-		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
-		/* Out of line attribute, cannot double split, but make
-		 * room for the attribute value itself.
-		 */
-		nblks += dblocks;
-		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
-	}
-
 	/* Size is now blocks for attribute data */
-	args.total = nblks;
+	args.total = xfs_attr_calc_size(dp, namelen, valuelen, &local);
 
 	/*
 	 * Start our first transaction of the day.
@@ -277,18 +294,17 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	if (rsvd)
 		args.trans->t_flags |= XFS_TRANS_RESERVE;
 
-	if ((error = xfs_trans_reserve(args.trans, (uint) nblks,
-				      XFS_ATTRSET_LOG_RES(mp, nblks),
-				      0, XFS_TRANS_PERM_LOG_RES,
-				      XFS_ATTRSET_LOG_COUNT))) {
+	if ((error = xfs_trans_reserve(args.trans, args.total,
+			XFS_ATTRSET_LOG_RES(mp, args.total), 0,
+			XFS_TRANS_PERM_LOG_RES, XFS_ATTRSET_LOG_COUNT))) {
 		xfs_trans_cancel(args.trans, 0);
 		return(error);
 	}
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, nblks, 0,
-			 rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-				XFS_QMOPT_RES_REGBLKS);
+	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, args.total, 0,
+				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+				       XFS_QMOPT_RES_REGBLKS);
 	if (error) {
 		xfs_iunlock(dp, XFS_ILOCK_EXCL);
 		xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index 786eba3..ea457f5 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -158,6 +158,7 @@ struct xfs_da_args;
 /*
  * Overall external interface routines.
  */
+int xfs_attr_calc_size(struct xfs_inode *, int, int, int *);
 int xfs_attr_set_int(struct xfs_inode *, const char *, int, char *, int, int);
 int xfs_attr_remove_int(struct xfs_inode *, const char *, int, int);
 int xfs_attr_list_int(struct xfs_attr_list_context *);
-- 
1.5.6

Re: [PATCH] Move xfs_attr_rolltrans to xfs_trans_roll

[Christoph Hellwig]

Looks good except for a tiny nitpick:

> +/*
> + * Calculate how many blocks we need for the new attribute,
> + */
> +int
> +xfs_attr_calc_size(
> +	struct xfs_inode *ip,
> +	int		namelen,
> +	int		valuelen,
> +	int		*local)
> +{
> +	struct xfs_mount *mp = ip->i_mount;

Add another tab before the variable names so that it aligns nicely.

Re: [PATCH] Move xfs_attr_rolltrans to xfs_trans_roll

[Christoph Hellwig]

But this isn't actually the patch described by the subject..

On Fri, Jun 27, 2008 at 09:03:10AM -0400, Christoph Hellwig wrote:
> Looks good except for a tiny nitpick:
> 
> > +/*
> > + * Calculate how many blocks we need for the new attribute,
> > + */
> > +int
> > +xfs_attr_calc_size(
> > +	struct xfs_inode *ip,
> > +	int		namelen,
> > +	int		valuelen,
> > +	int		*local)
> > +{
> > +	struct xfs_mount *mp = ip->i_mount;
> 
> Add another tab before the variable names so that it aligns nicely.

Re: [PATCH] Move xfs_attr_rolltrans to xfs_trans_roll

[Timothy Shimmin]

Niv Sardi wrote:
> Move it from the attr code to the transaction code and make the attr
> code call the new function.
> 
> We rolltrans is really usefull whenever we want to use rolling
> transaction, should be generic, it isn't dependent on any part of the
> attr code anyway.
> 
So you just have some renames on calls and change where func definition is located.
And you've added comments.


> +/*
> + * Roll from one trans in the sequence of PERMANENT transactions to
> + * the next: permanent transactions are only flushed out when commited
> + * with XFS_TRANS_RELEASE_LOG_RES, but we still want as soon as
> + * possible to let chunks of it go to the log. So we commit the chunck
> + * we've been working on and get a new transaction to continue.
> + */

typos:
s/chunck/chunk/
s/commited/committed/

--Tim

Re: [PATCH] Move attr log alloc size calculator to another function.

[Christoph Hellwig]

On Mon, Jun 23, 2008 at 02:42:27PM +1000, Niv Sardi wrote:
> From: Niv Sardi <xaiki@debian.org>
> 
> We will need that to be able to calculate the size of log we need for
> a specific attr (for parent pointers in create). We need the local so
> that we can fail if we run into ENOSPC when trying to alloc blocks
> 
> Signed-off-by: Niv Sardi <xaiki@sgi.com>
> ---
>  fs/xfs/xfs_attr.c |   78 +++++++++++++++++++++++++++++++---------------------
>  fs/xfs/xfs_attr.h |    2 +-
>  2 files changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index e58f321..0d19e90 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -185,6 +185,43 @@ xfs_attr_get(
>  }
>  
>  int
> +xfs_attr_calc_size(

should be marked STATIC, and a little comment describing what
the function does would help, too.

> +	xfs_inode_t	*ip,

please use struct xfs_inode instead of the typedef for new code.

> +	int		namelen,
> +	int		valuelen,
> +	int		*local)
> +{
> +	xfs_mount_t	*mp = ip->i_mount;

Same here, should be struct xfs_mount

> +	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
> +	if (*local) {
> +		if (size > (mp->m_sb.sb_blocksize >> 1)) {
> +			/* Double split possible */
> +			nblks <<= 1;
> +		}

The comment cold be a little more verbose, and using a *= 2 would
be more descriptive (the compiler will optimize it to a shift anyway)

> -	if ((error = xfs_trans_reserve(args.trans, (uint) nblks,
> -				      XFS_ATTRSET_LOG_RES(mp, nblks),
> -				      0, XFS_TRANS_PERM_LOG_RES,
> -				      XFS_ATTRSET_LOG_COUNT))) {
> +	if ((error = xfs_trans_reserve(args.trans, (uint) args.total,
> +				       XFS_ATTRSET_LOG_RES(mp, args.total),
> +				       0, XFS_TRANS_PERM_LOG_RES,
> +				       XFS_ATTRSET_LOG_COUNT))) {

When you touch this anyway please rewrite it to the canonical form:

	error = xfs_trans_reserve(args.trans, args.total,
			XFS_ATTRSET_LOG_RES(mp, args.total), 0,
			XFS_TRANS_PERM_LOG_RES, XFS_ATTRSET_LOG_COUNT);
	if (error) {

With these little tidyups this looks good enough as a cleanup that can
go in anytime.

Re: [PATCH] Move attr log alloc size calculator to another function.

[Niv Sardi]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

Attached updated patch.

Christoph Hellwig <hch@infradead.org> writes:
> On Mon, Jun 23, 2008 at 02:42:27PM +1000, Niv Sardi wrote:
>> From: Niv Sardi <xaiki@debian.org>
>> 
>> We will need that to be able to calculate the size of log we need for
>> a specific attr (for parent pointers in create). We need the local so
>> that we can fail if we run into ENOSPC when trying to alloc blocks

Updated Comments, structs instead of typdefs
 
>> Signed-off-by: Niv Sardi <xaiki@sgi.com>
>> ---
>>  fs/xfs/xfs_attr.c |   78 +++++++++++++++++++++++++++++++---------------------
>>  fs/xfs/xfs_attr.h |    2 +-
>>  2 files changed, 47 insertions(+), 33 deletions(-)
>> 
>> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
>> index e58f321..0d19e90 100644
>> --- a/fs/xfs/xfs_attr.c
>> +++ b/fs/xfs/xfs_attr.c
>> @@ -185,6 +185,43 @@ xfs_attr_get(
>>  }
>>  
>>  int
>> +xfs_attr_calc_size(
>
> should be marked STATIC,

The whole idea is to be able to use it in xfs_create().

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Move-attr-log-alloc-size-calculator-to-another-funct.patch --]
[-- Type: text/x-diff, Size: 4641 bytes --]

>From 1ca0cbca566944d8d26027d4b877be11b2ebaad0 Mon Sep 17 00:00:00 2001
From: Niv Sardi <xaiki@sgi.com>
Date: Thu, 8 May 2008 16:34:32 +1000
Subject: [PATCH] Move attr log alloc size calculator to another function.

We will need that to be able to calculate the size of log we need for
a specific attr (for Create+EA). The local flag is needed so that we
can fail if we run into ENOSPC when trying to alloc blocks.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c |   80 +++++++++++++++++++++++++++++++---------------------
 fs/xfs/xfs_attr.h |    1 +
 2 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index e58f321..ca3cabf 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -184,6 +184,46 @@ xfs_attr_get(
 	return(error);
 }
 
+/*
+ * Calculate how many blocks we need for the new attribute,
+ */
+int
+xfs_attr_calc_size(
+	struct xfs_inode *ip,
+	int		namelen,
+	int		valuelen,
+	int		*local)
+{
+	struct xfs_mount *mp = ip->i_mount;
+	int		size;
+	int		nblks;
+
+	/*
+	 * Determine space new attribute will use, and if it would be
+	 * "local" or "remote" (note: local != inline).
+	 */
+	size = xfs_attr_leaf_newentsize(namelen, valuelen,
+					mp->m_sb.sb_blocksize, local);
+
+	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
+	if (*local) {
+		if (size > (mp->m_sb.sb_blocksize >> 1)) {
+			/* Double split possible */
+			nblks *= 2;
+		}
+	} else {
+		/*
+		 * Out of line attribute, cannot double split, but
+		 * make room for the attribute value itself.
+		 */
+		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
+		nblks += dblocks;
+		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
+	}
+
+	return nblks;
+}
+
 int
 xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 		 char *value, int valuelen, int flags)
@@ -192,10 +232,9 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	xfs_fsblock_t	firstblock;
 	xfs_bmap_free_t flist;
 	int		error, err2, committed;
-	int		local, size;
-	uint		nblks;
 	xfs_mount_t	*mp = dp->i_mount;
 	int             rsvd = (flags & ATTR_ROOT) != 0;
+	int		local;
 
 	/*
 	 * Attach the dquots to the inode.
@@ -232,30 +271,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	args.addname = 1;
 	args.oknoent = 1;
 
-	/*
-	 * Determine space new attribute will use, and if it would be
-	 * "local" or "remote" (note: local != inline).
-	 */
-	size = xfs_attr_leaf_newentsize(namelen, valuelen,
-					mp->m_sb.sb_blocksize, &local);
-
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	if (local) {
-		if (size > (mp->m_sb.sb_blocksize >> 1)) {
-			/* Double split possible */
-			nblks <<= 1;
-		}
-	} else {
-		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
-		/* Out of line attribute, cannot double split, but make
-		 * room for the attribute value itself.
-		 */
-		nblks += dblocks;
-		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
-	}
-
 	/* Size is now blocks for attribute data */
-	args.total = nblks;
+	args.total = xfs_attr_calc_size(dp, namelen, valuelen, &local);
 
 	/*
 	 * Start our first transaction of the day.
@@ -277,18 +294,17 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int namelen,
 	if (rsvd)
 		args.trans->t_flags |= XFS_TRANS_RESERVE;
 
-	if ((error = xfs_trans_reserve(args.trans, (uint) nblks,
-				      XFS_ATTRSET_LOG_RES(mp, nblks),
-				      0, XFS_TRANS_PERM_LOG_RES,
-				      XFS_ATTRSET_LOG_COUNT))) {
+	if ((error = xfs_trans_reserve(args.trans, args.total,
+			XFS_ATTRSET_LOG_RES(mp, args.total), 0,
+			XFS_TRANS_PERM_LOG_RES, XFS_ATTRSET_LOG_COUNT))) {
 		xfs_trans_cancel(args.trans, 0);
 		return(error);
 	}
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, nblks, 0,
-			 rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-				XFS_QMOPT_RES_REGBLKS);
+	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, args.total, 0,
+				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+				       XFS_QMOPT_RES_REGBLKS);
 	if (error) {
 		xfs_iunlock(dp, XFS_ILOCK_EXCL);
 		xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index 786eba3..ea457f5 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -158,6 +158,7 @@ struct xfs_da_args;
 /*
  * Overall external interface routines.
  */
+int xfs_attr_calc_size(struct xfs_inode *, int, int, int *);
 int xfs_attr_set_int(struct xfs_inode *, const char *, int, char *, int, int);
 int xfs_attr_remove_int(struct xfs_inode *, const char *, int, int);
 int xfs_attr_list_int(struct xfs_attr_list_context *);
-- 
1.5.6


[-- Attachment #3: Type: text/plain, Size: 23 bytes --]


Cheers,
-- 
Niv Sardi

Re: [PATCH] Move attr log alloc size calculator to another function.

[Timothy Shimmin]

Niv Sardi wrote:
> Attached updated patch.
> 
> Christoph Hellwig <hch@infradead.org> writes:
>> On Mon, Jun 23, 2008 at 02:42:27PM +1000, Niv Sardi wrote:
>>> From: Niv Sardi <xaiki@debian.org>
>>>
>>> We will need that to be able to calculate the size of log we need for
>>> a specific attr (for parent pointers in create). We need the local so
>>> that we can fail if we run into ENOSPC when trying to alloc blocks
> 
> Updated Comments, structs instead of typdefs
>  
>>> Signed-off-by: Niv Sardi <xaiki@sgi.com>
>>> ---
>>>  fs/xfs/xfs_attr.c |   78 +++++++++++++++++++++++++++++++---------------------
>>>  fs/xfs/xfs_attr.h |    2 +-
>>>  2 files changed, 47 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
>>> index e58f321..0d19e90 100644
>>> --- a/fs/xfs/xfs_attr.c
>>> +++ b/fs/xfs/xfs_attr.c
>>> @@ -185,6 +185,43 @@ xfs_attr_get(
>>>  }
>>>  
>>>  int
>>> +xfs_attr_calc_size(
>> should be marked STATIC,
> 
> The whole idea is to be able to use it in xfs_create().
>
I guess in isolation it just looks weird as the only caller is within
the file.
In isolation it would make sense to be STATIC.
(Then again, in isolation, it looks strange returning the "local" parameter -
 as you said, you need it elsewhere).
And I guess, Christoph's point was that it could go in as an isolated
cleanup patch if it was made static.

--Tim

[UPDATED RFC] Create with EA initial work

[Niv Sardi]

Thanks to everyone for the review of the former patchset, this ones
fixes the bugs found by dgc, changes the naming of the *_trans_*
functions, splits out xfs_bmap_add_attrfork{,_trans} to have one
version doing the whole allocation and calling into the other as
suggested by hch, and has the x_t -> struct x changes too.

There is a bug in this, that I can see with the Parent Pointers code
going on top of it (that will be posted soon), it is basically calling
xfs_attr_set_int_trans() in xfs_create() just before the last
commit. For some reason, the first call to xfs_roll_trans (after
xfs_bmap_add_attrfork_trans()) will complain about the inode being
unlocked after xfs_trans_commit(). I understand I need to call
xfs_trans_ihold(ip) on it, but we already do in xfs_create() so I
think I must be missing something else… any ideas ?

[PATCH] Move attr log alloc size calculator to another function.

[Niv Sardi]

We will need that to be able to calculate the size of log we need for
a specific attr (for Create+EA). The local flag is needed so that we
can fail if we run into ENOSPC when trying to alloc blocks.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c |   80 +++++++++++++++++++++++++++++++---------------------
 fs/xfs/xfs_attr.h |    1 +
 2 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index df151a8..4036bdc 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -196,6 +196,46 @@ xfs_attr_get(
 	return(error);
 }
 
+/*
+ * Calculate how many blocks we need for the new attribute,
+ */
+int
+xfs_attr_calc_size(
+	struct xfs_inode 	*ip,
+	int			namelen,
+	int			valuelen,
+	int			*local)
+{
+	struct xfs_mount 	*mp = ip->i_mount;
+	int			size;
+	int			nblks;
+
+	/*
+	 * Determine space new attribute will use, and if it would be
+	 * "local" or "remote" (note: local != inline).
+	 */
+	size = xfs_attr_leaf_newentsize(namelen, valuelen,
+					mp->m_sb.sb_blocksize, local);
+
+	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
+	if (*local) {
+		if (size > (mp->m_sb.sb_blocksize >> 1)) {
+			/* Double split possible */
+			nblks *= 2;
+		}
+	} else {
+		/*
+		 * Out of line attribute, cannot double split, but
+		 * make room for the attribute value itself.
+		 */
+		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
+		nblks += dblocks;
+		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
+	}
+
+	return nblks;
+}
+
 STATIC int
 xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 		char *value, int valuelen, int flags)
@@ -204,10 +244,9 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	xfs_fsblock_t	firstblock;
 	xfs_bmap_free_t flist;
 	int		error, err2, committed;
-	int		local, size;
-	uint		nblks;
 	xfs_mount_t	*mp = dp->i_mount;
 	int             rsvd = (flags & ATTR_ROOT) != 0;
+	int		local;
 
 	/*
 	 * Attach the dquots to the inode.
@@ -244,30 +283,8 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	args.addname = 1;
 	args.oknoent = 1;
 
-	/*
-	 * Determine space new attribute will use, and if it would be
-	 * "local" or "remote" (note: local != inline).
-	 */
-	size = xfs_attr_leaf_newentsize(name->len, valuelen,
-					mp->m_sb.sb_blocksize, &local);
-
-	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);
-	if (local) {
-		if (size > (mp->m_sb.sb_blocksize >> 1)) {
-			/* Double split possible */
-			nblks <<= 1;
-		}
-	} else {
-		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
-		/* Out of line attribute, cannot double split, but make
-		 * room for the attribute value itself.
-		 */
-		nblks += dblocks;
-		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
-	}
-
 	/* Size is now blocks for attribute data */
-	args.total = nblks;
+	args.total = xfs_attr_calc_size(dp, name->len, valuelen, &local);
 
 	/*
 	 * Start our first transaction of the day.
@@ -289,18 +306,17 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	if (rsvd)
 		args.trans->t_flags |= XFS_TRANS_RESERVE;
 
-	if ((error = xfs_trans_reserve(args.trans, (uint) nblks,
-				      XFS_ATTRSET_LOG_RES(mp, nblks),
-				      0, XFS_TRANS_PERM_LOG_RES,
-				      XFS_ATTRSET_LOG_COUNT))) {
+	if ((error = xfs_trans_reserve(args.trans, args.total,
+			XFS_ATTRSET_LOG_RES(mp, args.total), 0,
+			XFS_TRANS_PERM_LOG_RES, XFS_ATTRSET_LOG_COUNT))) {
 		xfs_trans_cancel(args.trans, 0);
 		return(error);
 	}
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, nblks, 0,
-			 rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-				XFS_QMOPT_RES_REGBLKS);
+	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, args.total, 0,
+				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+				       XFS_QMOPT_RES_REGBLKS);
 	if (error) {
 		xfs_iunlock(dp, XFS_ILOCK_EXCL);
 		xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index 6cfc938..f99395c 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -158,6 +158,7 @@ struct xfs_da_args;
 /*
  * Overall external interface routines.
  */
+int xfs_attr_calc_size(struct xfs_inode *, int, int, int *);
 int xfs_attr_inactive(struct xfs_inode *dp);
 
 int xfs_attr_shortform_getvalue(struct xfs_da_args *);
-- 
1.5.6.2

[PATCH] Move xfs_attr_rolltrans to xfs_trans_roll

[Niv Sardi]

Move it from the attr code to the transaction code and make the attr
code call the new function.

We rolltrans is really usefull whenever we want to use rolling
transaction, should be generic, it isn't dependent on any part of the
attr code anyway.

We use this excuse to change all the:
 if ((error = xfs_attr_rolltrans()))

calls into:
 error = xfs_trans_roll();
 if (error)

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c      |   30 +++++++++++++------
 fs/xfs/xfs_attr_leaf.c |   73 +++++------------------------------------------
 fs/xfs/xfs_attr_leaf.h |    2 -
 fs/xfs/xfs_trans.c     |   63 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_trans.h     |    1 +
 5 files changed, 92 insertions(+), 77 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 4036bdc..4888a35 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -403,7 +403,9 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 		 * Commit the leaf transformation.  We'll need another (linked)
 		 * transaction to add the new attribute to the leaf.
 		 */
-		if ((error = xfs_attr_rolltrans(&args.trans, dp)))
+
+		error = xfs_trans_roll(&args.trans, dp);
+		if (error)
 			goto out;
 
 	}
@@ -1035,7 +1037,8 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 		 * Commit the current trans (including the inode) and start
 		 * a new one.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		error = xfs_trans_roll(&args->trans, dp);
+		if (error)
 			return (error);
 
 		/*
@@ -1049,7 +1052,8 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 	 * Commit the transaction that added the attr name so that
 	 * later routines can manage their own transactions.
 	 */
-	if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+	error = xfs_trans_roll(&args->trans, dp);
+	if (error)
 		return (error);
 
 	/*
@@ -1138,7 +1142,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
 		/*
 		 * Commit the remove and start the next trans in series.
 		 */
-		error = xfs_attr_rolltrans(&args->trans, dp);
+		error = xfs_trans_roll(&args->trans, dp);
 
 	} else if (args->rmtblkno > 0) {
 		/*
@@ -1369,7 +1373,8 @@ restart:
 			 * Commit the node conversion and start the next
 			 * trans in the chain.
 			 */
-			if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+			error = xfs_trans_roll(&args->trans, dp);
+			if (error)
 				goto out;
 
 			goto restart;
@@ -1420,7 +1425,8 @@ restart:
 	 * Commit the leaf addition or btree split and start the next
 	 * trans in the chain.
 	 */
-	if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+	error = xfs_trans_roll(&args->trans, dp);
+	if (error)
 		goto out;
 
 	/*
@@ -1520,7 +1526,8 @@ restart:
 		/*
 		 * Commit and start the next trans in the chain.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		error = xfs_trans_roll(&args->trans, dp);
+		if (error)
 			goto out;
 
 	} else if (args->rmtblkno > 0) {
@@ -1652,7 +1659,8 @@ xfs_attr_node_removename(xfs_da_args_t *args)
 		/*
 		 * Commit the Btree join operation and start a new trans.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		error = xfs_trans_roll(&args->trans, dp);
+		if (error)
 			goto out;
 	}
 
@@ -2153,7 +2161,8 @@ xfs_attr_rmtval_set(xfs_da_args_t *args)
 		/*
 		 * Start the next trans in the chain.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, dp)))
+		error = xfs_trans_roll(&args->trans, dp);
+		if (error)
 			return (error);
 	}
 
@@ -2303,7 +2312,8 @@ xfs_attr_rmtval_remove(xfs_da_args_t *args)
 		/*
 		 * Close out trans and start the next one in the chain.
 		 */
-		if ((error = xfs_attr_rolltrans(&args->trans, args->dp)))
+		error = xfs_trans_roll(&args->trans, args->dp);
+		if (error)
 			return (error);
 	}
 	return(0);
diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c
index 303d41e..36aa10c 100644
--- a/fs/xfs/xfs_attr_leaf.c
+++ b/fs/xfs/xfs_attr_leaf.c
@@ -2543,9 +2543,7 @@ xfs_attr_leaf_clearflag(xfs_da_args_t *args)
 	/*
 	 * Commit the flag value change and start the next trans in series.
 	 */
-	error = xfs_attr_rolltrans(&args->trans, args->dp);
-
-	return(error);
+	return xfs_trans_roll(&args->trans, args->dp);
 }
 
 /*
@@ -2592,7 +2590,7 @@ xfs_attr_leaf_setflag(xfs_da_args_t *args)
 	/*
 	 * Commit the flag value change and start the next trans in series.
 	 */
-	error = xfs_attr_rolltrans(&args->trans, args->dp);
+	error = xfs_trans_roll(&args->trans, args->dp);
 
 	return(error);
 }
@@ -2710,7 +2708,7 @@ xfs_attr_leaf_flipflags(xfs_da_args_t *args)
 	/*
 	 * Commit the flag value change and start the next trans in series.
 	 */
-	error = xfs_attr_rolltrans(&args->trans, args->dp);
+	error = xfs_trans_roll(&args->trans, args->dp);
 
 	return(error);
 }
@@ -2768,7 +2766,7 @@ xfs_attr_root_inactive(xfs_trans_t **trans, xfs_inode_t *dp)
 	/*
 	 * Commit the invalidate and start the next transaction.
 	 */
-	error = xfs_attr_rolltrans(trans, dp);
+	error = xfs_trans_roll(trans, dp);
 
 	return (error);
 }
@@ -2870,7 +2868,8 @@ xfs_attr_node_inactive(xfs_trans_t **trans, xfs_inode_t *dp, xfs_dabuf_t *bp,
 		/*
 		 * Atomically commit the whole invalidate stuff.
 		 */
-		if ((error = xfs_attr_rolltrans(trans, dp)))
+		error = xfs_trans_roll(trans, dp);
+		if (error)
 			return (error);
 	}
 
@@ -3009,7 +3008,8 @@ xfs_attr_leaf_freextent(xfs_trans_t **trans, xfs_inode_t *dp,
 			/*
 			 * Roll to next transaction.
 			 */
-			if ((error = xfs_attr_rolltrans(trans, dp)))
+			error = xfs_trans_roll(trans, dp);
+			if (error)
 				return (error);
 		}
 
@@ -3019,60 +3019,3 @@ xfs_attr_leaf_freextent(xfs_trans_t **trans, xfs_inode_t *dp,
 
 	return(0);
 }
-
-
-/*
- * Roll from one trans in the sequence of PERMANENT transactions to the next.
- */
-int
-xfs_attr_rolltrans(xfs_trans_t **transp, xfs_inode_t *dp)
-{
-	xfs_trans_t *trans;
-	unsigned int logres, count;
-	int	error;
-
-	/*
-	 * Ensure that the inode is always logged.
-	 */
-	trans = *transp;
-	xfs_trans_log_inode(trans, dp, XFS_ILOG_CORE);
-
-	/*
-	 * Copy the critical parameters from one trans to the next.
-	 */
-	logres = trans->t_log_res;
-	count = trans->t_log_count;
-	*transp = xfs_trans_dup(trans);
-
-	/*
-	 * Commit the current transaction.
-	 * If this commit failed, then it'd just unlock those items that
-	 * are not marked ihold. That also means that a filesystem shutdown
-	 * is in progress. The caller takes the responsibility to cancel
-	 * the duplicate transaction that gets returned.
-	 */
-	if ((error = xfs_trans_commit(trans, 0)))
-		return (error);
-
-	trans = *transp;
-
-	/*
-	 * Reserve space in the log for th next transaction.
-	 * This also pushes items in the "AIL", the list of logged items,
-	 * out to disk if they are taking up space at the tail of the log
-	 * that we want to use.  This requires that either nothing be locked
-	 * across this call, or that anything that is locked be logged in
-	 * the prior and the next transactions.
-	 */
-	error = xfs_trans_reserve(trans, 0, logres, 0,
-				  XFS_TRANS_PERM_LOG_RES, count);
-	/*
-	 *  Ensure that the inode is in the new transaction and locked.
-	 */
-	if (!error) {
-		xfs_trans_ijoin(trans, dp, XFS_ILOCK_EXCL);
-		xfs_trans_ihold(trans, dp);
-	}
-	return (error);
-
-}
diff --git a/fs/xfs/xfs_attr_leaf.h b/fs/xfs/xfs_attr_leaf.h
index 040f732..698e078 100644
--- a/fs/xfs/xfs_attr_leaf.h
+++ b/fs/xfs/xfs_attr_leaf.h
@@ -301,6 +301,4 @@ int	xfs_attr_leaf_order(struct xfs_dabuf *leaf1_bp,
 				   struct xfs_dabuf *leaf2_bp);
 int	xfs_attr_leaf_newentsize(int namelen, int valuelen, int blocksize,
 					int *local);
-int	xfs_attr_rolltrans(struct xfs_trans **transp, struct xfs_inode *dp);
-
 #endif	/* __XFS_ATTR_LEAF_H__ */
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 1403864..8403eac 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -43,6 +43,7 @@
 #include "xfs_quota.h"
 #include "xfs_trans_priv.h"
 #include "xfs_trans_space.h"
+#include "xfs_inode_item.h"
 
 
 STATIC void	xfs_trans_apply_sb_deltas(xfs_trans_t *);
@@ -1216,6 +1217,68 @@ xfs_trans_free(
 	kmem_zone_free(xfs_trans_zone, tp);
 }
 
+/*
+ * Roll from one trans in the sequence of PERMANENT transactions to
+ * the next: permanent transactions are only flushed out when
+ * committed with XFS_TRANS_RELEASE_LOG_RES, but we still want as soon
+ * as possible to let chunks of it go to the log. So we commit the
+ * chunk we've been working on and get a new transaction to continue.
+ */
+int
+xfs_trans_roll(
+	struct xfs_trans	**tpp,
+	struct xfs_inode	*dp)
+{
+	struct xfs_trans	*trans;
+	unsigned int		logres, count;
+	int			error;
+
+	/*
+	 * Ensure that the inode is always logged.
+	 */
+	trans = *tpp;
+	xfs_trans_log_inode(trans, dp, XFS_ILOG_CORE);
+
+	/*
+	 * Copy the critical parameters from one trans to the next.
+	 */
+	logres = trans->t_log_res;
+	count = trans->t_log_count;
+	*tpp = xfs_trans_dup(trans);
+
+	/*
+	 * Commit the current transaction.
+	 * If this commit failed, then it'd just unlock those items that
+	 * are not marked ihold. That also means that a filesystem shutdown
+	 * is in progress. The caller takes the responsibility to cancel
+	 * the duplicate transaction that gets returned.
+	 */
+	error = xfs_trans_commit(trans, 0);
+	if (error)
+		return (error);
+
+	trans = *tpp;
+
+	/*
+	 * Reserve space in the log for th next transaction.
+	 * This also pushes items in the "AIL", the list of logged items,
+	 * out to disk if they are taking up space at the tail of the log
+	 * that we want to use.  This requires that either nothing be locked
+	 * across this call, or that anything that is locked be logged in
+	 * the prior and the next transactions.
+	 */
+	error = xfs_trans_reserve(trans, 0, logres, 0,
+				  XFS_TRANS_PERM_LOG_RES, count);
+	/*
+	 *  Ensure that the inode is in the new transaction and locked.
+	 */
+	if (error)
+		return error;
+
+	xfs_trans_ijoin(trans, dp, XFS_ILOCK_EXCL);
+	xfs_trans_ihold(trans, dp);
+	return 0;
+}
 
 /*
  * THIS SHOULD BE REWRITTEN TO USE xfs_trans_next_item().
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 0804207..9161e99 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -986,6 +986,7 @@ int		_xfs_trans_commit(xfs_trans_t *,
 				  int *);
 #define xfs_trans_commit(tp, flags)	_xfs_trans_commit(tp, flags, NULL)
 void		xfs_trans_cancel(xfs_trans_t *, int);
+int		xfs_trans_roll(struct xfs_trans **, struct xfs_inode *);
 int		xfs_trans_ail_init(struct xfs_mount *);
 void		xfs_trans_ail_destroy(struct xfs_mount *);
 void		xfs_trans_push_ail(struct xfs_mount *, xfs_lsn_t);
-- 
1.5.6.2

[PATCH] Introduce xfs_bmap_add_attrfork_trans.

[Niv Sardi]

That takes a transaction and doesn't require everything to be locked
anymore. This doesn't commit the transaction ! so direct callers,
willing to use xfs_trans_roll() should do it themselves.

Change xfs_bmap_add_attrfork to do the initialization/allocation of
the transaction and commit arround xfs_bmap_add_attrfork_trans.

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_bmap.c |  107 ++++++++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_bmap.h |   11 +++++
 2 files changed, 81 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 53c259f..231c8bc 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -3941,20 +3941,20 @@ xfs_bunmap_trace(
 #endif
 
 /*
- * Convert inode from non-attributed to attributed.
- * Must not be in a transaction, ip must not be locked.
+ * Convert inode from non-attributed to attributed (transaction 
+ * version: Use the transaction given via tpp)
  */
 int						/* error code */
-xfs_bmap_add_attrfork(
-	xfs_inode_t		*ip,		/* incore inode pointer */
+xfs_bmap_add_attrfork_trans(
+	struct xfs_trans	**tpp,		/* transaction pointer */
+	struct xfs_inode	*ip,		/* incore inode pointer */
 	int			size,		/* space new attribute needs */
 	int			rsvd)		/* xact may use reserved blks */
 {
 	xfs_fsblock_t		firstblock;	/* 1st block/ag allocated */
-	xfs_bmap_free_t		flist;		/* freed extent records */
-	xfs_mount_t		*mp;		/* mount structure */
-	xfs_trans_t		*tp;		/* transaction pointer */
-	int			blks;		/* space reservation */
+	struct xfs_bmap_free	flist;		/* freed extent records */
+	struct xfs_mount	*mp;		/* mount structure */
+	struct xfs_trans	*tp;		/* transaction pointer */
 	int			version = 1;	/* superblock attr version */
 	int			committed;	/* xaction was committed */
 	int			logflags;	/* logging flags */
@@ -3966,24 +3966,8 @@ xfs_bmap_add_attrfork(
 
 	mp = ip->i_mount;
 	ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
-	tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
-	blks = XFS_ADDAFORK_SPACE_RES(mp);
-	if (rsvd)
-		tp->t_flags |= XFS_TRANS_RESERVE;
-	if ((error = xfs_trans_reserve(tp, blks, XFS_ADDAFORK_LOG_RES(mp), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_ADDAFORK_LOG_COUNT)))
-		goto error0;
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, tp, ip, blks, 0, rsvd ?
-			XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
-			XFS_QMOPT_RES_REGBLKS);
-	if (error) {
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
-		return error;
-	}
-	if (XFS_IFORK_Q(ip))
-		goto error1;
+	ASSERT(*tpp);
+	tp = *tpp;
 	if (ip->i_d.di_aformat != XFS_DINODE_FMT_EXTENTS) {
 		/*
 		 * For inodes coming from pre-6.2 filesystems.
@@ -3991,10 +3975,7 @@ xfs_bmap_add_attrfork(
 		ASSERT(ip->i_d.di_aformat == 0);
 		ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
 	}
-	ASSERT(ip->i_d.di_anextents == 0);
-	VN_HOLD(XFS_ITOV(ip));
-	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
 	switch (ip->i_d.di_format) {
 	case XFS_DINODE_FMT_DEV:
 		ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
@@ -4014,7 +3995,7 @@ xfs_bmap_add_attrfork(
 	default:
 		ASSERT(0);
 		error = XFS_ERROR(EINVAL);
-		goto error1;
+		goto error0;
 	}
 	ip->i_df.if_ext_max =
 		XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
@@ -4045,7 +4026,7 @@ xfs_bmap_add_attrfork(
 	if (logflags)
 		xfs_trans_log_inode(tp, ip, logflags);
 	if (error)
-		goto error2;
+		goto error1;
 	if (!xfs_sb_version_hasattr(&mp->m_sb) ||
 	   (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2)) {
 		__int64_t sbfields = 0;
@@ -4065,14 +4046,66 @@ xfs_bmap_add_attrfork(
 		} else
 			spin_unlock(&mp->m_sb_lock);
 	}
-	if ((error = xfs_bmap_finish(&tp, &flist, &committed)))
-		goto error2;
-	error = xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
+	error = xfs_bmap_finish(tpp, &flist, &committed);
+	if (error)
+		goto error1;
 	ASSERT(ip->i_df.if_ext_max ==
 	       XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t));
-	return error;
-error2:
+	return 0;
+error1:
 	xfs_bmap_cancel(&flist);
+error0:
+	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
+	ASSERT(ip->i_df.if_ext_max ==
+	       XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t));
+	return error;
+}
+
+/*
+ * Convert inode from non-attributed to attributed.
+ * Must not be in a transaction, ip must not be locked.
+ */
+int
+xfs_bmap_add_attrfork(
+	struct xfs_inode	*ip,		/* incore inode pointer */
+	int			size,		/* space new attribute needs */
+	int			rsvd)		/* xact may use reserved blks */
+{
+	struct xfs_trans	*tp;		/* transaction pointer */
+	struct xfs_mount	*mp;		/* mount structure */
+	int			blks;		/* space reservation */
+	int			error;		/* error return value */
+
+	mp = ip->i_mount;
+	tp = xfs_trans_alloc(mp, XFS_TRANS_ADDAFORK);
+	blks = XFS_ADDAFORK_SPACE_RES(mp);
+
+	if (rsvd)
+		tp->t_flags |= XFS_TRANS_RESERVE;
+	error = xfs_trans_reserve(tp, blks, XFS_ADDAFORK_LOG_RES(mp), 0,
+				XFS_TRANS_PERM_LOG_RES, XFS_ADDAFORK_LOG_COUNT);
+	if (error)
+		goto error0;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, tp, ip, blks, 0, rsvd ?
+			      XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+			      XFS_QMOPT_RES_REGBLKS);
+	if (error)
+		goto error1;
+
+	if (XFS_IFORK_Q(ip))
+		goto error1;
+
+	ASSERT(ip->i_d.di_anextents == 0);
+	VN_HOLD(XFS_ITOV(ip));
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
+	error = xfs_bmap_add_attrfork_trans(&tp, ip, size, rsvd);
+	if (error)
+		return error;
+	return xfs_trans_commit(tp, XFS_TRANS_PERM_LOG_RES);
 error1:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 error0:
diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
index 6ff70cd..7a6b3f3 100644
--- a/fs/xfs/xfs_bmap.h
+++ b/fs/xfs/xfs_bmap.h
@@ -157,6 +157,17 @@ xfs_bmap_trace_exlist(
 #endif
 
 /*
+ * Convert inode from non-attributed to attributed (transaction 
+ * version: Use the transaction given via tpp)
+ */
+int					/* error code */
+xfs_bmap_add_attrfork_trans(
+	struct xfs_trans	**tpp,   /* transaction */
+	struct xfs_inode	*ip,	/* incore inode pointer */
+	int			size,	/* space needed for new attribute */
+	int			rsvd);	/* flag for reserved block allocation */
+
+/*
  * Convert inode from non-attributed to attributed.
  * Must not be in a transaction, ip must not be locked.
  */
-- 
1.5.6.2

[PATCH] Give a transaction to xfs_attr_set_int

[Niv Sardi]

We introduce xfs_attr_set_int_trans that takes a transaction pointer
as an argument (or creates one if NULL) and only finishes the
transaction if it has created it. We use xfs_trans_roll to do the
trans_dup dance.

xfs_attr_set_int is changed to a wrapper that will only call
xfs_attr_set_int_trans with a NULL transaction.

make it use the new xfs_bmap_add_attrfork_trans(), and roll the
transaction after the call.

This is needed for Create+EA/Parent Pointers

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c |  139 ++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 94 insertions(+), 45 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 4888a35..8e9598d 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -236,9 +236,19 @@ xfs_attr_calc_size(
 	return nblks;
 }
 
+/*
+ * So if the trans is given we don't have the right to dispose of it,
+ * we can't commit it either as we don't know if the caller is
+ * done with it.
+ */
 STATIC int
-xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
-		char *value, int valuelen, int flags)
+xfs_attr_set_int_trans(
+	xfs_trans_t	**tpp,
+	xfs_inode_t	*dp,
+	struct xfs_name *name,
+	char		*value,
+	int		valuelen,
+	int		flags)
 {
 	xfs_da_args_t	args;
 	xfs_fsblock_t	firstblock;
@@ -260,10 +270,14 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	 */
 	if (XFS_IFORK_Q(dp) == 0) {
 		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
-			      XFS_ATTR_SF_ENTSIZE_BYNAME(name->len, valuelen);
+			XFS_ATTR_SF_ENTSIZE_BYNAME(name->len, valuelen);
 
-		if ((error = xfs_bmap_add_attrfork(dp, sf_size, rsvd)))
-			return(error);
+		error = xfs_bmap_add_attrfork_trans(tpp, dp, sf_size, rsvd);
+		if (error)
+			return error;
+		error = xfs_trans_roll(tpp, dp);
+		if (error)
+			return error;
 	}
 
 	/*
@@ -286,45 +300,52 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	/* Size is now blocks for attribute data */
 	args.total = xfs_attr_calc_size(dp, name->len, valuelen, &local);
 
-	/*
-	 * Start our first transaction of the day.
-	 *
-	 * All future transactions during this code must be "chained" off
-	 * this one via the trans_dup() call.  All transactions will contain
-	 * the inode, and the inode will always be marked with trans_ihold().
-	 * Since the inode will be locked in all transactions, we must log
-	 * the inode in every transaction to let it float upward through
-	 * the log.
-	 */
-	args.trans = xfs_trans_alloc(mp, XFS_TRANS_ATTR_SET);
+	if (tpp) {
+		ASSERT(*tpp);
+		args.trans = *tpp;
+	} else {
+		/*
+		 * Start our first transaction of the day.
+		 *
+		 * All future transactions during this code must be "chained" off
+		 * this one via the trans_dup() call.  All transactions will contain
+		 * the inode, and the inode will always be marked with trans_ihold().
+		 * Since the inode will be locked in all transactions, we must log
+		 * the inode in every transaction to let it float upward through
+		 * the log.
+		 */
+		args.trans = xfs_trans_alloc(mp, XFS_TRANS_ATTR_SET);
 
-	/*
-	 * Root fork attributes can use reserved data blocks for this
-	 * operation if necessary
-	 */
+		/*
+		 * Root fork attributes can use reserved data blocks for this
+		 * operation if necessary
+		 */
 
-	if (rsvd)
-		args.trans->t_flags |= XFS_TRANS_RESERVE;
+		if (rsvd)
+			args.trans->t_flags |= XFS_TRANS_RESERVE;
 
-	if ((error = xfs_trans_reserve(args.trans, args.total,
-			XFS_ATTRSET_LOG_RES(mp, args.total), 0,
-			XFS_TRANS_PERM_LOG_RES, XFS_ATTRSET_LOG_COUNT))) {
-		xfs_trans_cancel(args.trans, 0);
-		return(error);
-	}
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
+		error = xfs_trans_reserve(args.trans, args.total,
+				 XFS_ATTRSET_LOG_RES(mp, args.total), 0,
+				 XFS_TRANS_PERM_LOG_RES, XFS_ATTRSET_LOG_COUNT);
+		if (error) {
+			xfs_trans_cancel(args.trans, 0);
+			return(error);
+		}
+		xfs_ilock(dp, XFS_ILOCK_EXCL);
 
-	error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp, args.total, 0,
-				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
+		error = XFS_TRANS_RESERVE_QUOTA_NBLKS(mp, args.trans, dp,
+					args.total, 0,
+				rsvd?XFS_QMOPT_RES_REGBLKS|XFS_QMOPT_FORCE_RES:
 				       XFS_QMOPT_RES_REGBLKS);
-	if (error) {
-		xfs_iunlock(dp, XFS_ILOCK_EXCL);
-		xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
-		return (error);
-	}
+		if (error) {
+			xfs_iunlock(dp, XFS_ILOCK_EXCL);
+			xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
+			return (error);
+		}
 
-	xfs_trans_ijoin(args.trans, dp, XFS_ILOCK_EXCL);
-	xfs_trans_ihold(args.trans, dp);
+		xfs_trans_ijoin(args.trans, dp, XFS_ILOCK_EXCL);
+		xfs_trans_ihold(args.trans, dp);
+	}
 
 	/*
 	 * If the attribute list is non-existent or a shortform list,
@@ -360,8 +381,14 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 			if (mp->m_flags & XFS_MOUNT_WSYNC) {
 				xfs_trans_set_sync(args.trans);
 			}
-			err2 = xfs_trans_commit(args.trans,
-						 XFS_TRANS_RELEASE_LOG_RES);
+
+			/* Only finish trans if it's ours */
+			if (tpp) {
+				err2 = xfs_trans_roll(&args.trans, dp);
+			} else {
+				err2 = xfs_trans_commit(args.trans,
+							XFS_TRANS_RELEASE_LOG_RES);
+			}
 			xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
 			/*
@@ -370,6 +397,8 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 			if (!error && (flags & ATTR_KERNOTIME) == 0) {
 				xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
 			}
+			if (tpp)
+				*tpp = args.trans;
 			return(error == 0 ? err2 : error);
 		}
 
@@ -415,13 +444,13 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	} else {
 		error = xfs_attr_node_addname(&args);
 	}
-	if (error) {
+	if (error)
 		goto out;
-	}
 
 	/*
 	 * If this is a synchronous mount, make sure that the
-	 * transaction goes to disk before returning to the user.
+	 * transaction goes to disk before returning to the user. If
+	 * this is not our transaction, the allocator should do this.
 	 */
 	if (mp->m_flags & XFS_MOUNT_WSYNC) {
 		xfs_trans_set_sync(args.trans);
@@ -430,8 +459,12 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 	/*
 	 * Commit the last in the sequence of transactions.
 	 */
-	xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
-	error = xfs_trans_commit(args.trans, XFS_TRANS_RELEASE_LOG_RES);
+	if (tpp) {
+		xfs_trans_roll(&args.trans, dp);
+	} else {
+		xfs_trans_log_inode(args.trans, dp, XFS_ILOG_CORE);
+		error = xfs_trans_commit(args.trans, XFS_TRANS_RELEASE_LOG_RES);
+	}
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
 	/*
@@ -441,6 +474,8 @@ xfs_attr_set_int(xfs_inode_t *dp, struct xfs_name *name,
 		xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
 	}
 
+	if (tpp)
+		*tpp = args.trans;
 	return(error);
 
 out:
@@ -448,9 +483,23 @@ out:
 		xfs_trans_cancel(args.trans,
 			XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
+	if (tpp)
+		*tpp = args.trans;
 	return(error);
 }
 
+STATIC int
+xfs_attr_set_int(
+	xfs_inode_t	*dp,
+	struct xfs_name *name,
+	char		*value,
+	int		valuelen,
+	int		flags)
+{
+	return xfs_attr_set_int_trans(NULL, dp, name,
+				      value, valuelen, flags);
+}
+
 int
 xfs_attr_set(
 	xfs_inode_t	*dp,
-- 
1.5.6.2

[PATCH] Export xfs_attr_set_int_trans

[Niv Sardi]

make xfs_attr_set_int_trans non static so we can use it outside of xfs_attr.c
(Needed to use it in vnodeops.c, for create+EA)

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_attr.c |    2 +-
 fs/xfs/xfs_attr.h |    3 +++
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 8e9598d..24ba91d 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -241,7 +241,7 @@ xfs_attr_calc_size(
  * we can't commit it either as we don't know if the caller is
  * done with it.
  */
-STATIC int
+int
 xfs_attr_set_int_trans(
 	xfs_trans_t	**tpp,
 	xfs_inode_t	*dp,
diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
index f99395c..32e80f9 100644
--- a/fs/xfs/xfs_attr.h
+++ b/fs/xfs/xfs_attr.h
@@ -161,6 +161,9 @@ struct xfs_da_args;
 int xfs_attr_calc_size(struct xfs_inode *, int, int, int *);
 int xfs_attr_inactive(struct xfs_inode *dp);
 
+int xfs_attr_set_int_trans(struct xfs_trans **, struct xfs_inode *, 
+			   struct xfs_name *, char *, int, int);
+
 int xfs_attr_shortform_getvalue(struct xfs_da_args *);
 int xfs_attr_fetch(struct xfs_inode *, struct xfs_name *, char *, int *, int);
 int xfs_attr_rmtval_get(struct xfs_da_args *args);
-- 
1.5.6.2

[PATCH] hack to test create + ea

[Niv Sardi]

---
 fs/xfs/xfs_vnodeops.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index e475e37..48df96b 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -1789,6 +1789,9 @@ xfs_create(
 	 */
 	XFS_QM_DQVOPCREATE(mp, tp, ip, udqp, gdqp);
 
+	/* hack to test create + ea */
+	xfs_attr_set_int_trans(&tp, ip, name, "TEST", strlen("TEST"), ATTR_ROOT);
+
 	/*
 	 * xfs_trans_commit normally decrements the vnode ref count
 	 * when it unlocks the inode. Since we want to return the
-- 
1.5.6.2

Re: [PATCH] Export xfs_attr_set_int_trans

[Christoph Hellwig]

On Fri, Jul 11, 2008 at 03:38:14PM +1000, Niv Sardi wrote:
> make xfs_attr_set_int_trans non static so we can use it outside of xfs_attr.c
> (Needed to use it in vnodeops.c, for create+EA)

Any reason this is not merged into the previous patch?

Re: [PATCH] Export xfs_attr_set_int_trans

[Niv Sardi]

Christoph Hellwig <hch@infradead.org> writes:

> On Fri, Jul 11, 2008 at 03:38:14PM +1000, Niv Sardi wrote:
>> make xfs_attr_set_int_trans non static so we can use it outside of xfs_attr.c
>> (Needed to use it in vnodeops.c, for create+EA)
>
> Any reason this is not merged into the previous patch?

you =) you didn't like my attr_* function to be exported, so I splitted
the export out from now on. that's about it.

Cheers,
-- 
Niv Sardi

Re: [PATCH] Export xfs_attr_set_int_trans

[Christoph Hellwig]

On Tue, Jul 22, 2008 at 04:06:55PM +1000, Niv Sardi wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Fri, Jul 11, 2008 at 03:38:14PM +1000, Niv Sardi wrote:
> >> make xfs_attr_set_int_trans non static so we can use it outside of xfs_attr.c
> >> (Needed to use it in vnodeops.c, for create+EA)
> >
> > Any reason this is not merged into the previous patch?
> 
> you =) you didn't like my attr_* function to be exported, so I splitted
> the export out from now on. that's about it.

Heh, sorry.  The general rule is exporting it is fine if one of the
next patches uses it.  That wasn't obvious when you posted the first
patches, so I asked for it to be static for now.

Re: [PATCH] Give a transaction to xfs_attr_set_int

[Niv Sardi]

Ok, so following are 2 patches that will, export xfs_attr_set_int_trans,
and use it where we would use it in Parent Pointers (and V4ACL to what I
understand).

This exposes the bug that I refered to before, and that I'm not sure I
understand fully.

Cheers,
-- 
Niv Sardi

Re: [PATCH] Give a transaction to xfs_attr_set_int

[Christoph Hellwig]

On Fri, Jul 11, 2008 at 03:44:39PM +1000, Niv Sardi wrote:
> 
> Ok, so following are 2 patches that will, export xfs_attr_set_int_trans,
> and use it where we would use it in Parent Pointers (and V4ACL to what I
> understand).
> 
> This exposes the bug that I refered to before, and that I'm not sure I
> understand fully.

Actually plain Posix ACLs and selinux need it too to guarantee atomicy.
See xfs_init_security() and whatever _ACL_INHERIT expans to in
fs/xfs/linux-2.6/xfs_iops.c.  These might be good testcases to get
started with this work.

Re: [PATCH] Introduce xfs_bmap_add_attrfork_trans.

[Christoph Hellwig]

> +error0:
> +	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
> +	ASSERT(ip->i_df.if_ext_max ==
> +	       XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t));
> +	return error;

I think the transaction cancelling should be done by the caller.  The
callers will surely have cases where they have to cancel the transaction
already, and it's also more symmetric.

> +	VN_HOLD(XFS_ITOV(ip));

Please just use

	IHOLD(ip);

instead of the above construct.

Otherwise this looks fine and like another candidate to commit already.

Re: [UPDATED RFC] Create with EA initial work

[Christoph Hellwig]

On Thu, Jul 10, 2008 at 05:39:01PM +1000, Niv Sardi wrote:
> There is a bug in this, that I can see with the Parent Pointers code
> going on top of it (that will be posted soon), it is basically calling
> xfs_attr_set_int_trans() in xfs_create() just before the last
> commit. For some reason, the first call to xfs_roll_trans (after
> xfs_bmap_add_attrfork_trans()) will complain about the inode being
> unlocked after xfs_trans_commit(). I understand I need to call
> xfs_trans_ihold(ip) on it, but we already do in xfs_create() so I
> think I must be missing something else??? any ideas ?

There are multiple ways to deal with inodes linked to transactions.

In all cases it needs to be linked into the transaction by
xfs_trans_ijoin, or the opencoded equivalent for a new inode in
xfs_trans_iget.  Then you can use xfs_trans_ihold to make sure on
transaction commit the inode reference count is not dropped and the
inode is not unlocked, or simply grab a reference to the inode and let
the transaction commit handler unlock it and decrement the reference
count.  The latter is what's used by xfs_create and the former is what
the attr code does, and as far as I can see the only things what works
with xfs_attr_rolltrans.

Re: [UPDATED RFC] Create with EA initial work

[Niv Sardi]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

Christoph Hellwig <hch@infradead.org> writes:
> On Thu, Jul 10, 2008 at 05:39:01PM +1000, Niv Sardi wrote:
>> There is a bug in this, that I can see with the Parent Pointers code
>> going on top of it (that will be posted soon), it is basically calling
>> xfs_attr_set_int_trans() in xfs_create() just before the last
>> commit. For some reason, the first call to xfs_roll_trans (after
>> xfs_bmap_add_attrfork_trans()) will complain about the inode being
>> unlocked after xfs_trans_commit(). I understand I need to call
>> xfs_trans_ihold(ip) on it, but we already do in xfs_create() so I
>> think I must be missing something else??? any ideas ?
>
> There are multiple ways to deal with inodes linked to transactions.
>
> In all cases it needs to be linked into the transaction by
> xfs_trans_ijoin, or the opencoded equivalent for a new inode in
> xfs_trans_iget.

Shouldn't the inode already be in the transaction ? we just created it
and added it to the directory ?

xfs_create()
  xfs_dir_ialloc(tp, dp, ip)
    xfs_ialloc(tp, dp, ip)
      xfs_trans_iget(tp, ip)

and it has a i_transp after dir_ialloc.

And is why don't we use ijoin in iget as we copy the exact same code a
few lines later ? (like in [0])

>  Then you can use xfs_trans_ihold to make sure on transaction commit
> the inode reference count is not dropped and the inode is not
> unlocked, or simply grab a reference to the inode and let the
> transaction commit handler unlock it and decrement the reference
> count.  The latter is what's used by xfs_create and the former is what
> the attr code does, and as far as I can see the only things what works
> with xfs_attr_rolltrans.

hum, that seemed to work on my UML, shall we require callers to do all
the ihold, ijoin stuff or should we do that (or at least check it) from
the new attr*_trans code ?

[0] 

[-- Attachment #2: Type: text/plain, Size: 1406 bytes --]

>From 18a682c77b0f89faae6906ebc0d73af59ddff632 Mon Sep 17 00:00:00 2001
From: Niv Sardi <xaiki@debian.org>
Date: Wed, 23 Jul 2008 11:27:20 +1000
Subject: [PATCH] Use xfs_trans_ijoin in xfs_trans_iget

Avoid some code duplication

Signed-off-by: Niv Sardi <xaiki@sgi.com>
---
 fs/xfs/xfs_trans_inode.c |   30 +-----------------------------
 1 files changed, 1 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
index 2a1c0f0..1dbcbe9 100644
--- a/fs/xfs/xfs_trans_inode.c
+++ b/fs/xfs/xfs_trans_inode.c
@@ -138,35 +138,7 @@ xfs_trans_iget(
 	}
 	ASSERT(ip != NULL);
 
-	/*
-	 * Get a log_item_desc to point at the new item.
-	 */
-	if (ip->i_itemp == NULL)
-		xfs_inode_item_init(ip, mp);
-	iip = ip->i_itemp;
-	(void) xfs_trans_add_item(tp, (xfs_log_item_t *)(iip));
-
-	xfs_trans_inode_broot_debug(ip);
-
-	/*
-	 * If the IO lock has been acquired, mark that in
-	 * the inode log item so we'll know to unlock it
-	 * when the transaction commits.
-	 */
-	ASSERT(iip->ili_flags == 0);
-	if (lock_flags & XFS_IOLOCK_EXCL) {
-		iip->ili_flags |= XFS_ILI_IOLOCKED_EXCL;
-	} else if (lock_flags & XFS_IOLOCK_SHARED) {
-		iip->ili_flags |= XFS_ILI_IOLOCKED_SHARED;
-	}
-
-	/*
-	 * Initialize i_transp so we can find it with xfs_inode_incore()
-	 * above.
-	 */
-	ip->i_transp = tp;
-
-	*ipp = ip;
+	xfs_trans_ijoin(tp, ip, lock_flags);
 	return 0;
 }
 
-- 
1.5.6


[-- Attachment #3: Type: text/plain, Size: 23 bytes --]


Cheers,
-- 
Niv Sardi

Re: [UPDATED RFC] Create with EA initial work

[Christoph Hellwig]

On Wed, Jul 23, 2008 at 03:35:23PM +1000, Niv Sardi wrote:
> > There are multiple ways to deal with inodes linked to transactions.
> >
> > In all cases it needs to be linked into the transaction by
> > xfs_trans_ijoin, or the opencoded equivalent for a new inode in
> > xfs_trans_iget.
> 
> Shouldn't the inode already be in the transaction ? we just created it
> and added it to the directory ?
> 
> xfs_create()
>   xfs_dir_ialloc(tp, dp, ip)
>     xfs_ialloc(tp, dp, ip)
>       xfs_trans_iget(tp, ip)
> 
> and it has a i_transp after dir_ialloc.
> 
> And is why don't we use ijoin in iget as we copy the exact same code a
> few lines later ? (like in [0])

Yes, it is in the transaction.  I just meant to say xfs_trans_iget
opencodes the transaction linking otherwise done in xfs_trans_ijoin.

> >  Then you can use xfs_trans_ihold to make sure on transaction commit
> > the inode reference count is not dropped and the inode is not
> > unlocked, or simply grab a reference to the inode and let the
> > transaction commit handler unlock it and decrement the reference
> > count.  The latter is what's used by xfs_create and the former is what
> > the attr code does, and as far as I can see the only things what works
> > with xfs_attr_rolltrans.
> 
> hum, that seemed to work on my UML, shall we require callers to do all
> the ihold, ijoin stuff or should we do that (or at least check it) from
> the new attr*_trans code ?

I'm not sure.  What we need to is to make sure we don't have multiple
linked transactions that use the iget + unlock style.  One way to do
that would to alwas use the xfs_trans_ihold style first and then
add a helper that just clears XFS_ILI_HOLD for the final transaction
in create.

> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index 2a1c0f0..1dbcbe9 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -138,35 +138,7 @@ xfs_trans_iget(
>  	}
>  	ASSERT(ip != NULL);
>  
> -	/*
> -	 * Get a log_item_desc to point at the new item.
> -	 */
> -	if (ip->i_itemp == NULL)
> -		xfs_inode_item_init(ip, mp);
> -	iip = ip->i_itemp;
> -	(void) xfs_trans_add_item(tp, (xfs_log_item_t *)(iip));
> -
> -	xfs_trans_inode_broot_debug(ip);
> -
> -	/*
> -	 * If the IO lock has been acquired, mark that in
> -	 * the inode log item so we'll know to unlock it
> -	 * when the transaction commits.
> -	 */
> -	ASSERT(iip->ili_flags == 0);
> -	if (lock_flags & XFS_IOLOCK_EXCL) {
> -		iip->ili_flags |= XFS_ILI_IOLOCKED_EXCL;
> -	} else if (lock_flags & XFS_IOLOCK_SHARED) {
> -		iip->ili_flags |= XFS_ILI_IOLOCKED_SHARED;
> -	}
> -
> -	/*
> -	 * Initialize i_transp so we can find it with xfs_inode_incore()
> -	 * above.
> -	 */
> -	ip->i_transp = tp;
> -
> -	*ipp = ip;
> +	xfs_trans_ijoin(tp, ip, lock_flags);

Did you test this?  For one the *ipp assignment seems to be missing now,
and second I'd think we'd have to audit the ASSERTs in xfs_trans_ijoin
if they are correct for the iget case.  But I think this is a good idea,
and if the asserts are not okay just create an _xfs_trans_ijoin without
them that's used by xfs_trans_ijoin.