Skip to content

Commit

Permalink
xfs: eliminate committed arg from xfs_bmap_finish
Browse files Browse the repository at this point in the history 
Calls to xfs_bmap_finish() and xfs_trans_ijoin(), and the
associated comments were replicated several times across
the attribute code, all dealing with what to do if the
transaction was or wasn't committed.

And in that replicated code, an ASSERT() test of an
uninitialized variable occurs in several locations:

	error = xfs_attr_thing(&args);
	if (!error) {
		error = xfs_bmap_finish(&args.trans, args.flist,
					&committed);
	}
	if (error) {
		ASSERT(committed);

If the first xfs_attr_thing() failed, we'd skip the xfs_bmap_finish,
never set "committed", and then test it in the ASSERT.

Fix this up by moving the committed state internal to xfs_bmap_finish,
and add a new inode argument.  If an inode is passed in, it is passed
through to __xfs_trans_roll() and joined to the transaction there if
the transaction was committed.

xfs_qm_dqalloc() was a little unique in that it called bjoin rather
than ijoin, but as Dave points out we can detect the committed state
but checking whether (*tpp != tp).

Addresses-Coverity-Id: 102360
Addresses-Coverity-Id: 102361
Addresses-Coverity-Id: 102363
Addresses-Coverity-Id: 102364
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Dave Chinner <david@fromorbit.com>
  • Loading branch information
Eric Sandeen authored and Dave Chinner committed Jan 11, 2016
1 parent e354381 commit f6106ef
Show file tree
Hide file tree
Showing 10 changed files with 68 additions and 218 deletions.
141 changes: 23 additions & 118 deletions fs/xfs/libxfs/xfs_attr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ xfs_attr_set(
struct xfs_trans_res tres;
xfs_fsblock_t firstblock;
int rsvd = (flags & ATTR_ROOT) != 0;
int error, err2, committed, local;
int error, err2, local;

XFS_STATS_INC(mp, xs_attr_set);

Expand Down Expand Up @@ -334,24 +334,14 @@ xfs_attr_set(
*/
xfs_bmap_init(args.flist, args.firstblock);
error = xfs_attr_shortform_to_leaf(&args);
if (!error) {
error = xfs_bmap_finish(&args.trans, args.flist,
&committed);
}
if (!error)
error = xfs_bmap_finish(&args.trans, args.flist, dp);
if (error) {
ASSERT(committed);
args.trans = NULL;
xfs_bmap_cancel(&flist);
goto out;
}

/*
* bmap_finish() may have committed the last trans and started
* a new one. We need the inode to be in all transactions.
*/
if (committed)
xfs_trans_ijoin(args.trans, dp, 0);

/*
* Commit the leaf transformation. We'll need another (linked)
* transaction to add the new attribute to the leaf.
Expand Down Expand Up @@ -568,7 +558,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
{
xfs_inode_t *dp;
struct xfs_buf *bp;
int retval, error, committed, forkoff;
int retval, error, forkoff;

trace_xfs_attr_leaf_addname(args);

Expand Down Expand Up @@ -628,24 +618,14 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
*/
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_attr3_leaf_to_node(args);
if (!error) {
error = xfs_bmap_finish(&args->trans, args->flist,
&committed);
}
if (!error)
error = xfs_bmap_finish(&args->trans, args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
return error;
}

/*
* bmap_finish() may have committed the last trans and started
* a new one. We need the inode to be in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);

/*
* Commit the current trans (including the inode) and start
* a new one.
Expand Down Expand Up @@ -729,25 +709,14 @@ xfs_attr_leaf_addname(xfs_da_args_t *args)
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
/* bp is gone due to xfs_da_shrink_inode */
if (!error) {
if (!error)
error = xfs_bmap_finish(&args->trans,
args->flist,
&committed);
}
args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
return error;
}

/*
* bmap_finish() may have committed the last trans
* and started a new one. We need the inode to be
* in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);
}

/*
Expand Down Expand Up @@ -775,7 +744,7 @@ xfs_attr_leaf_removename(xfs_da_args_t *args)
{
xfs_inode_t *dp;
struct xfs_buf *bp;
int error, committed, forkoff;
int error, forkoff;

trace_xfs_attr_leaf_removename(args);

Expand Down Expand Up @@ -803,23 +772,13 @@ xfs_attr_leaf_removename(xfs_da_args_t *args)
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
/* bp is gone due to xfs_da_shrink_inode */
if (!error) {
error = xfs_bmap_finish(&args->trans, args->flist,
&committed);
}
if (!error)
error = xfs_bmap_finish(&args->trans, args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
return error;
}

/*
* bmap_finish() may have committed the last trans and started
* a new one. We need the inode to be in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);
}
return 0;
}
Expand Down Expand Up @@ -877,7 +836,7 @@ xfs_attr_node_addname(xfs_da_args_t *args)
xfs_da_state_blk_t *blk;
xfs_inode_t *dp;
xfs_mount_t *mp;
int committed, retval, error;
int retval, error;

trace_xfs_attr_node_addname(args);

Expand Down Expand Up @@ -938,26 +897,15 @@ xfs_attr_node_addname(xfs_da_args_t *args)
state = NULL;
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_attr3_leaf_to_node(args);
if (!error) {
if (!error)
error = xfs_bmap_finish(&args->trans,
args->flist,
&committed);
}
args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
goto out;
}

/*
* bmap_finish() may have committed the last trans
* and started a new one. We need the inode to be
* in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);

/*
* Commit the node conversion and start the next
* trans in the chain.
Expand All @@ -977,23 +925,13 @@ xfs_attr_node_addname(xfs_da_args_t *args)
*/
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_da3_split(state);
if (!error) {
error = xfs_bmap_finish(&args->trans, args->flist,
&committed);
}
if (!error)
error = xfs_bmap_finish(&args->trans, args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
goto out;
}

/*
* bmap_finish() may have committed the last trans and started
* a new one. We need the inode to be in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);
} else {
/*
* Addition succeeded, update Btree hashvals.
Expand Down Expand Up @@ -1086,25 +1024,14 @@ xfs_attr_node_addname(xfs_da_args_t *args)
if (retval && (state->path.active > 1)) {
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_da3_join(state);
if (!error) {
if (!error)
error = xfs_bmap_finish(&args->trans,
args->flist,
&committed);
}
args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
goto out;
}

/*
* bmap_finish() may have committed the last trans
* and started a new one. We need the inode to be
* in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);
}

/*
Expand Down Expand Up @@ -1146,7 +1073,7 @@ xfs_attr_node_removename(xfs_da_args_t *args)
xfs_da_state_blk_t *blk;
xfs_inode_t *dp;
struct xfs_buf *bp;
int retval, error, committed, forkoff;
int retval, error, forkoff;

trace_xfs_attr_node_removename(args);

Expand Down Expand Up @@ -1220,24 +1147,13 @@ xfs_attr_node_removename(xfs_da_args_t *args)
if (retval && (state->path.active > 1)) {
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_da3_join(state);
if (!error) {
error = xfs_bmap_finish(&args->trans, args->flist,
&committed);
}
if (!error)
error = xfs_bmap_finish(&args->trans, args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
goto out;
}

/*
* bmap_finish() may have committed the last trans and started
* a new one. We need the inode to be in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);

/*
* Commit the Btree join operation and start a new trans.
*/
Expand Down Expand Up @@ -1265,25 +1181,14 @@ xfs_attr_node_removename(xfs_da_args_t *args)
xfs_bmap_init(args->flist, args->firstblock);
error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
/* bp is gone due to xfs_da_shrink_inode */
if (!error) {
if (!error)
error = xfs_bmap_finish(&args->trans,
args->flist,
&committed);
}
args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
goto out;
}

/*
* bmap_finish() may have committed the last trans
* and started a new one. We need the inode to be
* in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);
} else
xfs_trans_brelse(args->trans, bp);
}
Expand Down
31 changes: 4 additions & 27 deletions fs/xfs/libxfs/xfs_attr_remote.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,6 @@ xfs_attr_rmtval_set(
* Roll through the "value", allocating blocks on disk as required.
*/
while (blkcnt > 0) {
int committed;

/*
* Allocate a single extent, up to the size of the value.
*
Expand All @@ -467,24 +465,14 @@ xfs_attr_rmtval_set(
error = xfs_bmapi_write(args->trans, dp, (xfs_fileoff_t)lblkno,
blkcnt, XFS_BMAPI_ATTRFORK, args->firstblock,
args->total, &map, &nmap, args->flist);
if (!error) {
error = xfs_bmap_finish(&args->trans, args->flist,
&committed);
}
if (!error)
error = xfs_bmap_finish(&args->trans, args->flist, dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
return error;
}

/*
* bmap_finish() may have committed the last trans and started
* a new one. We need the inode to be in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, dp, 0);

ASSERT(nmap == 1);
ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
(map.br_startblock != HOLESTARTBLOCK));
Expand Down Expand Up @@ -615,30 +603,19 @@ xfs_attr_rmtval_remove(
blkcnt = args->rmtblkcnt;
done = 0;
while (!done) {
int committed;

xfs_bmap_init(args->flist, args->firstblock);
error = xfs_bunmapi(args->trans, args->dp, lblkno, blkcnt,
XFS_BMAPI_ATTRFORK, 1, args->firstblock,
args->flist, &done);
if (!error) {
if (!error)
error = xfs_bmap_finish(&args->trans, args->flist,
&committed);
}
args->dp);
if (error) {
ASSERT(committed);
args->trans = NULL;
xfs_bmap_cancel(args->flist);
return error;
}

/*
* bmap_finish() may have committed the last trans and started
* a new one. We need the inode to be in all transactions.
*/
if (committed)
xfs_trans_ijoin(args->trans, args->dp, 0);

/*
* Close out trans and start the next one in the chain.
*/
Expand Down
6 changes: 2 additions & 4 deletions fs/xfs/libxfs/xfs_bmap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1117,7 +1117,6 @@ xfs_bmap_add_attrfork(
xfs_trans_t *tp; /* transaction pointer */
int blks; /* space reservation */
int version = 1; /* superblock attr version */
int committed; /* xaction was committed */
int logflags; /* logging flags */
int error; /* error return value */

Expand Down Expand Up @@ -1220,7 +1219,7 @@ xfs_bmap_add_attrfork(
xfs_log_sb(tp);
}

error = xfs_bmap_finish(&tp, &flist, &committed);
error = xfs_bmap_finish(&tp, &flist, NULL);
if (error)
goto bmap_cancel;
error = xfs_trans_commit(tp);
Expand Down Expand Up @@ -5957,7 +5956,6 @@ xfs_bmap_split_extent(
struct xfs_trans *tp;
struct xfs_bmap_free free_list;
xfs_fsblock_t firstfsb;
int committed;
int error;

tp = xfs_trans_alloc(mp, XFS_TRANS_DIOSTRAT);
Expand All @@ -5978,7 +5976,7 @@ xfs_bmap_split_extent(
if (error)
goto out;

error = xfs_bmap_finish(&tp, &free_list, &committed);
error = xfs_bmap_finish(&tp, &free_list, NULL);
if (error)
goto out;

Expand Down
Loading

0 comments on commit f6106ef

Please sign in to comment.