[PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Andi Kleen]

For my configuration, that is without quota or RT.

Mostly dead code removed I think (but needs additional review) 

That is there were one or two bad error handling cases,
but they were not easily fixable, with comments 
and I left the warnings in for those for you to remember.

e.g. if there is a ENOSPC down in xfs_trans.c while
modifying the superblock it would not be handled.

Unused statements were mostly related to stub macros for disabled
features like QUOTA or RT ALLOC. I replace those with
inlines.

There were also some problems with variables used in ASSERT()
I partly moved those into the ASSERT itself and partly
used a new QASSERT that always evaluates.

Cc: xfs@oss.sgi.com

Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 fs/xfs/linux-2.6/xfs_sync.c |    3 +++
 fs/xfs/support/debug.h      |    4 ++++
 fs/xfs/xfs_alloc.c          |   10 +++-------
 fs/xfs/xfs_da_btree.c       |   15 +++++----------
 fs/xfs/xfs_dir2_block.c     |    6 +++---
 fs/xfs/xfs_filestream.c     |   10 ++--------
 fs/xfs/xfs_iget.c           |    3 ---
 fs/xfs/xfs_inode.c          |    4 ----
 fs/xfs/xfs_inode_item.c     |    8 ++------
 fs/xfs/xfs_log.c            |    2 --
 fs/xfs/xfs_quota.h          |   14 ++++++++++----
 fs/xfs/xfs_trans.c          |    1 +
 12 files changed, 33 insertions(+), 47 deletions(-)

Index: linux-2.6.35-rc2-gcc/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/linux-2.6/xfs_sync.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/linux-2.6/xfs_sync.c
@@ -554,6 +554,9 @@ xfs_sync_worker(
 		xfs_log_force(mp, 0);
 		xfs_reclaim_inodes(mp, 0);
 		/* dgc: errors ignored here */
+		/* ak: yes and you'll get a warning for it now when you
+		 * upgrade compilers.
+		 */
 		error = xfs_qm_sync(mp, SYNC_TRYLOCK);
 		if (xfs_log_need_covered(mp))
 			error = xfs_commit_dummy_trans(mp, 0);
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_da_btree.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_da_btree.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_da_btree.c
@@ -581,10 +581,8 @@ xfs_da_node_add(xfs_da_state_t *state, x
 	xfs_da_intnode_t *node;
 	xfs_da_node_entry_t *btree;
 	int tmp;
-	xfs_mount_t *mp;
 
 	node = oldblk->bp->data;
-	mp = state->mp;
 	ASSERT(be16_to_cpu(node->hdr.info.magic) == XFS_DA_NODE_MAGIC);
 	ASSERT((oldblk->index >= 0) && (oldblk->index <= be16_to_cpu(node->hdr.count)));
 	ASSERT(newblk->blkno != 0);
@@ -710,8 +708,6 @@ STATIC int
 xfs_da_root_join(xfs_da_state_t *state, xfs_da_state_blk_t *root_blk)
 {
 	xfs_da_intnode_t *oldroot;
-	/* REFERENCED */
-	xfs_da_blkinfo_t *blkinfo;
 	xfs_da_args_t *args;
 	xfs_dablk_t child;
 	xfs_dabuf_t *bp;
@@ -742,15 +738,14 @@ xfs_da_root_join(xfs_da_state_t *state,
 	if (error)
 		return(error);
 	ASSERT(bp != NULL);
-	blkinfo = bp->data;
 	if (be16_to_cpu(oldroot->hdr.level) == 1) {
-		ASSERT(be16_to_cpu(blkinfo->magic) == XFS_DIR2_LEAFN_MAGIC ||
-		       be16_to_cpu(blkinfo->magic) == XFS_ATTR_LEAF_MAGIC);
+		ASSERT(be16_to_cpu(bp->data->magic) == XFS_DIR2_LEAFN_MAGIC ||
+		       be16_to_cpu(bp->data->magic) == XFS_ATTR_LEAF_MAGIC);
 	} else {
-		ASSERT(be16_to_cpu(blkinfo->magic) == XFS_DA_NODE_MAGIC);
+		ASSERT(be16_to_cpu(bp->data->magic) == XFS_DA_NODE_MAGIC);
 	}
-	ASSERT(!blkinfo->forw);
-	ASSERT(!blkinfo->back);
+	ASSERT(!bp->data->forw);
+	ASSERT(!bp->data->back);
 	memcpy(root_blk->bp->data, bp->data, state->blocksize);
 	xfs_da_log_buf(args->trans, root_blk->bp, 0, state->blocksize - 1);
 	error = xfs_da_shrink_inode(args, child, bp);
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_dir2_block.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_dir2_block.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_dir2_block.c
@@ -1073,10 +1073,10 @@ xfs_dir2_sf_to_block(
 	 */
 
 	buf_len = dp->i_df.if_bytes;
-	buf = kmem_alloc(dp->i_df.if_bytes, KM_SLEEP);
+	buf = kmem_alloc(buf_len, KM_SLEEP);
 
-	memcpy(buf, sfp, dp->i_df.if_bytes);
-	xfs_idata_realloc(dp, -dp->i_df.if_bytes, XFS_DATA_FORK);
+	memcpy(buf, sfp, buf_len);
+	xfs_idata_realloc(dp, -buf_len, XFS_DATA_FORK);
 	dp->i_d.di_size = 0;
 	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 	/*
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_filestream.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_filestream.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_filestream.c
@@ -140,9 +140,8 @@ _xfs_filestream_pick_ag(
 	int		flags,
 	xfs_extlen_t	minlen)
 {
-	int		streams, max_streams;
 	int		err, trylock, nscan;
-	xfs_extlen_t	longest, free, minfree, maxfree = 0;
+	xfs_extlen_t	longest, minfree, maxfree = 0;
 	xfs_agnumber_t	ag, max_ag = NULLAGNUMBER;
 	struct xfs_perag *pag;
 
@@ -174,7 +173,6 @@ _xfs_filestream_pick_ag(
 		/* Keep track of the AG with the most free blocks. */
 		if (pag->pagf_freeblks > maxfree) {
 			maxfree = pag->pagf_freeblks;
-			max_streams = atomic_read(&pag->pagf_fstrms);
 			max_ag = ag;
 		}
 
@@ -196,8 +194,6 @@ _xfs_filestream_pick_ag(
 		     (flags & XFS_PICK_LOWSPACE))) {
 
 			/* Break out, retaining the reference on the AG. */
-			free = pag->pagf_freeblks;
-			streams = atomic_read(&pag->pagf_fstrms);
 			xfs_perag_put(pag);
 			*agp = ag;
 			break;
@@ -234,8 +230,6 @@ next_ag:
 		if (max_ag != NULLAGNUMBER) {
 			xfs_filestream_get_ag(mp, max_ag);
 			TRACE_AG_PICK1(mp, max_ag, maxfree);
-			streams = max_streams;
-			free = maxfree;
 			*agp = max_ag;
 			break;
 		}
@@ -364,7 +358,7 @@ xfs_fstrm_free_func(
 	/* Drop the reference taken on the AG when the item was added. */
 	ref = xfs_filestream_put_ag(ip->i_mount, item->ag);
 
-	ASSERT(ref >= 0);
+	QASSERT(ref >= 0);
 	TRACE_FREE(ip->i_mount, ip, item->pip, item->ag,
 		xfs_filestream_peek_ag(ip->i_mount, item->ag));
 
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_iget.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_iget.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_iget.c
@@ -265,7 +265,6 @@ xfs_iget_cache_miss(
 {
 	struct xfs_inode	*ip;
 	int			error;
-	unsigned long		first_index, mask;
 	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, ino);
 
 	ip = xfs_inode_alloc(mp, ino);
@@ -302,8 +301,6 @@ xfs_iget_cache_miss(
 			BUG();
 	}
 
-	mask = ~(((XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog)) - 1);
-	first_index = agino & mask;
 	write_lock(&pag->pag_ici_lock);
 
 	/* insert the new inode */
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_inode.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_inode.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_inode.c
@@ -925,7 +925,6 @@ xfs_iread_extents(
 	int		error;
 	xfs_ifork_t	*ifp;
 	xfs_extnum_t	nextents;
-	size_t		size;
 
 	if (unlikely(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
 		XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
@@ -933,7 +932,6 @@ xfs_iread_extents(
 		return XFS_ERROR(EFSCORRUPTED);
 	}
 	nextents = XFS_IFORK_NEXTENTS(ip, whichfork);
-	size = nextents * sizeof(xfs_bmbt_rec_t);
 	ifp = XFS_IFORK_PTR(ip, whichfork);
 
 	/*
@@ -3517,13 +3515,11 @@ xfs_iext_remove_indirect(
 	xfs_extnum_t	ext_diff;	/* extents to remove in current list */
 	xfs_extnum_t	nex1;		/* number of extents before idx */
 	xfs_extnum_t	nex2;		/* extents after idx + count */
-	int		nlists;		/* entries in indirection array */
 	int		page_idx = idx;	/* index in target extent list */
 
 	ASSERT(ifp->if_flags & XFS_IFEXTIREC);
 	erp = xfs_iext_idx_to_irec(ifp,  &page_idx, &erp_idx, 0);
 	ASSERT(erp != NULL);
-	nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
 	nex1 = page_idx;
 	ext_cnt = count;
 	while (ext_cnt) {
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_inode_item.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_inode_item.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_inode_item.c
@@ -220,7 +220,6 @@ xfs_inode_item_format(
 	xfs_inode_t		*ip;
 	size_t			data_bytes;
 	xfs_bmbt_rec_t		*ext_buffer;
-	int			nrecs;
 	xfs_mount_t		*mp;
 
 	ip = iip->ili_inode;
@@ -323,9 +322,8 @@ xfs_inode_item_format(
 			ASSERT(ip->i_df.if_u1.if_extents != NULL);
 			ASSERT(ip->i_d.di_nextents > 0);
 			ASSERT(iip->ili_extents_buf == NULL);
-			nrecs = ip->i_df.if_bytes /
-				(uint)sizeof(xfs_bmbt_rec_t);
-			ASSERT(nrecs > 0);
+			ASSERT((ip->i_df.if_bytes /
+				(uint)sizeof(xfs_bmbt_rec_t)) > 0);
 #ifdef XFS_NATIVE_HOST
 			if (nrecs == ip->i_d.di_nextents) {
 				/*
@@ -957,10 +955,8 @@ xfs_iflush_abort(
 	xfs_inode_t		*ip)
 {
 	xfs_inode_log_item_t	*iip = ip->i_itemp;
-	xfs_mount_t		*mp;
 
 	iip = ip->i_itemp;
-	mp = ip->i_mount;
 	if (iip) {
 		struct xfs_ail	*ailp = iip->ili_item.li_ailp;
 		if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_log.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_log.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_log.c
@@ -1047,7 +1047,6 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	xlog_in_core_t		*iclog, *prev_iclog=NULL;
 	xfs_buf_t		*bp;
 	int			i;
-	int			iclogsize;
 	int			error = ENOMEM;
 	uint			log2_size = 0;
 
@@ -1127,7 +1126,6 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	 * with different amounts of memory.  See the definition of
 	 * xlog_in_core_t in xfs_log_priv.h for details.
 	 */
-	iclogsize = log->l_iclog_size;
 	ASSERT(log->l_iclog_size >= 4096);
 	for (i=0; i < log->l_iclog_bufs; i++) {
 		*iclogp = kmem_zalloc(sizeof(xlog_in_core_t), KM_MAYFAIL);
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_quota.h
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_quota.h
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_quota.h
@@ -346,7 +346,13 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip,
 #define xfs_trans_mod_dquot_byino(tp, ip, fields, delta)
 #define xfs_trans_apply_dquot_deltas(tp)
 #define xfs_trans_unreserve_and_mod_dquots(tp)
-#define xfs_trans_reserve_quota_nblks(tp, ip, nblks, ninos, flags)	(0)
+
+static inline int xfs_trans_reserve_quota_nblks(struct xfs_trans *t,
+		struct xfs_inode *i, long a, long b, uint c)
+{
+	return 0;
+}
+
 #define xfs_trans_reserve_quota_bydquots(tp, mp, u, g, nb, ni, fl)	(0)
 #define xfs_qm_vop_create_dqattach(tp, ip, u, g)
 #define xfs_qm_vop_rename_dqattach(it)					(0)
@@ -355,13 +361,13 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip,
 #define xfs_qm_dqattach(ip, fl)						(0)
 #define xfs_qm_dqattach_locked(ip, fl)					(0)
 #define xfs_qm_dqdetach(ip)
-#define xfs_qm_dqrele(d)
+static inline void xfs_qm_dqrele(struct xfs_dquot *d) {}
 #define xfs_qm_statvfs(ip, s)
-#define xfs_qm_sync(mp, fl)						(0)
+static inline int xfs_qm_sync(struct xfs_mount *m, int i) { return 0; }
 #define xfs_qm_newmount(mp, a, b)					(0)
 #define xfs_qm_mount_quotas(mp)
 #define xfs_qm_unmount(mp)
-#define xfs_qm_unmount_quotas(mp)					(0)
+static inline void xfs_qm_unmount_quotas(struct xfs_mount *m) {}
 #endif /* CONFIG_XFS_QUOTA */
 
 #define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, ninos, flags) \
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_trans.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_trans.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_trans.c
@@ -1120,6 +1120,7 @@ xfs_trans_unreserve_and_mod_sb(
 		error = xfs_mod_incore_sb_batch(tp->t_mountp, msb,
 			(uint)(msbp - msb), rsvd);
 		ASSERT(error == 0);
+		/* FIXME: need real error handling here, error can be ENOSPC */
 	}
 }
 
Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_alloc.c
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_alloc.c
+++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_alloc.c
@@ -688,8 +688,6 @@ xfs_alloc_ag_vextent_near(
 	xfs_agblock_t	ltbno;		/* start bno of left side entry */
 	xfs_agblock_t	ltbnoa;		/* aligned ... */
 	xfs_extlen_t	ltdiff;		/* difference to left side entry */
-	/*REFERENCED*/
-	xfs_agblock_t	ltend;		/* end bno of left side entry */
 	xfs_extlen_t	ltlen;		/* length of left side entry */
 	xfs_extlen_t	ltlena;		/* aligned ... */
 	xfs_agblock_t	ltnew;		/* useful start bno of left side */
@@ -814,8 +812,7 @@ xfs_alloc_ag_vextent_near(
 		if ((error = xfs_alloc_get_rec(cnt_cur, &ltbno, &ltlen, &i)))
 			goto error0;
 		XFS_WANT_CORRUPTED_GOTO(i == 1, error0);
-		ltend = ltbno + ltlen;
-		ASSERT(ltend <= be32_to_cpu(XFS_BUF_TO_AGF(args->agbp)->agf_length));
+		ASSERT(ltbno + ltlen <= be32_to_cpu(XFS_BUF_TO_AGF(args->agbp)->agf_length));
 		args->len = blen;
 		if (!xfs_alloc_fix_minleft(args)) {
 			xfs_btree_del_cursor(cnt_cur, XFS_BTREE_NOERROR);
@@ -828,7 +825,7 @@ xfs_alloc_ag_vextent_near(
 		 */
 		args->agbno = bnew;
 		ASSERT(bnew >= ltbno);
-		ASSERT(bnew + blen <= ltend);
+		ASSERT(bnew + blen <= ltbno + ltlen);
 		/*
 		 * Set up a cursor for the by-bno tree.
 		 */
@@ -1157,7 +1154,6 @@ xfs_alloc_ag_vextent_near(
 	/*
 	 * Fix up the length and compute the useful address.
 	 */
-	ltend = ltbno + ltlen;
 	args->len = XFS_EXTLEN_MIN(ltlena, args->maxlen);
 	xfs_alloc_fix_len(args);
 	if (!xfs_alloc_fix_minleft(args)) {
@@ -1170,7 +1166,7 @@ xfs_alloc_ag_vextent_near(
 	(void)xfs_alloc_compute_diff(args->agbno, rlen, args->alignment, ltbno,
 		ltlen, &ltnew);
 	ASSERT(ltnew >= ltbno);
-	ASSERT(ltnew + rlen <= ltend);
+	ASSERT(ltnew + rlen <= ltbno + ltlen);
 	ASSERT(ltnew + rlen <= be32_to_cpu(XFS_BUF_TO_AGF(args->agbp)->agf_length));
 	args->agbno = ltnew;
 	if ((error = xfs_alloc_fixup_trees(cnt_cur, bno_cur_lt, ltbno, ltlen,
Index: linux-2.6.35-rc2-gcc/fs/xfs/support/debug.h
===================================================================
--- linux-2.6.35-rc2-gcc.orig/fs/xfs/support/debug.h
+++ linux-2.6.35-rc2-gcc/fs/xfs/support/debug.h
@@ -37,6 +37,9 @@ extern void assfail(char *expr, char *f,
 #ifndef DEBUG
 #define ASSERT(expr)	((void)0)
 
+/* Assert that always evaluates its input to avoid warnings */
+#define QASSERT(expr)	((void)(expr))
+
 #ifndef STATIC
 # define STATIC static noinline
 #endif
@@ -45,6 +48,7 @@ extern void assfail(char *expr, char *f,
 
 #define ASSERT(expr)	\
 	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
+#define QASSERT(expr) ASSERT(expr)
 
 #ifndef STATIC
 # define STATIC noinline

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Christoph Hellwig]

Most of this looks good to me, but I really don't like the QASSERT
macro.  If you're fine with it I'll split this up into smaller patches
and resubmit the unmodified parts under your name and redo those bits
I don't like.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Andi Kleen]

On Fri, Jun 11, 2010 at 12:20:07PM -0400, Christoph Hellwig wrote:
> Most of this looks good to me, but I really don't like the QASSERT
> macro.  If you're fine with it I'll split this up into smaller patches

Why do you not like it? 

The alternative will be likely uglier.

> and resubmit the unmodified parts under your name and redo those bits
> I don't like.

Fine for me.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Dave Chinner]

On Thu, Jun 10, 2010 at 01:10:52PM +0200, Andi Kleen wrote:
> 
> For my configuration, that is without quota or RT.
> 
> Mostly dead code removed I think (but needs additional review) 
>
> That is there were one or two bad error handling cases,
> but they were not easily fixable, with comments 
> and I left the warnings in for those for you to remember.
> 
> e.g. if there is a ENOSPC down in xfs_trans.c while
> modifying the superblock it would not be handled.

See my comments about these below.

> Unused statements were mostly related to stub macros for disabled
> features like QUOTA or RT ALLOC. I replace those with
> inlines.

Did you compile with/without XFS_DEBUG (I don't think so)? when
changing code that affects ASSERT statements, CONFIG_XFS_DEBUG needs to
be selected to test that this code compiles. Most XFs developers
build with XFS_CONFIG_DEBUG for everything other than performance
testing, so ensuring this builds is definitely required. ;)

I'd also be interested if any fixes are needed with all options
enabled. Seems silly just to fix a few warnings in just one
particular configuration rather than all of them...

> There were also some problems with variables used in ASSERT()
> I partly moved those into the ASSERT itself and partly
> used a new QASSERT that always evaluates.

That's a pretty ugly hack for a single occurrence of a warning.
Re-arrnaging the code is a much better idea than introducing a new
ASSERT type. e.g:

-	ASSERT(ref >= 0);
+	if (ref < 0)
+		ASSERT(0);

> Cc: xfs@oss.sgi.com
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  fs/xfs/linux-2.6/xfs_sync.c |    3 +++
>  fs/xfs/support/debug.h      |    4 ++++
>  fs/xfs/xfs_alloc.c          |   10 +++-------
>  fs/xfs/xfs_da_btree.c       |   15 +++++----------
>  fs/xfs/xfs_dir2_block.c     |    6 +++---
>  fs/xfs/xfs_filestream.c     |   10 ++--------
>  fs/xfs/xfs_iget.c           |    3 ---
>  fs/xfs/xfs_inode.c          |    4 ----
>  fs/xfs/xfs_inode_item.c     |    8 ++------
>  fs/xfs/xfs_log.c            |    2 --
>  fs/xfs/xfs_quota.h          |   14 ++++++++++----
>  fs/xfs/xfs_trans.c          |    1 +
>  12 files changed, 33 insertions(+), 47 deletions(-)
> 
> Index: linux-2.6.35-rc2-gcc/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- linux-2.6.35-rc2-gcc.orig/fs/xfs/linux-2.6/xfs_sync.c
> +++ linux-2.6.35-rc2-gcc/fs/xfs/linux-2.6/xfs_sync.c
> @@ -554,6 +554,9 @@ xfs_sync_worker(
>  		xfs_log_force(mp, 0);
>  		xfs_reclaim_inodes(mp, 0);
>  		/* dgc: errors ignored here */
> +		/* ak: yes and you'll get a warning for it now when you
> +		 * upgrade compilers.
> +		 */
>  		error = xfs_qm_sync(mp, SYNC_TRYLOCK);
>  		if (xfs_log_need_covered(mp))
>  			error = xfs_commit_dummy_trans(mp, 0);

I don't think the coment is necessary - the compiler will remind us
that we are ignoring errors.

FWIW, we've now got a situation where external static code checkers
will tell us that we are not handling an error case (which is where
this code and comment came from), and now the compiler will warn us
we are assigning but not using the return value. It's a bit of a
Catch-22 situation. Hence my question is this - how are we supposed
to "safely" ignore a return value seeing as the compiler is now
making the current method rather noisy?

> Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_da_btree.c
> ===================================================================
> --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_da_btree.c
> +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_da_btree.c
> @@ -581,10 +581,8 @@ xfs_da_node_add(xfs_da_state_t *state, x
>  	xfs_da_intnode_t *node;
>  	xfs_da_node_entry_t *btree;
>  	int tmp;
> -	xfs_mount_t *mp;
>  
>  	node = oldblk->bp->data;
> -	mp = state->mp;
>  	ASSERT(be16_to_cpu(node->hdr.info.magic) == XFS_DA_NODE_MAGIC);
>  	ASSERT((oldblk->index >= 0) && (oldblk->index <= be16_to_cpu(node->hdr.count)));
>  	ASSERT(newblk->blkno != 0);

That'll break a CONFIG_XFS_DEBUG build as the next statement:

        if (state->args->whichfork == XFS_DATA_FORK)
                ASSERT(newblk->blkno >= mp->m_dirleafblk &&
                       newblk->blkno < mp->m_dirfreeblk);

uses mp inside ASSERT statements.

> Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_dir2_block.c
> ===================================================================
> --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_dir2_block.c
> +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_dir2_block.c
> @@ -1073,10 +1073,10 @@ xfs_dir2_sf_to_block(
>  	 */
>  
>  	buf_len = dp->i_df.if_bytes;
> -	buf = kmem_alloc(dp->i_df.if_bytes, KM_SLEEP);
> +	buf = kmem_alloc(buf_len, KM_SLEEP);
>  
> -	memcpy(buf, sfp, dp->i_df.if_bytes);
> -	xfs_idata_realloc(dp, -dp->i_df.if_bytes, XFS_DATA_FORK);
> +	memcpy(buf, sfp, buf_len);
> +	xfs_idata_realloc(dp, -buf_len, XFS_DATA_FORK);
>  	dp->i_d.di_size = 0;
>  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  	/*

Just remove the buf_len variable in this case.

> Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_filestream.c
> ===================================================================
> --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_filestream.c
> +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_filestream.c
> @@ -140,9 +140,8 @@ _xfs_filestream_pick_ag(
>  	int		flags,
>  	xfs_extlen_t	minlen)
>  {
> -	int		streams, max_streams;
>  	int		err, trylock, nscan;
> -	xfs_extlen_t	longest, free, minfree, maxfree = 0;
> +	xfs_extlen_t	longest, minfree, maxfree = 0;
>  	xfs_agnumber_t	ag, max_ag = NULLAGNUMBER;
>  	struct xfs_perag *pag;
>  
> @@ -174,7 +173,6 @@ _xfs_filestream_pick_ag(
>  		/* Keep track of the AG with the most free blocks. */
>  		if (pag->pagf_freeblks > maxfree) {
>  			maxfree = pag->pagf_freeblks;
> -			max_streams = atomic_read(&pag->pagf_fstrms);
>  			max_ag = ag;
>  		}
>  
> @@ -196,8 +194,6 @@ _xfs_filestream_pick_ag(
>  		     (flags & XFS_PICK_LOWSPACE))) {
>  
>  			/* Break out, retaining the reference on the AG. */
> -			free = pag->pagf_freeblks;
> -			streams = atomic_read(&pag->pagf_fstrms);
>  			xfs_perag_put(pag);
>  			*agp = ag;
>  			break;
> @@ -234,8 +230,6 @@ next_ag:
>  		if (max_ag != NULLAGNUMBER) {
>  			xfs_filestream_get_ag(mp, max_ag);
>  			TRACE_AG_PICK1(mp, max_ag, maxfree);
> -			streams = max_streams;
> -			free = maxfree;
>  			*agp = max_ag;
>  			break;
>  		}
> @@ -364,7 +358,7 @@ xfs_fstrm_free_func(
>  	/* Drop the reference taken on the AG when the item was added. */
>  	ref = xfs_filestream_put_ag(ip->i_mount, item->ag);
>  
> -	ASSERT(ref >= 0);
> +	QASSERT(ref >= 0);
>  	TRACE_FREE(ip->i_mount, ip, item->pip, item->ag,
>  		xfs_filestream_peek_ag(ip->i_mount, item->ag));

These are all "unused" because they are used in debug code only. i.e.
in XFS_FILESTREAMS_TRACE configs. This is manual debug code that
needs to be converted to the trace infrastructure - the compiler may
say it is unused, but it is not dead code, so shoul dnot be removed.

See also my comment about QASSERT() above.

>  #define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, ninos, flags) \
> Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_trans.c
> ===================================================================
> --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_trans.c
> +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_trans.c
> @@ -1120,6 +1120,7 @@ xfs_trans_unreserve_and_mod_sb(
>  		error = xfs_mod_incore_sb_batch(tp->t_mountp, msb,
>  			(uint)(msbp - msb), rsvd);
>  		ASSERT(error == 0);
> +		/* FIXME: need real error handling here, error can be ENOSPC */

That comment is wrong and hence is not needed.  By design, we should
never, ever get an error here because we've already reserved all the
space we need and this is just an accounting call.  That's what the
ASSERT(error == 0) is documenting. It's ben placed there to catch
any re-occurrence of the race condition that is documented in the
function head comment during development.  Anyway, if we do get an
error here, we cannot handle it anyway - it's too late to do
anything short of a complete shutdown as we've already written the
transaction to the log.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Andi Kleen]

On Mon, Jun 14, 2010 at 02:27:00PM +1000, Dave Chinner wrote:
> > Unused statements were mostly related to stub macros for disabled
> > features like QUOTA or RT ALLOC. I replace those with
> > inlines.
> 
> Did you compile with/without XFS_DEBUG (I don't think so)? when

No.

I merely made my own config work with relatively little warnings.

> changing code that affects ASSERT statements, CONFIG_XFS_DEBUG needs to
> be selected to test that this code compiles. Most XFs developers
> build with XFS_CONFIG_DEBUG for everything other than performance
> testing, so ensuring this builds is definitely required. ;)

Ok fair enough.

> 
> I'd also be interested if any fixes are needed with all options
> enabled. Seems silly just to fix a few warnings in just one
> particular configuration rather than all of them...

There are tons more warnings with allyesconfig I'm sure,
not only in xfs.  They will need time to work out.

This will happen over time as people eventually move
to gcc 4.6 (after it's released)

Some of the warnings are also related
to not enabling everything (e.g. no quota)

> 
> > There were also some problems with variables used in ASSERT()
> > I partly moved those into the ASSERT itself and partly
> > used a new QASSERT that always evaluates.
> 
> That's a pretty ugly hack for a single occurrence of a warning.
> Re-arrnaging the code is a much better idea than introducing a new
> ASSERT type. e.g:

I originally planned to use it for more, but then ended up
changing other code in other ways.

I still don't think it's a ugly hack, it's a relatively
simple way to handle this. But I can change this single occurrence.

> FWIW, we've now got a situation where external static code checkers
> will tell us that we are not handling an error case (which is where
> this code and comment came from), and now the compiler will warn us
> we are assigning but not using the return value. It's a bit of a
> Catch-22 situation. Hence my question is this - how are we supposed
> to "safely" ignore a return value seeing as the compiler is now
> making the current method rather noisy?

Fix the problem?  

Otherwise you can use a (void) cast, but that's a bad way
if there's a real problem.

> >  	dp->i_d.di_size = 0;
> >  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
> >  	/*
> 
> Just remove the buf_len variable in this case.

I think the code looks cleaner when using buf_len

> These are all "unused" because they are used in debug code only. i.e.
> in XFS_FILESTREAMS_TRACE configs. This is manual debug code that
> needs to be converted to the trace infrastructure - the compiler may

I have no plans to do that.

> say it is unused, but it is not dead code, so shoul dnot be removed.

I did not remove anything.

> >  			(uint)(msbp - msb), rsvd);
> >  		ASSERT(error == 0);
> > +		/* FIXME: need real error handling here, error can be ENOSPC */
> 
> That comment is wrong and hence is not needed.  By design, we should
> never, ever get an error here because we've already reserved all the
> space we need and this is just an accounting call.  That's what the
> ASSERT(error == 0) is documenting. It's ben placed there to catch

Ok. But I must say ASSERT() is not really a good way to 
document things like that. Whoever wrote this gets
what he deserves now ...

> function head comment during development.  Anyway, if we do get an
> error here, we cannot handle it anyway - it's too late to do
> anything short of a complete shutdown as we've already written the
> transaction to the log.

Well I guess it should be unconditional BUG_ON then.


-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Dave Chinner]

On Mon, Jun 14, 2010 at 09:43:09AM +0200, Andi Kleen wrote:
> On Mon, Jun 14, 2010 at 02:27:00PM +1000, Dave Chinner wrote:
> > > There were also some problems with variables used in ASSERT()
> > > I partly moved those into the ASSERT itself and partly
> > > used a new QASSERT that always evaluates.
> > 
> > That's a pretty ugly hack for a single occurrence of a warning.
> > Re-arrnaging the code is a much better idea than introducing a new
> > ASSERT type. e.g:
> 
> I originally planned to use it for more, but then ended up
> changing other code in other ways.
> 
> I still don't think it's a ugly hack, it's a relatively
> simple way to handle this. But I can change this single occurrence.
> 
> > FWIW, we've now got a situation where external static code checkers
> > will tell us that we are not handling an error case (which is where
> > this code and comment came from), and now the compiler will warn us
> > we are assigning but not using the return value. It's a bit of a
> > Catch-22 situation. Hence my question is this - how are we supposed
> > to "safely" ignore a return value seeing as the compiler is now
> > making the current method rather noisy?
> 
> Fix the problem?  

There is no problem. The end of the error reporting line is the
main loop of a background kernel thread - anything important is
already stashed for later reporting to a real context - so all that
is left to do is throw away the error once it propagated to the top
of the call chain....

> Otherwise you can use a (void) cast, but that's a bad way
> if there's a real problem.

Right, and that's exactly my point - we removed all the (void)
casts because the error checker flagged them as dangerous. Now the
compiler is complaining about not using the error that is
returned. So my question still stands....

> > >  			(uint)(msbp - msb), rsvd);
> > >  		ASSERT(error == 0);
> > > +		/* FIXME: need real error handling here, error can be ENOSPC */
> > 
> > That comment is wrong and hence is not needed.  By design, we should
> > never, ever get an error here because we've already reserved all the
> > space we need and this is just an accounting call.  That's what the
> > ASSERT(error == 0) is documenting. It's ben placed there to catch
> 
> Ok. But I must say ASSERT() is not really a good way to 
> document things like that. Whoever wrote this gets
> what he deserves now ...

We have historically documented code assumptions and bounds
with ASSERT() calls rather than in comments because it means
they are checked at runtime. It means we find out really quickly
when we've made some change that has had an unintended side effect,
rather than it going unnoticed until some user trips over it.

This one in specific has been there for at least 5 years - goes back
to before git was used and has proven to be useful for finding at
least one subtle race in new code introduced back in 2007
(45c34141126a89da07197d5b89c04c6847f1171a "[XFS] Apply transaction
delta counts atomically to incore counters").

FWIW, there's around 2000 asserts in the XFS code - that's about 2%
of the code - which means assumptions in the XFS code are pretty
well documented compared to other Linux filesystems...

> > function head comment during development.  Anyway, if we do get an
> > error here, we cannot handle it anyway - it's too late to do
> > anything short of a complete shutdown as we've already written the
> > transaction to the log.
> 
> Well I guess it should be unconditional BUG_ON then.

Don't be silly. A filesystem shutdown is all that is necessary,
especially as the XFS shutdown procedure has hooks to turn
corruption events into system panics if the admin wants to configure
it that way (generally useful for triggering crash dumps on
corruption events for offline triage).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Andi Kleen]

> > > function head comment during development.  Anyway, if we do get an
> > > error here, we cannot handle it anyway - it's too late to do
> > > anything short of a complete shutdown as we've already written the
> > > transaction to the log.
> > 
> > Well I guess it should be unconditional BUG_ON then.
> 
> Don't be silly. A filesystem shutdown is all that is necessary,

Without BUG_ON it will not end up in kerneloops.org and you will
never know about it.  That's standard Linux kernel development
practice.

Maybe XFS should catch up on that.

Ok in principle you could make the shutdown a WARN()

Anyways I'm out of this.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Dave Chinner]

On Mon, Jun 14, 2010 at 04:37:20PM +0200, Andi Kleen wrote:
> > > > function head comment during development.  Anyway, if we do get an
> > > > error here, we cannot handle it anyway - it's too late to do
> > > > anything short of a complete shutdown as we've already written the
> > > > transaction to the log.
> > > 
> > > Well I guess it should be unconditional BUG_ON then.
> > 
> > Don't be silly. A filesystem shutdown is all that is necessary,
> 
> Without BUG_ON it will not end up in kerneloops.org and you will
> never know about it. 

We find out about corrupted filesystems all the time from users
sending mail to the list. Even if we did panic by default on
corruption events, kerneloops.org is *useless* for reporting them
because finding out about a corruption is only the very first step
of what is usually a long and involved process that requires user
interaction to gather information necessary to find the cause of the
corruption.

Besides, if we _really_ want the machine to panic on corruption,
then we configure the machine specifically for it via setting the
relevant corruption type bit in /proc/sys/fs/xfs/panic_mask. This is
generally only used when a developer asks a user to set it to get
kernel crash dumps triggered when a corruption event occurs so we
can do remote, offline analysis of the failure.

> That's standard Linux kernel development
> practice.  Maybe XFS should catch up on that.

I find this really amusing because linux filesystems have, over the
last few years, implemented a simpler version of XFS's way of
dealing with corruption events(*). Perhaps you should catch up
with the state of the art before throwing rocks, Andi....

Cheers,

Dave.

(*) extN, fat, hpfs, jfs, nilfs2, ntfs, ocfs2 and logfs all have
configurable corruption event behaviour that default to remount-ro
and can be configured to panic the machine.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Andi Kleen]

> We find out about corrupted filesystems all the time from users
> sending mail to the list. Even if we did panic by default on
> corruption events, kerneloops.org is *useless* for reporting them
> because finding out about a corruption is only the very first step
> of what is usually a long and involved process that requires user
> interaction to gather information necessary to find the cause of the
> corruption.

The idea behind kerneloops.org 
is normally that any single report can be always a flake
(broken memory, hardware, flipped bit whatever).

An error becomes important and interesting when there are multiple
occurrences of it in the field.
> 
> Besides, if we _really_ want the machine to panic on corruption,

BUG_ON is not panic normally.

> then we configure the machine specifically for it via setting the
> relevant corruption type bit in /proc/sys/fs/xfs/panic_mask. This is
> generally only used when a developer asks a user to set it to get
> kernel crash dumps triggered when a corruption event occurs so we
> can do remote, offline analysis of the failure.

Especially when you're talking about desktop class systems
without ECC memory that will mean you'll spend at least some
time on errors which are simply bit flips.

> > That's standard Linux kernel development
> > practice.  Maybe XFS should catch up on that.
> 
> I find this really amusing because linux filesystems have, over the

This has really nothing to do with file systems, it's general
practice for everything (well except XFS) 

> last few years, implemented a simpler version of XFS's way of
> dealing with corruption events(*). Perhaps you should catch up
> with the state of the art before throwing rocks, Andi....

I suspect you miss quite a lot of valuable information from
your user base by not supporting kerneloops.org. On the other
hand it would likely also save you from spending time on 
flakes.

That said you don't need BUG_ON to support it (WARN etc. work
too), it's just the easiest way.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Christoph Hellwig]

On Tue, Jun 15, 2010 at 09:02:45AM +0200, Andi Kleen wrote:
> I suspect you miss quite a lot of valuable information from
> your user base by not supporting kerneloops.org. On the other
> hand it would likely also save you from spending time on 
> flakes.
> 
> That said you don't need BUG_ON to support it (WARN etc. work
> too), it's just the easiest way.

Note that a XFS filesystem shutdown already gives a stack trace.
But picking up every filesystem shutdown on kerneloops.org seems
to be quite a bit too much.  It's usually due to IO errors from
the underlying device.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings

[Andi Kleen]

On Tue, Jun 15, 2010 at 03:40:28AM -0400, Christoph Hellwig wrote:
> On Tue, Jun 15, 2010 at 09:02:45AM +0200, Andi Kleen wrote:
> > I suspect you miss quite a lot of valuable information from
> > your user base by not supporting kerneloops.org. On the other
> > hand it would likely also save you from spending time on 
> > flakes.
> > 
> > That said you don't need BUG_ON to support it (WARN etc. work
> > too), it's just the easiest way.
> 
> Note that a XFS filesystem shutdown already gives a stack trace.
> But picking up every filesystem shutdown on kerneloops.org seems
> to be quite a bit too much.  It's usually due to IO errors from
> the underlying device.

Yes, but known race check asserts should be probably there, right?
Maybe you need a special kind of ASSERT (or shutdown) for those? 

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs