Commit f6106efa authored by Eric Sandeen's avatar Eric Sandeen Committed by Dave Chinner
Browse files

xfs: eliminate committed arg from xfs_bmap_finish 



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: default avatarEric Sandeen <sandeen@redhat.com>
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Signed-off-by: default avatarDave Chinner <david@fromorbit.com>
parent e3543819

fs/xfs/libxfs/xfs_attr.c

+23 −118
Original line number Diff line number Diff line
@@ -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);
@@ -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.
@@ -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);
@@ -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.
@@ -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);

		}



		/*
@@ -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);
@@ -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;

}
@@ -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);
@@ -938,26 +897,15 @@ restart: 
			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.
@@ -977,23 +925,13 @@ restart: 
		 */

		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.
@@ -1086,25 +1024,14 @@ restart: 
		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);

		}



		/*
@@ -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);
@@ -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.

		 */
@@ -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);

	}

fs/xfs/libxfs/xfs_attr_remote.c

+4 −27
Original line number Diff line number Diff line
@@ -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.

		 *
@@ -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));
@@ -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.

		 */

fs/xfs/libxfs/xfs_bmap.c

+2 −4
Original line number Diff line number Diff line
@@ -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 */
@@ -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);
@@ -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);
@@ -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;

fs/xfs/libxfs/xfs_bmap.h

+1 −1
Original line number Diff line number Diff line
@@ -195,7 +195,7 @@ void xfs_bmap_add_free(xfs_fsblock_t bno, xfs_filblks_t len, 
		struct xfs_bmap_free *flist, struct xfs_mount *mp);

void	xfs_bmap_cancel(struct xfs_bmap_free *flist);

int	xfs_bmap_finish(struct xfs_trans **tp, struct xfs_bmap_free *flist,

			int *committed);

			struct xfs_inode *ip);

void	xfs_bmap_compute_maxlevels(struct xfs_mount *mp, int whichfork);

int	xfs_bmap_first_unused(struct xfs_trans *tp, struct xfs_inode *ip,

		xfs_extlen_t len, xfs_fileoff_t *unused, int whichfork);

fs/xfs/xfs_bmap_util.c

+16 −27
Original line number Diff line number Diff line
@@ -91,32 +91,32 @@ xfs_zero_extent( 
 * last due to locking considerations.  We never free any extents in

 * the first transaction.

 *

 * Return 1 if the given transaction was committed and a new one

 * started, and 0 otherwise in the committed parameter.

 * If an inode *ip is provided, rejoin it to the transaction if

 * the transaction was committed.

 */

int						/* error */

xfs_bmap_finish(

	struct xfs_trans		**tp,	/* transaction pointer addr */

	struct xfs_bmap_free		*flist,	/* i/o: list extents to free */

	int				*committed)/* xact committed or not */

	struct xfs_inode		*ip)

{

	struct xfs_efd_log_item		*efd;	/* extent free data */

	struct xfs_efi_log_item		*efi;	/* extent free intention */

	int				error;	/* error return value */

	int				committed;/* xact committed or not */

	struct xfs_bmap_free_item	*free;	/* free extent item */

	struct xfs_bmap_free_item	*next;	/* next item on free list */



	ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);

	if (flist->xbf_count == 0) {

		*committed = 0;

	if (flist->xbf_count == 0)

		return 0;

	}



	efi = xfs_trans_get_efi(*tp, flist->xbf_count);

	for (free = flist->xbf_first; free; free = free->xbfi_next)

		xfs_trans_log_efi_extent(*tp, efi, free->xbfi_startblock,

			free->xbfi_blockcount);



	error = __xfs_trans_roll(tp, NULL, committed);

	error = __xfs_trans_roll(tp, ip, &committed);

	if (error) {

		/*

		 * If the transaction was committed, drop the EFD reference
@@ -128,16 +128,13 @@ xfs_bmap_finish( 
		 * transaction so we should return committed=1 even though we're

		 * returning an error.

		 */

		if (*committed) {

		if (committed) {

			xfs_efi_release(efi);

			xfs_force_shutdown((*tp)->t_mountp,

				(error == -EFSCORRUPTED) ?

					SHUTDOWN_CORRUPT_INCORE :

					SHUTDOWN_META_IO_ERROR);

		} else {

			*committed = 1;

		}



		return error;

	}
@@ -969,7 +966,6 @@ xfs_alloc_file_space( 
	xfs_bmbt_irec_t		imaps[1], *imapp;

	xfs_bmap_free_t		free_list;

	uint			qblocks, resblks, resrtextents;

	int			committed;

	int			error;



	trace_xfs_alloc_file_space(ip);
@@ -1064,23 +1060,20 @@ xfs_alloc_file_space( 
		error = xfs_bmapi_write(tp, ip, startoffset_fsb,

					allocatesize_fsb, alloc_type, &firstfsb,

					resblks, imapp, &nimaps, &free_list);

		if (error) {

		if (error)

			goto error0;

		}



		/*

		 * Complete the transaction

		 */

		error = xfs_bmap_finish(&tp, &free_list, &committed);

		if (error) {

		error = xfs_bmap_finish(&tp, &free_list, NULL);

		if (error)

			goto error0;

		}



		error = xfs_trans_commit(tp);

		xfs_iunlock(ip, XFS_ILOCK_EXCL);

		if (error) {

		if (error)

			break;

		}



		allocated_fsb = imapp->br_blockcount;
@@ -1206,7 +1199,6 @@ xfs_free_file_space( 
	xfs_off_t		offset,

	xfs_off_t		len)

{

	int			committed;

	int			done;

	xfs_fileoff_t		endoffset_fsb;

	int			error;
@@ -1346,17 +1338,15 @@ xfs_free_file_space( 
		error = xfs_bunmapi(tp, ip, startoffset_fsb,

				  endoffset_fsb - startoffset_fsb,

				  0, 2, &firstfsb, &free_list, &done);

		if (error) {

		if (error)

			goto error0;

		}



		/*

		 * complete the transaction

		 */

		error = xfs_bmap_finish(&tp, &free_list, &committed);

		if (error) {

		error = xfs_bmap_finish(&tp, &free_list, NULL);

		if (error)

			goto error0;

		}



		error = xfs_trans_commit(tp);

		xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -1434,7 +1424,6 @@ xfs_shift_file_space( 
	int			error;

	struct xfs_bmap_free	free_list;

	xfs_fsblock_t		first_block;

	int			committed;

	xfs_fileoff_t		stop_fsb;

	xfs_fileoff_t		next_fsb;

	xfs_fileoff_t		shift_fsb;
@@ -1526,7 +1515,7 @@ xfs_shift_file_space( 
		if (error)

			goto out_bmap_cancel;



		error = xfs_bmap_finish(&tp, &free_list, &committed);

		error = xfs_bmap_finish(&tp, &free_list, NULL);

		if (error)

			goto out_bmap_cancel;

CRA Git | Maintained and supported by SUSTech CRA and CCSE