From 9833c1a425e00d1cf113ed66bea03c5d801ac2f7 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Wed, 24 Jul 2019 15:54:06 -0400 Subject: [PATCH] xfs: always rejoin held resources during defer roll Source kernel commit: 710d707d2fa9cf4c2aa9def129e71e99513466ea During testing of xfs/141 on a V4 filesystem, I observed some inconsistent behavior with regards to resources that are held (i.e. remain locked) across a defer roll. The transaction roll always gives the defer roll function a new transaction, even if committing the old transaction fails. However, the defer roll function only rejoins the held resources if the transaction commit succeedied. This means that callers of defer roll have to figure out whether the held resources are attached to the transaction being passed back. Worse yet, if the defer roll was part of a defer finish call, we have a third possibility: the defer finish could pass back a dirty transaction with dirty held resources and an error code. The only sane way to handle all of these scenarios is to require that the code that held the resource either cancel the transaction before unlocking and releasing the resources, or use functions that detach resources from a transaction properly (e.g. xfs_trans_brelse) if they need to drop the reference before committing or cancelling the transaction. In order to make this so, change the defer roll code to join held resources to the new transaction unconditionally and fix all the bhold callers to release the held buffers correctly. Signed-off-by: Darrick J. Wong Reviewed-by: Brian Foster Signed-off-by: Eric Sandeen --- include/xfs_trace.h | 1 + include/xfs_trans.h | 1 + libxfs/libxfs_api_defs.h | 1 + libxfs/trans.c | 24 ++++++++++++++++++++++++ libxfs/xfs_attr.c | 35 ++++++++++++----------------------- libxfs/xfs_attr.h | 2 +- libxfs/xfs_defer.c | 14 +++++++++----- 7 files changed, 49 insertions(+), 29 deletions(-) diff --git a/include/xfs_trace.h b/include/xfs_trace.h index 092dcbac8..da0d49953 100644 --- a/include/xfs_trace.h +++ b/include/xfs_trace.h @@ -167,6 +167,7 @@ #define trace_xfs_trans_binval(a) ((void) 0) #define trace_xfs_trans_bjoin(a) ((void) 0) #define trace_xfs_trans_bhold(a) ((void) 0) +#define trace_xfs_trans_bhold_release(a) ((void) 0) #define trace_xfs_trans_get_buf(a) ((void) 0) #define trace_xfs_trans_getsb_recur(a) ((void) 0) #define trace_xfs_trans_getsb(a) ((void) 0) diff --git a/include/xfs_trans.h b/include/xfs_trans.h index 60e1dbdb4..e0046b51a 100644 --- a/include/xfs_trans.h +++ b/include/xfs_trans.h @@ -99,6 +99,7 @@ void libxfs_trans_brelse(struct xfs_trans *, struct xfs_buf *); void libxfs_trans_binval(struct xfs_trans *, struct xfs_buf *); void libxfs_trans_bjoin(struct xfs_trans *, struct xfs_buf *); void libxfs_trans_bhold(struct xfs_trans *, struct xfs_buf *); +void libxfs_trans_bhold_release(struct xfs_trans *, struct xfs_buf *); void libxfs_trans_dirty_buf(struct xfs_trans *, struct xfs_buf *); void libxfs_trans_log_buf(struct xfs_trans *, struct xfs_buf *, uint, uint); diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h index 71a7ef53f..7bd373f70 100644 --- a/libxfs/libxfs_api_defs.h +++ b/libxfs/libxfs_api_defs.h @@ -20,6 +20,7 @@ #define xfs_trans_alloc_empty libxfs_trans_alloc_empty #define xfs_trans_add_item libxfs_trans_add_item #define xfs_trans_bhold libxfs_trans_bhold +#define xfs_trans_bhold_release libxfs_trans_bhold_release #define xfs_trans_binval libxfs_trans_binval #define xfs_trans_bjoin libxfs_trans_bjoin #define xfs_trans_brelse libxfs_trans_brelse diff --git a/libxfs/trans.c b/libxfs/trans.c index 5c56b4feb..bb54c98c3 100644 --- a/libxfs/trans.c +++ b/libxfs/trans.c @@ -519,6 +519,12 @@ libxfs_trans_bjoin( trace_xfs_trans_bjoin(bp->b_log_item); } +/* + * Mark the buffer as not needing to be unlocked when the buf item's + * iop_unlock() routine is called. The buffer must already be locked + * and associated with the given transaction. + */ +/* ARGSUSED */ void libxfs_trans_bhold( xfs_trans_t *tp, @@ -533,6 +539,24 @@ libxfs_trans_bhold( trace_xfs_trans_bhold(bip); } +/* + * Cancel the previous buffer hold request made on this buffer + * for this transaction. + */ +void +libxfs_trans_bhold_release( + xfs_trans_t *tp, + xfs_buf_t *bp) +{ + struct xfs_buf_log_item *bip = bp->b_log_item; + + ASSERT(bp->b_transp == tp); + ASSERT(bip != NULL); + + bip->bli_flags &= ~XFS_BLI_HOLD; + trace_xfs_trans_bhold_release(bip); +} + xfs_buf_t * libxfs_trans_get_buf_map( xfs_trans_t *tp, diff --git a/libxfs/xfs_attr.c b/libxfs/xfs_attr.c index 170e64cf0..41956e57a 100644 --- a/libxfs/xfs_attr.c +++ b/libxfs/xfs_attr.c @@ -220,10 +220,10 @@ xfs_attr_try_sf_addname( */ int xfs_attr_set_args( - struct xfs_da_args *args, - struct xfs_buf **leaf_bp) + struct xfs_da_args *args) { struct xfs_inode *dp = args->dp; + struct xfs_buf *leaf_bp = NULL; int error; /* @@ -251,7 +251,7 @@ xfs_attr_set_args( * It won't fit in the shortform, transform to a leaf block. * GROT: another possible req'mt for a double-split btree op. */ - error = xfs_attr_shortform_to_leaf(args, leaf_bp); + error = xfs_attr_shortform_to_leaf(args, &leaf_bp); if (error) return error; @@ -259,23 +259,16 @@ xfs_attr_set_args( * Prevent the leaf buffer from being unlocked so that a * concurrent AIL push cannot grab the half-baked leaf * buffer and run into problems with the write verifier. + * Once we're done rolling the transaction we can release + * the hold and add the attr to the leaf. */ - xfs_trans_bhold(args->trans, *leaf_bp); - + xfs_trans_bhold(args->trans, leaf_bp); error = xfs_defer_finish(&args->trans); - if (error) - return error; - - /* - * Commit the leaf transformation. We'll need another - * (linked) transaction to add the new attribute to the - * leaf. - */ - error = xfs_trans_roll_inode(&args->trans, dp); - if (error) + xfs_trans_bhold_release(args->trans, leaf_bp); + if (error) { + xfs_trans_brelse(args->trans, leaf_bp); return error; - xfs_trans_bjoin(args->trans, *leaf_bp); - *leaf_bp = NULL; + } } if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) @@ -318,7 +311,6 @@ xfs_attr_set( int flags) { struct xfs_mount *mp = dp->i_mount; - struct xfs_buf *leaf_bp = NULL; struct xfs_da_args args; struct xfs_trans_res tres; int rsvd = (flags & ATTR_ROOT) != 0; @@ -377,9 +369,9 @@ xfs_attr_set( goto out_trans_cancel; xfs_trans_ijoin(args.trans, dp, 0); - error = xfs_attr_set_args(&args, &leaf_bp); + error = xfs_attr_set_args(&args); if (error) - goto out_release_leaf; + goto out_trans_cancel; if (!args.trans) { /* shortform attribute has already been committed */ goto out_unlock; @@ -404,9 +396,6 @@ out_unlock: xfs_iunlock(dp, XFS_ILOCK_EXCL); return error; -out_release_leaf: - if (leaf_bp) - xfs_trans_brelse(args.trans, leaf_bp); out_trans_cancel: if (args.trans) xfs_trans_cancel(args.trans); diff --git a/libxfs/xfs_attr.h b/libxfs/xfs_attr.h index 2297d8467..3b0dce06e 100644 --- a/libxfs/xfs_attr.h +++ b/libxfs/xfs_attr.h @@ -140,7 +140,7 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, unsigned char *value, int *valuelenp, int flags); int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, unsigned char *value, int valuelen, int flags); -int xfs_attr_set_args(struct xfs_da_args *args, struct xfs_buf **leaf_bp); +int xfs_attr_set_args(struct xfs_da_args *args); int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); int xfs_attr_remove_args(struct xfs_da_args *args); int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, diff --git a/libxfs/xfs_defer.c b/libxfs/xfs_defer.c index d9d427131..1cb5eea47 100644 --- a/libxfs/xfs_defer.c +++ b/libxfs/xfs_defer.c @@ -272,13 +272,15 @@ xfs_defer_trans_roll( trace_xfs_defer_trans_roll(tp, _RET_IP_); - /* Roll the transaction. */ + /* + * Roll the transaction. Rolling always given a new transaction (even + * if committing the old one fails!) to hand back to the caller, so we + * join the held resources to the new transaction so that we always + * return with the held resources joined to @tpp, no matter what + * happened. + */ error = xfs_trans_roll(tpp); tp = *tpp; - if (error) { - trace_xfs_defer_trans_roll_error(tp, error); - return error; - } /* Rejoin the joined inodes. */ for (i = 0; i < ipcount; i++) @@ -290,6 +292,8 @@ xfs_defer_trans_roll( xfs_trans_bhold(tp, bplist[i]); } + if (error) + trace_xfs_defer_trans_roll_error(tp, error); return error; } -- 2.47.2