On Wed, Jun 29, 2011 at 10:01:18AM -0400, Christoph Hellwig wrote: > Split the guts of xfs_itruncate_finish that loop over the existing extents > and calls xfs_bunmapi on them into a new helper, xfs_itruncate_externs. > Make xfs_attr_inactive call it directly instead of xfs_itruncate_finish, > which allows to simplify the latter a lot, by only letting it deal with > the data fork. As a result xfs_itruncate_finish is renamed to > xfs_itruncate_data to make its use case more obvious. > > Also remove the sync parameter from xfs_itruncate_data, which has been > unessecary since the introduction of the busy extent list in 2002, and > completely dead code since 2003 when the XFS_BMAPI_ASYNC parameter was > made a no-op. > > I can't actually see why the xfs_attr_inactive needs to set the transaction > sync, but let's keep this patch simple and without changes in behaviour. > > Signed-off-by: Christoph Hellwig <hch@xxxxxx> Overall, looks good. A few minor comments in line, but consider it Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> > + struct xfs_mount *mp = ip->i_mount; > + struct xfs_trans *ntp = *tpp; > + xfs_bmap_free_t free_list; > + xfs_fsblock_t first_block; > + xfs_fileoff_t first_unmap_block; > + xfs_fileoff_t last_block; > + xfs_filblks_t unmap_len; > + int committed; > + int error = 0; > + int done = 0; > > 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); > @@ -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..... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
