On Mon, Jul 23, 2018 at 09:04:03AM -0400, Brian Foster wrote:
> xfs_defer_finish() has a couple quirks that are not safe with
> respect to the upcoming internal dfops functionality. First,
> xfs_defer_finish() attaches the passed in dfops structure to
> ->t_dfops and caches and restores the original value. Second, it
> continues to use the initial dfops reference before and after the
> transaction roll.
>
> These behaviors assume that dop is an independent memory allocation
> from the transaction itself, which may not always be true once
> transactions begin to use an embedded dfops structure. In the latter
> model, dfops processing creates a new xfs_defer_ops structure with
> each transaction and the associated state is migrated across to the
> new transaction.
>
> Fix up xfs_defer_finish() to handle the possibility of the current
> dfops changing after a transaction roll. Since ->t_dfops is used
> unconditionally in this path, it is no longer necessary to
> attach/restore ->t_dfops and pass it explicitly down to
> xfs_defer_trans_roll(). Update dop in the latter function and the
> caller to ensure that it always refers to the current dfops
> structure.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Looks good.
Reviewed-by: Bill O'Donnell <billodo@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_defer.c | 32 ++++++++++++++------------------
> 1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index c4b0eaeb5190..ee734a8b3fa9 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -225,9 +225,9 @@ xfs_defer_trans_abort(
> /* Roll a transaction so we can do some deferred op processing. */
> STATIC int
> xfs_defer_trans_roll(
> - struct xfs_trans **tp,
> - struct xfs_defer_ops *dop)
> + struct xfs_trans **tp)
> {
> + struct xfs_defer_ops *dop = (*tp)->t_dfops;
> int i;
> int error;
>
> @@ -243,6 +243,7 @@ xfs_defer_trans_roll(
>
> /* Roll the transaction. */
> error = xfs_trans_roll(tp);
> + dop = (*tp)->t_dfops;
> if (error) {
> trace_xfs_defer_trans_roll_error((*tp)->t_mountp, dop, error);
> xfs_defer_trans_abort(*tp, dop, error);
> @@ -338,31 +339,25 @@ xfs_defer_finish(
> void *state;
> int error = 0;
> void (*cleanup_fn)(struct xfs_trans *, void *, int);
> - struct xfs_defer_ops *orig_dop;
>
> ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
> + ASSERT((*tp)->t_dfops == dop);
>
> trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
>
> - /*
> - * Attach dfops to the transaction during deferred ops processing. This
> - * explicitly causes calls into the allocator to defer AGFL block frees.
> - * Note that this code can go away once all dfops users attach to the
> - * associated tp.
> - */
> - ASSERT(!(*tp)->t_dfops || ((*tp)->t_dfops == dop));
> - orig_dop = (*tp)->t_dfops;
> - (*tp)->t_dfops = dop;
> -
> /* Until we run out of pending work to finish... */
> while (xfs_defer_has_unfinished_work(dop)) {
> /* Log intents for work items sitting in the intake. */
> xfs_defer_intake_work(*tp, dop);
>
> - /* Roll the transaction. */
> - error = xfs_defer_trans_roll(tp, dop);
> + /*
> + * Roll the transaction and update dop in case dfops was
> + * embedded in the transaction.
> + */
> + error = xfs_defer_trans_roll(tp);
> if (error)
> goto out;
> + dop = (*tp)->t_dfops;
>
> /* Log an intent-done item for the first pending item. */
> dfp = list_first_entry(&dop->dop_pending,
> @@ -428,10 +423,11 @@ xfs_defer_finish(
> * Roll the transaction once more to avoid returning to the caller
> * with a dirty transaction.
> */
> - if ((*tp)->t_flags & XFS_TRANS_DIRTY)
> - error = xfs_defer_trans_roll(tp, dop);
> + if ((*tp)->t_flags & XFS_TRANS_DIRTY) {
> + error = xfs_defer_trans_roll(tp);
> + dop = (*tp)->t_dfops;
> + }
> out:
> - (*tp)->t_dfops = orig_dop;
> if (error)
> trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
> else
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
