On Thu, Jun 30, 2011 at 12:44:28PM +1000, Dave Chinner wrote:

quoted

 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
-	ASSERT((new_size == 0) || (new_size <= ip->i_size));
-	ASSERT(*tp != NULL);
-	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
-	ASSERT(ip->i_transp == *tp);
+	ASSERT(new_size == 0 || new_size <= ip->i_size);

If new_size == 0, then it will always be <= ip->i_size, so that's
kind of a redundant check. I think this really should be two
different asserts, one that validates the data fork new_size range,
and one that validates the attr fork truncate to zero length only
condition:

	ASSERT(new_size <= ip->i_size);
	ASSERT(whichfork != XFS_ATTR_FORK || new_size == 0);

For now I was just keeping the existing assert, but changing this one
sounds ok.  OTOH I kept the whole routine fork agnostic, so I think
I'll rather just make the assert read:

	ASSERT(new_size <= ip->i_size);

and assume the one and only attr fork caller does the right thing.

quoted

@@ -1464,15 +1311,16 @@ xfs_itruncate_finish(
 		}
 
 		ntp = xfs_trans_dup(ntp);
-		error = xfs_trans_commit(*tp, 0);
-		*tp = ntp;
+		error = xfs_trans_commit(*tpp, 0);
+		*tpp = ntp;

I've always found this a mess to follow which transaction is which
because of the rewriting of ntp. This is easier to follow:

		ntp = xfs_trans_dup(*tpp);
		error = xfs_trans_commit(*tpp, 0);
		*tpp = ntp;

Now it's clear that we are duplicating *tpp, then committing it, and
then setting it to the duplicated transaction. Now I don't have to
go look at all the surrounding code to remind myself what ntp
contains to validate that the fragment of code is doing the right
thing.....

I've cleaned this up even further and added a local tp variable that
has the current transaction as a normal pointer.  *tpp is only assigned
back to in a single place after goto out, and ntp is only used for
the switching around to the duplicated transaction.

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