Commit 710d707d authored by Darrick J. Wong's avatar Darrick J. Wong

xfs: always rejoin held resources during defer roll

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: default avatarDarrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: default avatarBrian Foster <bfoster@redhat.com>
parent 1749d1ea
...@@ -224,10 +224,10 @@ xfs_attr_try_sf_addname( ...@@ -224,10 +224,10 @@ xfs_attr_try_sf_addname(
*/ */
int int
xfs_attr_set_args( xfs_attr_set_args(
struct xfs_da_args *args, struct xfs_da_args *args)
struct xfs_buf **leaf_bp)
{ {
struct xfs_inode *dp = args->dp; struct xfs_inode *dp = args->dp;
struct xfs_buf *leaf_bp = NULL;
int error; int error;
/* /*
...@@ -255,7 +255,7 @@ xfs_attr_set_args( ...@@ -255,7 +255,7 @@ xfs_attr_set_args(
* It won't fit in the shortform, transform to a leaf block. * It won't fit in the shortform, transform to a leaf block.
* GROT: another possible req'mt for a double-split btree op. * 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) if (error)
return error; return error;
...@@ -263,23 +263,16 @@ xfs_attr_set_args( ...@@ -263,23 +263,16 @@ xfs_attr_set_args(
* Prevent the leaf buffer from being unlocked so that a * Prevent the leaf buffer from being unlocked so that a
* concurrent AIL push cannot grab the half-baked leaf * concurrent AIL push cannot grab the half-baked leaf
* buffer and run into problems with the write verifier. * 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); error = xfs_defer_finish(&args->trans);
if (error) xfs_trans_bhold_release(args->trans, leaf_bp);
return error; if (error) {
xfs_trans_brelse(args->trans, leaf_bp);
/*
* 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)
return error; return error;
xfs_trans_bjoin(args->trans, *leaf_bp); }
*leaf_bp = NULL;
} }
if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
...@@ -322,7 +315,6 @@ xfs_attr_set( ...@@ -322,7 +315,6 @@ xfs_attr_set(
int flags) int flags)
{ {
struct xfs_mount *mp = dp->i_mount; struct xfs_mount *mp = dp->i_mount;
struct xfs_buf *leaf_bp = NULL;
struct xfs_da_args args; struct xfs_da_args args;
struct xfs_trans_res tres; struct xfs_trans_res tres;
int rsvd = (flags & ATTR_ROOT) != 0; int rsvd = (flags & ATTR_ROOT) != 0;
...@@ -381,9 +373,9 @@ xfs_attr_set( ...@@ -381,9 +373,9 @@ xfs_attr_set(
goto out_trans_cancel; goto out_trans_cancel;
xfs_trans_ijoin(args.trans, dp, 0); xfs_trans_ijoin(args.trans, dp, 0);
error = xfs_attr_set_args(&args, &leaf_bp); error = xfs_attr_set_args(&args);
if (error) if (error)
goto out_release_leaf; goto out_trans_cancel;
if (!args.trans) { if (!args.trans) {
/* shortform attribute has already been committed */ /* shortform attribute has already been committed */
goto out_unlock; goto out_unlock;
...@@ -408,9 +400,6 @@ xfs_attr_set( ...@@ -408,9 +400,6 @@ xfs_attr_set(
xfs_iunlock(dp, XFS_ILOCK_EXCL); xfs_iunlock(dp, XFS_ILOCK_EXCL);
return error; return error;
out_release_leaf:
if (leaf_bp)
xfs_trans_brelse(args.trans, leaf_bp);
out_trans_cancel: out_trans_cancel:
if (args.trans) if (args.trans)
xfs_trans_cancel(args.trans); xfs_trans_cancel(args.trans);
......
...@@ -140,7 +140,7 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, ...@@ -140,7 +140,7 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name,
unsigned char *value, int *valuelenp, int flags); unsigned char *value, int *valuelenp, int flags);
int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
unsigned char *value, int valuelen, int flags); 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(struct xfs_inode *dp, const unsigned char *name, int flags);
int xfs_attr_remove_args(struct xfs_da_args *args); int xfs_attr_remove_args(struct xfs_da_args *args);
int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
......
...@@ -274,13 +274,15 @@ xfs_defer_trans_roll( ...@@ -274,13 +274,15 @@ xfs_defer_trans_roll(
trace_xfs_defer_trans_roll(tp, _RET_IP_); 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); error = xfs_trans_roll(tpp);
tp = *tpp; tp = *tpp;
if (error) {
trace_xfs_defer_trans_roll_error(tp, error);
return error;
}
/* Rejoin the joined inodes. */ /* Rejoin the joined inodes. */
for (i = 0; i < ipcount; i++) for (i = 0; i < ipcount; i++)
...@@ -292,6 +294,8 @@ xfs_defer_trans_roll( ...@@ -292,6 +294,8 @@ xfs_defer_trans_roll(
xfs_trans_bhold(tp, bplist[i]); xfs_trans_bhold(tp, bplist[i]);
} }
if (error)
trace_xfs_defer_trans_roll_error(tp, error);
return error; return error;
} }
......
...@@ -277,7 +277,8 @@ xfs_dquot_set_prealloc_limits(struct xfs_dquot *dqp) ...@@ -277,7 +277,8 @@ xfs_dquot_set_prealloc_limits(struct xfs_dquot *dqp)
/* /*
* Ensure that the given in-core dquot has a buffer on disk backing it, and * Ensure that the given in-core dquot has a buffer on disk backing it, and
* return the buffer. This is called when the bmapi finds a hole. * return the buffer locked and held. This is called when the bmapi finds a
* hole.
*/ */
STATIC int STATIC int
xfs_dquot_disk_alloc( xfs_dquot_disk_alloc(
...@@ -355,13 +356,14 @@ xfs_dquot_disk_alloc( ...@@ -355,13 +356,14 @@ xfs_dquot_disk_alloc(
* If everything succeeds, the caller of this function is returned a * If everything succeeds, the caller of this function is returned a
* buffer that is locked and held to the transaction. The caller * buffer that is locked and held to the transaction. The caller
* is responsible for unlocking any buffer passed back, either * is responsible for unlocking any buffer passed back, either
* manually or by committing the transaction. * manually or by committing the transaction. On error, the buffer is
* released and not passed back.
*/ */
xfs_trans_bhold(tp, bp); xfs_trans_bhold(tp, bp);
error = xfs_defer_finish(tpp); error = xfs_defer_finish(tpp);
tp = *tpp;
if (error) { if (error) {
xfs_buf_relse(bp); xfs_trans_bhold_release(*tpp, bp);
xfs_trans_brelse(*tpp, bp);
return error; return error;
} }
*bpp = bp; *bpp = bp;
...@@ -521,7 +523,6 @@ xfs_qm_dqread_alloc( ...@@ -521,7 +523,6 @@ xfs_qm_dqread_alloc(
struct xfs_buf **bpp) struct xfs_buf **bpp)
{ {
struct xfs_trans *tp; struct xfs_trans *tp;
struct xfs_buf *bp;
int error; int error;
error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc, error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc,
...@@ -529,7 +530,7 @@ xfs_qm_dqread_alloc( ...@@ -529,7 +530,7 @@ xfs_qm_dqread_alloc(
if (error) if (error)
goto err; goto err;
error = xfs_dquot_disk_alloc(&tp, dqp, &bp); error = xfs_dquot_disk_alloc(&tp, dqp, bpp);
if (error) if (error)
goto err_cancel; goto err_cancel;
...@@ -539,10 +540,10 @@ xfs_qm_dqread_alloc( ...@@ -539,10 +540,10 @@ xfs_qm_dqread_alloc(
* Buffer was held to the transaction, so we have to unlock it * Buffer was held to the transaction, so we have to unlock it
* manually here because we're not passing it back. * manually here because we're not passing it back.
*/ */
xfs_buf_relse(bp); xfs_buf_relse(*bpp);
*bpp = NULL;
goto err; goto err;
} }
*bpp = bp;
return 0; return 0;
err_cancel: err_cancel:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment