On 5/4/20 6:30 AM, Brian Foster wrote:
On Thu, Apr 30, 2020 at 03:50:05PM -0700, Allison Collins wrote:
Some calls to xfs_trans_roll_inode and xfs_defer_finish routines are not
needed. If they are the last operations executed in these functions, and
no further changes are made, then higher level routines will roll or
commit the tranactions. The xfs_trans_roll in _removename is also not
needed because invalidating blocks is an incore-only change.
Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
---
fs/xfs/libxfs/xfs_attr.c | 40 ----------------------------------------
1 file changed, 40 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index d83443c..af47566 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
...
@@ -784,9 +770,6 @@ xfs_attr_leaf_removename(
/* bp is gone due to xfs_da_shrink_inode */
if (error)
return error;
- error = xfs_defer_finish(&args->trans);
- if (error)
- return error;
}
return 0;
Looks like this could be simplified to return error at the end of the
function. Some of the others might have similar simplifications
available.
Sure, will do
}
@@ -1074,13 +1057,6 @@ xfs_attr_node_addname(
goto out;
}
- /*
- * Commit and start the next trans in the chain.
- */
- error = xfs_trans_roll_inode(&args->trans, dp);
- if (error)
- goto out;
-
There's an xfs_defer_finish() before this roll. Is that not also
extraneous? It looks like we return straight up to xfs_attr_set() (trans
commit) from here..
Ok, I see it. Yes I think so, I will see if I can take it out
} else if (args->rmtblkno > 0) {
/*
* Added a "remote" value, just clear the incomplete flag.
...
@@ -1194,10 +1158,6 @@ xfs_attr_node_removename(
if (error)
goto out;
- error = xfs_trans_roll_inode(&args->trans, args->dp);
- if (error)
- goto out;
-
I'm still a bit on the fence about this one. At first glance it looks
like it's not necessary, but technically we're changing behavior by
combining setting the inactive flag and the first remote block unmap,
right? If so, that seems fairly reasonable, but it's hard to say for
sure with the state of the xattr transaction reservations...
As I recall, I had initially not removed this roll in v5, and I had an
extra XFS_DAS_RM_INVALIDATE state to roll the transaction here. And
then I think we reasoned that because invalidating blocks didnt have in
core changes, we could just remove it, thus getting rid of the extra
state. Later I split it out into this separate patch along with a few
other transaction rolls that we identified as not being necessary.
link just for reference...
https://patchwork.kernel.org/patch/11287029/
Looking around some more, I suppose this could be analogous to the
!remote remove case where an entry is removed and a potential dabtree
join occurs under the same transaction. If that's the best argument we
have, however, I might suggest to split this one out into an independent
patch and let the commit log describe what's going on in more detail.
That way it's more obvious to reviewers and if it's wrong it's easier to
revert.
I'm not sure that was the argument at the time, but I think it makes
sense. :-) Will split off into separate patch.
Thanks for the reviews!
Allison
Brian
error = xfs_attr_rmtval_invalidate(args);
if (error)
return error;
--
2.7.4
