On 1/24/22 5:52 PM, Darrick J. Wong wrote:
On Sun, Jan 23, 2022 at 10:26:58PM -0700, Allison Henderson wrote:
If the first operation in a string of defer ops has no intents,
then there is no reason to commit it before running the first call
to xfs_defer_finish_one(). This allows the defer ops to be used
effectively for non-intent based operations without requiring an
unnecessary extra transaction commit when first called.
This fixes a regression in per-attribute modification transaction
count when delayed attributes are not being used.
Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
---
fs/xfs/libxfs/xfs_defer.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 6dac8d6b8c21..51574f0371b5 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -187,7 +187,7 @@ static const struct xfs_defer_op_type *defer_op_types[] = {
[XFS_DEFER_OPS_TYPE_AGFL_FREE] = &xfs_agfl_free_defer_type,
};
-static void
+static bool
xfs_defer_create_intent(
struct xfs_trans *tp,
struct xfs_defer_pending *dfp,
@@ -198,6 +198,7 @@ xfs_defer_create_intent(
if (!dfp->dfp_intent)
dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work,
dfp->dfp_count, sort);
+ return dfp->dfp_intent;
Hm. My first reaction is that this still ought to be an explicit
boolean comparison...
Ah, sorry, I think you had mentioned that in the last review and I had
forgotten to update it...
}
/*
@@ -205,16 +206,18 @@ xfs_defer_create_intent(
* associated extents, then add the entire intake list to the end of
* the pending list.
*/
-STATIC void
+STATIC bool
xfs_defer_create_intents(
struct xfs_trans *tp)
{
struct xfs_defer_pending *dfp;
+ bool ret = false;
list_for_each_entry(dfp, &tp->t_dfops, dfp_list) {
trace_xfs_defer_create_intent(tp->t_mountp, dfp);
- xfs_defer_create_intent(tp, dfp, true);
+ ret |= xfs_defer_create_intent(tp, dfp, true);
}
+ return ret;
}
/* Abort all the intents that were committed. */
@@ -488,7 +491,7 @@ int
xfs_defer_finish_noroll(
struct xfs_trans **tp)
{
- struct xfs_defer_pending *dfp;
+ struct xfs_defer_pending *dfp = NULL;
int error = 0;
LIST_HEAD(dop_pending);
@@ -507,17 +510,19 @@ xfs_defer_finish_noroll(
* of time that any one intent item can stick around in memory,
* pinning the log tail.
*/
- xfs_defer_create_intents(*tp);
+ bool has_intents = xfs_defer_create_intents(*tp);
...but now it occurs to me that I think we can test ((*tp)->t_flags &
XFS_TRANS_DIRTY) instead of setting up the explicit return type.
If the ->create_intent function actually logs an intent item to the
transaction, we need to commit that intent item (to persist it to disk)
before we start on the work that it represents. If an intent item has
been added, the transaction will be dirty.
At this point in the loop, we're trying to set ourselves up to call
->finish_one. The ->finish_one implementations expect a clean
transaction, which means that we /never/ want to get to...
list_splice_init(&(*tp)->t_dfops, &dop_pending);
- error = xfs_defer_trans_roll(tp);
- if (error)
- goto out_shutdown;
+ if (has_intents || dfp) {
+ error = xfs_defer_trans_roll(tp);
+ if (error)
+ goto out_shutdown;
- /* Possibly relog intent items to keep the log moving. */
- error = xfs_defer_relog(tp, &dop_pending);
- if (error)
- goto out_shutdown;
+ /* Possibly relog intent items to keep the log moving. */
+ error = xfs_defer_relog(tp, &dop_pending);
+ if (error)
+ goto out_shutdown;
+ }
...this point here with the transaction still dirty. Therefore, I think
all this patch really needs to change is that first _trans_roll:
xfs_defer_create_intents(*tp);
list_splice_init(&(*tp)->t_dfops, &dop_pending);
/*
* We must ensure the transaction is clean before we try to finish
* deferred work by committing logged intent items and anything
* else that dirtied the transaction.
*/
if ((*tpp)->t_flags & XFS_TRANS_DIRTY) {
error = xfs_defer_trans_roll(tp);
if (error)
goto out_shutdown;
}
/* Possibly relog intent items to keep the log moving. */
error = xfs_defer_relog(tp, &dop_pending);
if (error)
goto out_shutdown;
dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
dfp_list);
error = xfs_defer_finish_one(*tp, dfp);
if (error && error != -EAGAIN)
goto out_shutdown;
Thoughts?
--D
But this makes a lot of sense, and I agree that it's a lot simpler. I
am fine with this as long as everyone else is? I think Dave had
initially authored this patch and I added it to the set. If this works
for everyone else, I will add these updates.
Allison
dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
dfp_list);
--
2.25.1
