On Mon, Jul 30, 2018 at 12:45:12PM -0400, Brian Foster wrote:
> Buffers that are held across deferred operations are explicitly
> joined to the dfops structure to ensure appropriate relogging.
> While buffers are currently joined explicitly, we can detect the
> conditions that require relogging at dfops finish time by inspecting
> the transaction item list for held buffers.
>
> Replace the xfs_defer_bjoin() infrastructure with such detection and
> automatic relogging of held buffers. This eliminates the need for
> the per-dfops buffer list, replaced by an on-stack variant in
> xfs_defer_trans_roll().
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 1 -
> fs/xfs/libxfs/xfs_defer.c | 55 ++++++++++++++++-----------------------
> fs/xfs/libxfs/xfs_defer.h | 1 -
> fs/xfs/xfs_dquot.c | 1 -
> fs/xfs/xfs_trans.h | 1 -
> 5 files changed, 23 insertions(+), 36 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 3deb5cdadf08..227887bee00d 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -320,7 +320,6 @@ xfs_attr_set(
> * buffer and run into problems with the write verifier.
> */
> xfs_trans_bhold(args.trans, leaf_bp);
> - xfs_defer_bjoin(args.trans->t_dfops, leaf_bp);
> xfs_defer_ijoin(args.trans->t_dfops, dp);
> error = xfs_defer_finish(&args.trans);
> if (error)
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 0306187b5f56..d0ee55048a7a 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -14,6 +14,7 @@
> #include "xfs_mount.h"
> #include "xfs_defer.h"
> #include "xfs_trans.h"
> +#include "xfs_buf_item.h"
> #include "xfs_trace.h"
>
> /*
> @@ -228,6 +229,10 @@ xfs_defer_trans_roll(
> struct xfs_trans **tp)
> {
> struct xfs_defer_ops *dop = (*tp)->t_dfops;
> + struct xfs_buf_log_item *bli;
> + struct xfs_log_item *lip;
> + struct xfs_buf *bplist[XFS_DEFER_OPS_NR_BUFS];
> + int bpcount = 0;
> int i;
> int error;
>
> @@ -235,9 +240,21 @@ xfs_defer_trans_roll(
> for (i = 0; i < XFS_DEFER_OPS_NR_INODES && dop->dop_inodes[i]; i++)
> xfs_trans_log_inode(*tp, dop->dop_inodes[i], XFS_ILOG_CORE);
>
> - /* Hold the (previously bjoin'd) buffer locked across the roll. */
> - for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
> - xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
> + list_for_each_entry(lip, &(*tp)->t_items, li_trans) {
> + switch (lip->li_type) {
> + case XFS_LI_BUF:
> + bli = container_of(lip, struct xfs_buf_log_item,
> + bli_item);
> + if (bli->bli_flags & XFS_BLI_HOLD) {
> + ASSERT(bpcount < XFS_DEFER_OPS_NR_BUFS);
This is a bit of a behavior change -- previously if we oveflowed the
array we'd bail out with -EFSCORRUPTED rather than go corrupting
whatever came after dop_bufs. Granted, we were pretty terrible about
checking the return values, but I think this ought to be:
if (bpcount >= XFS_DEFER_OPS_NR_BUFS) {
ASSERT(0);
return -EFSCORRUPTED;
}
rather than corrupting the on-stack array if we happen to bhold too many
buffers to the transaction.
--D
> + xfs_trans_dirty_buf(*tp, bli->bli_buf);
> + bplist[bpcount++] = bli->bli_buf;
> + }
> + break;
> + default:
> + break;
> + }
> + }
>
> trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
>
> @@ -255,9 +272,9 @@ xfs_defer_trans_roll(
> xfs_trans_ijoin(*tp, dop->dop_inodes[i], 0);
>
> /* Rejoin the buffers and dirty them so the log moves forward. */
> - for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++) {
> - xfs_trans_bjoin(*tp, dop->dop_bufs[i]);
> - xfs_trans_bhold(*tp, dop->dop_bufs[i]);
> + for (i = 0; i < bpcount; i++) {
> + xfs_trans_bjoin(*tp, bplist[i]);
> + xfs_trans_bhold(*tp, bplist[i]);
> }
>
> return error;
> @@ -295,30 +312,6 @@ xfs_defer_ijoin(
> return -EFSCORRUPTED;
> }
>
> -/*
> - * Add this buffer to the deferred op. Each joined buffer is relogged
> - * each time we roll the transaction.
> - */
> -int
> -xfs_defer_bjoin(
> - struct xfs_defer_ops *dop,
> - struct xfs_buf *bp)
> -{
> - int i;
> -
> - for (i = 0; i < XFS_DEFER_OPS_NR_BUFS; i++) {
> - if (dop->dop_bufs[i] == bp)
> - return 0;
> - else if (dop->dop_bufs[i] == NULL) {
> - dop->dop_bufs[i] = bp;
> - return 0;
> - }
> - }
> -
> - ASSERT(0);
> - return -EFSCORRUPTED;
> -}
> -
> /*
> * Reset an already used dfops after finish.
> */
> @@ -331,7 +324,6 @@ xfs_defer_reset(
> ASSERT(!xfs_defer_has_unfinished_work(dop));
>
> memset(dop->dop_inodes, 0, sizeof(dop->dop_inodes));
> - memset(dop->dop_bufs, 0, sizeof(dop->dop_bufs));
>
> /*
> * Low mode state transfers across transaction rolls to mirror dfops
> @@ -594,7 +586,6 @@ xfs_defer_move(
> list_splice_init(&src->dop_pending, &dst->dop_pending);
>
> memcpy(dst->dop_inodes, src->dop_inodes, sizeof(dst->dop_inodes));
> - memcpy(dst->dop_bufs, src->dop_bufs, sizeof(dst->dop_bufs));
>
> /*
> * Low free space mode was historically controlled by a dfops field.
> diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> index 8908a2716774..4a8bb838adf2 100644
> --- a/fs/xfs/libxfs/xfs_defer.h
> +++ b/fs/xfs/libxfs/xfs_defer.h
> @@ -43,7 +43,6 @@ void xfs_defer_cancel(struct xfs_trans *);
> void xfs_defer_init(struct xfs_trans *tp, struct xfs_defer_ops *dop);
> bool xfs_defer_has_unfinished_work(struct xfs_defer_ops *dop);
> int xfs_defer_ijoin(struct xfs_defer_ops *dop, struct xfs_inode *ip);
> -int xfs_defer_bjoin(struct xfs_defer_ops *dop, struct xfs_buf *bp);
> void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
>
> /* Description of a deferred type. */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index da5c55cec966..e1196854dbcd 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -362,7 +362,6 @@ xfs_dquot_disk_alloc(
> * manually or by committing the transaction.
> */
> xfs_trans_bhold(tp, bp);
> - error = xfs_defer_bjoin(tp->t_dfops, bp);
> if (error) {
> xfs_trans_bhold_release(tp, bp);
> xfs_trans_brelse(tp, bp);
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 7e493221160e..581456c79197 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -100,7 +100,6 @@ struct xfs_defer_ops {
>
> /* relog these with each roll */
> struct xfs_inode *dop_inodes[XFS_DEFER_OPS_NR_INODES];
> - struct xfs_buf *dop_bufs[XFS_DEFER_OPS_NR_BUFS];
> };
>
> /*
> --
> 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
